Re: [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode

[Hans Verkuil <hverkuil@xs4all.nl>]

On 7/26/22 16:42, Marek Vasut wrote:
> On 7/26/22 10:15, Linus Walleij wrote:
>> On Mon, Jul 25, 2022 at 12:50 AM Marek Vasut <marex@denx.de> wrote:
>>
>>> Always configure GPIO pins which are used as interrupt source as INPUTs.
>>> In case the default pin configuration is OUTPUT, or the prior stage does
>>> configure the pins as OUTPUT, then Linux will not reconfigure the pin as
>>> INPUT and no interrupts are received.
>>>
>>> Always configure interrupt source GPIO pin as input to fix the above case.
>>>
>>> Fixes: 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform")
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: Loic Poulain <loic.poulain@linaro.org>
>>> Cc: Marc Zyngier <maz@kernel.org>
>>> Cc: NXP Linux Team <linux-imx@nxp.com>
>>> Cc: Peng Fan <peng.fan@nxp.com>
>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>> ---
>>> V2: Actually update and clear bits in GDIR register
>>> V3: Rebase on top of new patch 1/2, expand CC list, add Fixes tag
>>> V4: No change
>>
>> I understand what you are trying to achieve, and it makes sense.
>>
>> There's is just this one generic GPIO-based driver that makes me
>> a little bit nervous here.
>>
>> Consider:
>> drivers/media/cec/platform/cec-gpio/cec-gpio.c
>> Look what the driver is doing with the gpiod_* operations on it's
>> cec->cec_gpio.
>>
>> A certain GPIO pin is switched back and forth between input and
>> output and in input mode, it is used to generate interrupts as well.
>>
>> Will this still work fine with the MXC driver after this change?
>> At least it will be set to input mode twice, but I suppose that is
>> fine, it's not your fault that the frameworks are orthogonal.
> 
> Ugh. I don't see why it shouldn't work, esp. if the CEC driver controls the direction and the default is input, but I wonder what other corner cases there are.

FYI: you can easily test cec-gpio by adding something along these lines to the dts:

	cec-gpio {
		compatible = "cec-gpio";
		cec-gpios = <&gpio 6 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
	};

(this is from a Raspberry Pi). As long as the cec-gpios pin can be configured as a
pull up, then you're OK. It doesn't have to be connected to anything.

If the cec-gpio driver is enabled as well, then you should see a /dev/cec0 device.

If you can run 'cec-ctl --playback -p 1.0.0.0', then it works.

Another driver that switches direction is drivers/gpu/drm/i2c/tda998x_drv.c
where the irq line has to be configured as an output during calibration.
See tda998x_cec_calibration().

Regards,

	Hans