Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Thomas Gleixner <tglx@xxxxxxxxxxxxx>]

On Wed, 12 Jul 2017, Linus Walleij wrote:
> On Tue, Jul 11, 2017 at 7:55 PM, Davide Hug <d@xxxxxxxxxx> wrote:
> 
> > I'm looking at a driver (hwmon/sht15) that does not load with message:
> >
> >         gpio-48 (SHT15 data): _gpiod_direction_output_raw: tried to set a GPIO tied to an IRQ as output
> >
> > This happens because the driver uses a GPIO line alternately (and not at the
> > same time) as IRQ and output. To do this irq_enable/disable are used to
> > prevent setting a GPIO with enabled IRQ as output. This seems the right
> > approach to me since in the patch "gpio: add API to be strict about GPIO IRQ
> > usage"[1] I read:
> >
> >         The API is symmetric so that lines can also be flagged from
> >         .irq_enable() and unflagged from IRQ by .irq_disable().
> >
> > But I didn't find any GPIO driver implementing this API in irq_enable/disable.
> >
> > Since my platform uses omap_gpio which does not implement
> > irq_enable/disable at all, I added a default irq_enable/disable
> > implementation in gpio/gpiolib.c (in the patch below).
> >
> > Is it the right approach to call gpiochip_lock_as_irq/gpiochip_unlock_as_irq
> 
> We already call this in gpiochip_irq_reqres() and gpiochip_irq_relres().
> from the .irq_request_resources()/.irq_release_resources() callbacks.
> 
> Your patch makes us call the functions twice I think.
> 
> Why isn't gpiochip_irq_relres() called before you reuse the line
> for something else than IRQ?
>
> 
> If there is a legitimate reason why .irq_disable() and .irq_enable()
> are called but not .irq_request_resources() and .irq_release_resources()
> I want to know why that is so.

Well, because that driver is completely broken.

It does:

	ret = devm_gpio_request(&pdev->dev, data->pdata->gpio_data,
                                "SHT15 data");

	request_irq(gpio_to_irq(data->pdata->gpio_data),....);

and at random places it does:

	ret = gpio_direction_input(data->pdata->gpio_data);
	
	err = gpio_direction_output(data->pdata->gpio_data, 1);

and the 'protection' for that is to use disable_irq_nosync() and
enable_irq() at various places.

That of course breaks with the current semantics of the GPIO interrupt
facility, because it prevents switching a requested interrupt gpio line
from input to output.

The only sane option is to get rid of the interrupt and switch the whole
thing to timer based polling. I'm sure that's not more overhead and removes
half of the unpenetrable mess in that drivere.

Thanks,

	tglx


    
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html