Re: [alsa-devel] [PATCH] ASoC: Add GPIO support for jack reporting interface

At Mon, 2 Mar 2009 19:05:28 -0600, Lopez Cruz, Misael wrote:

Add GPIO support to jack reporting framework in ASoC using gpiolib calls. The gpio support exports two new functions: snd_soc_jack_add_gpios and snd_soc_jack_free_gpios.

Client drivers using gpio feature must pass an array of jack_gpio pins belonging to a specific jack to the snd_soc_jack_add_gpios function. The framework will request the gpio, set the data direction and request irq. The framework will update power status of related jack_pins when an event on the gpio pins comes according to the reporting bits defined for each gpio.

All gpio resources allocated when adding jack_gpio pins for a particular jack can be released using snd_soc_jack_free_gpios function.

Signed-off-by: Misael Lopez Cruz x0052729@ti.com

I prefer GPIO stuff built in conditionally. The jack layer is used also for non-ASoC codes, and GPIO isn't always present.

+/* irq handler for gpio pin */ +static irqreturn_t gpio_handler(int irq, void *data) +{

• struct snd_soc_jack_gpio *gpio = data;
• return IRQ_RETVAL(schedule_work(&gpio->work));

Is this really correct?

+int snd_soc_jack_add_gpios(struct snd_soc_jack *jack, int count,

• 	struct snd_soc_jack_gpio *gpios)
  

+{

• int i, ret = 0;
• /* Link gpio array with the soc_jack */
• if (count > 0)

• jack->gpios = gpios;
  

• for (i = 0; i < count; i++) {

• if (!gpio_is_valid(gpios[i].gpio)) {
  

• 	printk(KERN_ERR "Invalid gpio %d\n",
  

• 		gpios[i].gpio);
  

• 	return -EINVAL;
  

• }
  

• if (!gpios[i].name) {
  

• 	printk(KERN_ERR "No name for gpio %d\n",
  

• 		gpios[i].gpio);
  

• 	return -EINVAL;
  

• }
  

• ret = gpio_request(gpios[i].gpio, gpios[i].name);
  

• if (ret)
  

• 	return ret;
  

• ret = gpio_direction_input(gpios[i].gpio);
  

• if (ret)
  

• 	break;
  

• ret = request_irq(gpio_to_irq(gpios[i].gpio),
  

• 		gpio_handler,
  

• 		IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
  

• 		jack->card->dev->driver->name,
  

• 		&gpios[i]);
  

• if (ret)
  

• 	break;
  

• INIT_WORK(&gpios[i].work, gpio_work);
  

• gpios[i].jack = jack;
  

• }
• if (ret)

• gpio_free(gpios[i].gpio);
  

This error path doesn't look good. It seems leaking / keeping some resources.

thanks,

Takashi