Re: [RFCv2 PATCH 2/4] gpio: remove gpiochip_(un)lock_as_irq() from drivers

[Hans Verkuil <hverkuil@xxxxxxxxx>]

On 08/29/2018 09:30 AM, Linus Walleij wrote:
> On Mon, Aug 27, 2018 at 3:06 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> 
>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>> Remove all calls to gpiochip_(un)lock_as_irq from drivers as this
>> is now done by gpiolib.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> 
>>  drivers/gpio/gpio-bcm-kona.c             | 24 ------------------
>>  drivers/gpio/gpio-dwapb.c                | 27 --------------------
>>  drivers/gpio/gpio-em.c                   | 24 ------------------
>>  drivers/gpio/gpio-tegra.c                | 19 --------------
>>  drivers/gpio/gpio-thunderx.c             | 15 +++--------
>>  drivers/gpio/gpio-uniphier.c             | 20 ---------------
>>  drivers/gpio/gpio-vr41xx.c               | 11 --------
>>  drivers/pinctrl/mediatek/mtk-eint.c      |  9 -------
>>  drivers/pinctrl/samsung/pinctrl-exynos.c | 10 --------
>>  drivers/pinctrl/stm32/pinctrl-stm32.c    | 16 ------------
>>  drivers/pinctrl/sunxi/pinctrl-sunxi.c    | 19 --------------
> 
> Are you sure about these? This driver does not use GPIOLIB_IRQCHIP
> (no select GPIOLIB_IRQCHIP in Kconfig).
> 
> Notice that gpiochip_lock_as_irq() used to be outside of
> #define CONFIG_GPIOLIB_IRQCHIP so these drivers can
> lock the IRQ without the help of the gpiolib IRQCHIP library,
> as they implement their own irqchips.

Right. So there are two classes of drivers: those that use GPIOLIB_IRQCHIP
and those that don't. Those that do use it should never call
gpiochip_lock_as_irq(), since it is now done for them, and those that do not
use it *should* call these functions in their own hooks.

This means that gpiochip_lock_as_irq() should check if it is called from
the wrong class of driver:

#ifdef CONFIG_GPIOLIB_IRQCHIP
	if (WARN_ON(gpiochip->irq.chip))
		return 0;
#endif

(I think that's the right check)

It also means that drivers not using GPIOLIB_IRQCHIP can be updated
one at a time. They are not broken as such, it is just that gpiochip_lock_as_irq()
is called when the interrupt is requested/freed instead of when the interrupt
is enabled/disabled.

> I think this and similar drivers simply have to assign their .startup()
> and .shutdown() etc hooks locally in the driver, becuase since
> gpiochip_add_irqchip() is not called, they do not get set up by the
> gpiolib core.
> 
>>  drivers/gpio/gpio-xgene-sb.c             | 10 --------
>>  drivers/hid/hid-cp2112.c                 | 14 ++---------
>>  drivers/pinctrl/pinctrl-st.c             | 10 +-------
> 
> This is fine (uses GPIOLIB_IRQCHIP)
> 
>>  drivers/gpio/gpiolib-acpi.c              | 13 ++--------
> 
> Uncertain about this one.

>From what I can tell this does not use GPIOLIB_IRQCHIP, so this patch is wrong.
I don't think this driver needs to be changed at all.

> 
>>  drivers/gpio/gpiolib-sysfs.c             | 18 +------------
> 
> OK

I think this wrong as well: this can be used with drivers that use GPIOLIB_IRQCHIP
and those that do not use it. So it has to check.

> 
>>  drivers/pinctrl/intel/pinctrl-intel.c    | 32 ------------------------
> 
> I think this is fine, but check.

Looks good to me.

Regards,

	Hans

> 
> Yours,
> Linus Walleij
>