[PATCH v2] gpio: mxc_gpio: fix reading state of GPIO pins in output mode

[Marek Vasut]

On 8/28/24 4:16 AM, Fabio Estevam wrote:
> Adding more folks on Cc to help review it.
> 
> 
> On Tue, Aug 27, 2024 at 11:13 PM Tomas Paukrt <tomaspaukrt at email.cz> wrote:
>>
>> The PSR register works correctly for GPIO pins in input mode,
>> but always returns 0 for GPIO pins in output mode unless the SION
>> bit is set. The DR register works correctly in both modes without
>> the need to set the SION bit.

I think this claim is not correct.

There is a difference between PSR and DR , PSR reports the sensed state 
on the pad, the DR reports the state that was set on the pad by user. 
Those may differ, e.g. in case the pin is not even configured as a GPIO 
but has a IOMUXC SION bit set, it is still possible to read back the 
state of the pad via PSR register, but not via DR register.

For more details, see discussion around similar (rejected) patch to 
Linux kernel:

https://lore.kernel.org/all/CAJ+vNU3w9Oi+dErmy9x8g6ps=eLHLNLO-w7=gn_8JtY4Kab6JQ@mail.gmail.com/

>> Please note that the Linux gpio-mxc driver already uses
>> the DR register for all GPIO pins in output mode:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=442b2494b17d1a4f0a14721580271eb23ebffd42

The flag BGPIOF_READ_OUTPUT_REG_SET indicates that for GPIOs which are 
set as direction-output, the state is read out of DR register, but for 
GPIOs which are direction-input, the state is read out of PSR register.

[...]

>> Signed-off-by: Tomas Paukrt <tomaspaukrt at email.cz>
>> ---
>>   drivers/gpio/mxc_gpio.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
>> index cac6b32..5cd7681 100644
>> --- a/drivers/gpio/mxc_gpio.c
>> +++ b/drivers/gpio/mxc_gpio.c
>> @@ -133,7 +133,7 @@ int gpio_get_value(unsigned gpio)
>>
>>          regs = (struct gpio_regs *)gpio_ports[port];
>>
>> -       val = (readl(&regs->gpio_psr) >> gpio) & 0x01;
>> +       val = (readl(&regs->gpio_dr) >> gpio) & 0x01;
>>
>>          return val;
>>   }
>> @@ -210,7 +210,7 @@ static void mxc_gpio_bank_set_value(struct gpio_regs *regs, int offset,
>>
>>   static int mxc_gpio_bank_get_value(struct gpio_regs *regs, int offset)
>>   {
>> -       return (readl(&regs->gpio_psr) >> offset) & 0x01;
>> +       return (readl(&regs->gpio_dr) >> offset) & 0x01;
>>   }

This change is not correct, it does unconditionally switch readback from 
PSR to DR and does not differentiate between input/output direction of 
the GPIO. This would have to be fixed in V3 to make that patch on par 
with the Linux behavior.