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

[Takashi Iwai <tiwai@suse.de>]

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