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

[Lopez Cruz, Misael]

> > 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.

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

Is it ok if I enclose all gpio functionality with #ifdef CONFIG_GPIOLIB?
 
> > +/* 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?

Changed to:

static irqreturn_t gpio_handler(int irq, void *data)
{
	struct snd_soc_jack_gpio *gpio = data;

	schedule_work(&gpio->work);
	return IRQ_HANDLED;
}

> > +int snd_soc_jack_add_gpios(struct snd_soc_jack *jack, int count,
> > +			struct snd_soc_jack_gpio *gpios)

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

Changed to:

int snd_soc_jack_add_gpios(struct snd_soc_jack *jack, int count,
                        struct snd_soc_jack_gpio *gpios)
{
	int i, ret;

	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) {
			gpio_free(gpios[i].gpio);
			return ret;
		}

		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) {
			gpio_free(gpios[i].gpio);
			return ret;
		}

		INIT_WORK(&gpios[i].work, gpio_work);
		gpios[i].jack = jack;
	}

	return 0;
}