| Message ID | 2C7D3DF36ADFFC479B44490D912B616705EC8B1C03@dlee07.ent.ti.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
On Thu, Feb 26, 2009 at 01:57:03AM -0600, Lopez Cruz, Misael wrote: > struct snd_soc_jack_pin { > + struct snd_soc_jack *jack; > + struct snd_soc_jack_gpio *gpio_pin; > struct list_head list; > const char *pin; > int mask; > bool invert; > + /* GPIO */ > + unsigned int gpio; > + unsigned int irq; > + unsigned long irqflags; > + irq_handler_t handler; > + struct work_struct work; > }; This needs to be rethought - it breaks the abstraction layers. There are three things working together here: - The snd_soc_jack, which represents a physical jack on the system and is what is visible to user space. - The snd_soc_jack_pin, which represents a DAPM pin to update depending on some of the status bits supported by the jack. Each snd_soc_jack has zero or more of these which are updated automatically. - The jack reporting mechanism, which represents something that can do detection - it is associated with a snd_soc_jack, reporting a subset of the status bits supported by the snd_soc_jack. Each jack may have multiple reporting mechanisms, though it will need at least one to be useful. These are all hooked together by the machine driver depending on the system hardware. The machine driver will set up the snd_soc_jack and the list of pins to update then set up one or more jack detection mechanisms to update that jack based on their current status. For example, a system may have a stereo headset jack with two reporting mechansms, one for the headphone and one for the microphone. Some systems won't be able to use their speaker output while a headphone is connected and so will want to make sure to update both speaker and headphone when the headphone jack status changes. The GPIO jack detection code should operate only on a snd_soc_jack that is provided to it by a machine driver - you'll need to define a structure to hold the information the GPIO jack detection needs (there is a structure there but you've not defined it so I'm not sure what you have in it at the minute). Please look at the wm8350 headphone detection for an example of how to integrate a detection mechanism. > +static void gpio_work(struct work_struct *work) > +{ > + struct snd_soc_jack_pin *pin; > + int report; > + > + pin = container_of(work, struct snd_soc_jack_pin, work); > + report = pin->jack->status & pin->mask; > + if (gpio_get_value(pin->gpio)) > + report |= pin->mask; > + else > + report &= ~pin->mask; > + > + snd_soc_jack_report(pin->jack, report, pin->jack->jack->type); > +} The value to report should be supplied by the machine driver. BTW, please remember to CC the maintainers of the things you're submitting patches for. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 25 February 2009, Lopez Cruz, Misael wrote: > +Â Â Â Â Â Â Â unsigned int gpio; Use "int" not unsigned for such might-be-a-GPIO codes ... > +Â Â Â Â Â Â Â unsigned int irq; > +Â Â Â Â Â Â Â unsigned long irqflags; > +Â Â Â Â Â Â Â irq_handler_t handler; > +Â Â Â Â Â Â Â struct work_struct work; > Â }; > Â > +#define NO_JACK_PIN_GPIOÂ Â Â Â Â Â Â UINT_MAX And any negative number to flag "no GPIO"; "-EINVAL" for example. > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (pins[i].gpio != NO_JACK_PIN_GPIO) { Make that: if (gpio_is_valid(pins[i].gpio)) ... -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > struct snd_soc_jack_pin { > > + struct snd_soc_jack *jack; > > + struct snd_soc_jack_gpio *gpio_pin; > > struct list_head list; > > const char *pin; > > int mask; > > bool invert; > > + /* GPIO */ > > + unsigned int gpio; > > + unsigned int irq; > > + unsigned long irqflags; > > + irq_handler_t handler; > > + struct work_struct work; > > }; > > This needs to be rethought - it breaks the abstraction layers. Ok, I see. Here is the new plan: * Create a new structure "snd_soc_jack_gpio" holding info specific for a gpio pin like: gpio, irq, irqflags, irqhandler, private data (to be passed to irqhandler). * Create a new function "snd_soc_jack_add_gpios" to add all jack_gpios that belong to a specific jack. This function should add all gpio pin references in a linked list as it's done for dapm pins. The linked list will be useful to be able to release acquired resources in another function "snd_soc_jack_free_gpios". * Machine driver will be responsible to call add_gpios function passing an array of gpios related to each jack. * Machine driver will tie each jack_gpio with corresponding jack in a machine specific jack_data structure, one hook per jack_gpio in the jack. A handler will also be associated to the jack_data structure. This jack_data struct will be passed to the gpio irqhandler as private data. -Misa-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 27, 2009 at 04:52:35AM -0600, Lopez Cruz, Misael wrote: > * Create a new structure "snd_soc_jack_gpio" holding info specific for a > gpio pin like: gpio, irq, irqflags, irqhandler, private data (to be > passed to irqhandler). Yes, roughly. The jack_gpio will also need to know the status bits to update and which jack to update. I'd expect something along the lines of: struct snd_soc_jack_gpio { struct snd_soc_jack *jack; int report; /* Value to report when jack detected */ int invert_report; /* Report presence when GPIO low */ int gpio; /* GPIO to read */ }; possibly with some other data stored (eg, a debounce time). You can use gpio_to_irq() to get the interrupt number. If the machine drivers need to customise the IRQ handler code itself then it's probably getting to the point where another detection method should be written, though perhaps I'm missing something? > * Create a new function "snd_soc_jack_add_gpios" to add all jack_gpios that > belong to a specific jack. This function should add all gpio pin references > in a linked list as it's done for dapm pins. The linked list will be > useful to be able to release acquired resources in another function > "snd_soc_jack_free_gpios". Since the detection mechanism will need to know the jack it's notifying it should be possible to set up every GPIO detector at once - they'll all have to know which jack to point to anyway. Given that it'd be as easy to use an array and not bother with the linked list. The reason the pins are added to a list is that we need to iterate over them all whenever the jack status changes to update the status of the pins. > * Machine driver will be responsible to call add_gpios function passing an > array of gpios related to each jack. Yes, the machine driver should set up that link. > * Machine driver will tie each jack_gpio with corresponding jack in a > machine specific jack_data structure, one hook per jack_gpio in the jack. > A handler will also be associated to the jack_data structure. This > jack_data struct will be passed to the gpio irqhandler as private data. I'd *expect* you can live without the custom handler, though I could be wrong. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > * Create a new structure "snd_soc_jack_gpio" holding info > > specific for a gpio pin like: gpio, irq, irqflags, > > irqhandler, private data (to be passed to irqhandler). > > Yes, roughly. The jack_gpio will also need to know the > status bits to update and which jack to update. I'd expect > something along the lines > of: > > struct snd_soc_jack_gpio { > struct snd_soc_jack *jack; > int report; /* Value to report when jack detected */ > int invert_report; /* Report presence when GPIO low */ > int gpio; /* GPIO to read */ > }; > > possibly with some other data stored (eg, a debounce time). > You can use gpio_to_irq() to get the interrupt number. > > If the machine drivers need to customise the IRQ handler code > itself then it's probably getting to the point where another > detection method should be written, though perhaps I'm > missing something? Well, customise the IRQ handler itself probably not since the irq handler only needs to queue a work for doing the actual detection/report. There can be a generic detection/report function for gpio, I was thinking in something like: void gpio_detect(struct snd_soc_jack_gpio *gpio) { struct snd_soc_jack *jack = gpio->jack; int enable; int report; if (gpio->debounce_time > 0) mdelay(gpio->debounce_time); enable = gpio_get_value(gpio->gpio); if (gpio->invert) enable != enable; if (enable) report = gpio->report; else report = 0; snd_soc_jack_report(jack, report, gpio->report); } This way we will be updating only bits associated to that particular gpio. But in a previous mail you mentioned about a case: > Some systems won't be able to use their speaker output while a > headphone is connected and so will want to make sure to update > both speaker and headphone when the headphone jack status > changes. Having a single/generic report function like shown above (as is) we can't handle that case. Could we leave the actual implementation of this report function to the machine driver? Since the things being done in detection function are common (even if other status are wanted to be updated), then probably machine driver could define a specific function ("action") for doing extra tasks, it can be called from generic gpio detect function. Could it be a valid approach? -Misa -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Mar 01, 2009 at 07:54:36PM -0600, Lopez Cruz, Misael wrote: > detection/report. There can be a generic detection/report function > for gpio, I was thinking in something like: Yes, that's more or less much what I was looking for. > > Some systems won't be able to use their speaker output while a > > headphone is connected and so will want to make sure to update > > both speaker and headphone when the headphone jack status > > changes. > Having a single/generic report function like shown above (as is) we > can't handle that case. Sure it can - remember that the DAPM updates are done separately to the detection and can be set to invert the status reported for the power. The speaker won't be visible as a jack in a situation like this. > Could we leave the actual implementation of this report function to > the machine driver? Since the things being done in detection function > are common (even if other status are wanted to be updated), then > probably machine driver could define a specific function ("action") > for doing extra tasks, it can be called from generic gpio detect > function. Could it be a valid approach? That sounds like adding a callback for power updates on the jack itself to me (which isn't a bad idea), rather than changing the report function of the jack detection method. The need for machine-specific extra actions probably isn't specific to jacks that are detected via GPIOs. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Could we leave the actual implementation of this report function to > > the machine driver? Since the things being done in detection function > > are common (even if other status are wanted to be updated), then > > probably machine driver could define a specific function ("action") > > for doing extra tasks, it can be called from generic gpio detect > > function. Could it be a valid approach? > That sounds like adding a callback for power updates on the jack itself > to me (which isn't a bad idea), rather than changing the report function > of the jack detection method. The need for machine-specific extra > actions probably isn't specific to jacks that are detected via GPIOs. In that situation, power updates should come only when the jack reporting bits are either all active (jack enabled) or none (jack disabled), is that correct? If so, then machine drivers can create callbacks receiving the soc_codec the jack belongs to and the current state of the jack. All the power updates (dapm_enable_pin/damp_disable_pin) will happen in the callback in machine driver but the dapm sync will happen in soc jack framework (i.e. when reporting current status of the jack). -Misa-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 02, 2009 at 03:16:22PM -0600, Lopez Cruz, Misael wrote: > > That sounds like adding a callback for power updates on the jack itself > > to me (which isn't a bad idea), rather than changing the report function > > of the jack detection method. The need for machine-specific extra > > actions probably isn't specific to jacks that are detected via GPIOs. > In that situation, power updates should come only when the jack reporting > bits are either all active (jack enabled) or none (jack disabled), is that > correct? No, the jack detect bits can change independently - you won't always physically be able to get everything that can be detected to report at once (eg, something that can distinguish between headphones and line output) or things may not always be present together (eg, a jack could detect headphones but no microphone even if it's possible that both may be present simultaneously). > If so, then machine drivers can create callbacks receiving the soc_codec > the jack belongs to and the current state of the jack. All the power updates > (dapm_enable_pin/damp_disable_pin) will happen in the callback in machine > driver but the dapm sync will happen in soc jack framework (i.e. when > reporting current status of the jack). No. The framework takes care of doing the power updates already, there is no need to add that functionality since the machine drivers can just use a simple data table. A callback could be used to do things that can't be coped with already or more complex updates that aren't simply directly mirroring the status in DAPM pins. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > That sounds like adding a callback for power updates on the jack > > > itself to me (which isn't a bad idea), rather than changing the > > > report function of the jack detection method. The need for > > > machine-specific extra actions probably isn't specific to jacks > > > that are detected via GPIOs. > > In that situation, power updates should come only when the jack > > reporting bits are either all active (jack enabled) or none (jack > > disabled), is that correct? > No, the jack detect bits can change independently - you won't always > physically be able to get everything that can be detected to report at > once (eg, something that can distinguish between headphones and line > output) or things may not always be present together (eg, a jack could > detect headphones but no microphone even if it's possible that both > may be present simultaneously). Then, when to trigger the callback? Every time jack status is going to be updated? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 02, 2009 at 06:03:12PM -0600, Lopez Cruz, Misael wrote: > Then, when to trigger the callback? Every time jack status is going to > be updated? Yes. Note that if we're going to add a callback it should be done as a separate patch since it's a separate feature. It's probably as well to get the GPIO jack detection and support for your system merged and then worry about any callback separately. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Then, when to trigger the callback? Every time jack status > > is going to be updated? > Yes. Note that if we're going to add a callback it should be > done as a separate patch since it's a separate feature. It's > probably as well to get the GPIO jack detection and support for > your system merged and then worry about any callback separately. Ok, then I'll post again GPIO patches. Thanks.-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/sound/soc.h b/include/sound/soc.h index 68d8149..846e2c1 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -16,6 +16,8 @@ #include <linux/platform_device.h> #include <linux/types.h> #include <linux/workqueue.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> #include <sound/core.h> #include <sound/pcm.h> #include <sound/control.h> @@ -254,14 +256,27 @@ int snd_soc_put_volsw_s8(struct snd_kcontrol *kcontrol, * @pin: name of the pin to update * @mask: bits to check for in reported jack status * @invert: if non-zero then pin is enabled when status is not reported + * @gpio: gpio number associated to the pin (gpiolib calls will be used) + * @irqflags IRQ flags + * @handler: handler for servicing interrupt events on gpio pin */ struct snd_soc_jack_pin { + struct snd_soc_jack *jack; + struct snd_soc_jack_gpio *gpio_pin; struct list_head list; const char *pin; int mask; bool invert; + /* GPIO */ + unsigned int gpio; + unsigned int irq; + unsigned long irqflags; + irq_handler_t handler; + struct work_struct work; }; +#define NO_JACK_PIN_GPIO UINT_MAX + struct snd_soc_jack { struct snd_jack *jack; struct snd_soc_card *card; diff --git a/sound/soc/soc-jack.c b/sound/soc/soc-jack.c index 8cc00c3..0d048b2 100644 --- a/sound/soc/soc-jack.c +++ b/sound/soc/soc-jack.c @@ -14,6 +14,9 @@ #include <sound/jack.h> #include <sound/soc.h> #include <sound/soc-dapm.h> +#include <linux/gpio.h> +#include <linux/interrupt.h> +#include <linux/workqueue.h> /** * snd_soc_jack_new - Create a new jack @@ -96,6 +99,32 @@ out: } EXPORT_SYMBOL_GPL(snd_soc_jack_report); +/* Default IRQ handler for a GPIO jack pin, it will queue a + * work that reads current value in GPIO pin and reports it + * to the jack framework. + */ +static irqreturn_t gpio_interrupt(int irq, void *data) +{ + struct snd_soc_jack_pin *pin = data; + + return IRQ_RETVAL(schedule_work(&pin->work)); +} + +static void gpio_work(struct work_struct *work) +{ + struct snd_soc_jack_pin *pin; + int report; + + pin = container_of(work, struct snd_soc_jack_pin, work); + report = pin->jack->status & pin->mask; + if (gpio_get_value(pin->gpio)) + report |= pin->mask; + else + report &= ~pin->mask; + + snd_soc_jack_report(pin->jack, report, pin->jack->jack->type); +} + /** * snd_soc_jack_add_pins - Associate DAPM pins with an ASoC jack * @@ -106,11 +135,18 @@ EXPORT_SYMBOL_GPL(snd_soc_jack_report); * After this function has been called the DAPM pins specified in the * pins array will have their status updated to reflect the current * state of the jack whenever the jack status is updated. + * + * A GPIO pin (using gpiolib) can be used to detect events. It requieres + * an IRQ handler and flags to be set in jack_pin structure; if they are + * not provided, default handler/flags will be used instead. If this + * feature is not desired, "gpio" field of jack_pin structure must be + * set to NO_JACK_PIN_GPIO. */ int snd_soc_jack_add_pins(struct snd_soc_jack *jack, int count, struct snd_soc_jack_pin *pins) { - int i; + unsigned int gpio = 0; + int i, ret = 0; for (i = 0; i < count; i++) { if (!pins[i].pin) { @@ -123,6 +159,32 @@ int snd_soc_jack_add_pins(struct snd_soc_jack *jack, int count, return -EINVAL; } + if (pins[i].gpio != NO_JACK_PIN_GPIO) { + pins[i].jack = jack; + gpio = pins[i].gpio; + ret = gpio_request(gpio, pins[i].pin); + if (ret) + return ret; + + ret = gpio_direction_input(gpio); + if (ret) + goto out; + pins[i].irq = gpio_to_irq(gpio); + /* If none set, use the default handler */ + if (!pins[i].handler) { + pins[i].handler = gpio_interrupt; + pins[i].irqflags = IRQF_TRIGGER_RISING | + IRQF_TRIGGER_FALLING; + INIT_WORK(&pins[i].work, gpio_work); + } + ret = request_irq(pins[i].irq, + pins[i].handler, + pins[i].irqflags, + jack->card->dev->driver->name, + &pins[i]); + if (ret) + goto out; + } INIT_LIST_HEAD(&pins[i].list); list_add(&(pins[i].list), &jack->pins); } @@ -133,6 +195,10 @@ int snd_soc_jack_add_pins(struct snd_soc_jack *jack, int count, */ snd_soc_jack_report(jack, 0, 0); - return 0; +out: + if (ret) + gpio_free(gpio); + + return ret; } EXPORT_SYMBOL_GPL(snd_soc_jack_add_pins);
Add GPIO support for jack reporting framework in ASoC, by using gpiolib calls. It's only required to append GPIO information (gpio number, irq handler and flags) to standard jack_pin declaration. GPIO request, data direction configuration and irq request are handled by the utility. The minimal GPIO information that can be provided is the gpio number, in that case default handler/flags will be used. Default handler queues a work that reads the current value in the gpio pin and informs to the jack framework. If the GPIO support is not required, the "gpio" field ot jack_pin structure must be set to NO_JACK_PIN_GPIO. Signed-off-by: Misael Lopez Cruz <x0052729@ti.com> --- include/sound/soc.h | 15 ++++++++++ sound/soc/soc-jack.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 2 deletions(-)