Re: [PATCH] gpio: zynq: Read using DATA register when direction is output

[Michal Simek <michal.simek@amd.com>]

On 4/10/25 10:25, Michal Simek wrote:
> From: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
> 
> Gpio status command reads the DATA_RO register rather than
> DATA registers even when the direction is "output", fix this
> by reading from DATA register when direction is "output".
> 
> Signed-off-by: Nam Ian <ian.nam@amd.com>
> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
> Signed-off-by: Michal Simek <michal.simek@amd.com>
> ---
> 
>   drivers/gpio/zynq_gpio.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/zynq_gpio.c b/drivers/gpio/zynq_gpio.c
> index 7db58c70663e..4fdce39d91b6 100644
> --- a/drivers/gpio/zynq_gpio.c
> +++ b/drivers/gpio/zynq_gpio.c
> @@ -64,6 +64,7 @@
>   /* MSW Mask & Data -WO */
>   #define ZYNQ_GPIO_DATA_MSW_OFFSET(BANK)	(0x004 + (8 * BANK))
>   /* Data Register-RW */
> +#define ZYNQ_GPIO_DATA_OFFSET(BANK)	(0x040 + (4 * BANK))
>   #define ZYNQ_GPIO_DATA_RO_OFFSET(BANK)	(0x060 + (4 * BANK))
>   /* Direction mode reg-RW */
>   #define ZYNQ_GPIO_DIRM_OFFSET(BANK)	(0x204 + (0x40 * BANK))
> @@ -230,7 +231,7 @@ static int check_gpio(unsigned gpio, struct udevice *dev)
>   
>   static int zynq_gpio_get_value(struct udevice *dev, unsigned gpio)
>   {
> -	u32 data;
> +	u32 data, reg;
>   	unsigned int bank_num, bank_pin_num;
>   	struct zynq_gpio_plat *plat = dev_get_plat(dev);
>   
> @@ -239,9 +240,15 @@ static int zynq_gpio_get_value(struct udevice *dev, unsigned gpio)
>   
>   	zynq_gpio_get_bank_pin(gpio, &bank_num, &bank_pin_num, dev);
>   
> -	data = readl(plat->base +
> -			     ZYNQ_GPIO_DATA_RO_OFFSET(bank_num));
> -
> +	reg = readl(plat->base + ZYNQ_GPIO_DIRM_OFFSET(bank_num));
> +	reg &= BIT(bank_pin_num);
> +	if (reg != GPIOF_INPUT) {
> +		data = readl(plat->base +
> +					ZYNQ_GPIO_DATA_OFFSET(bank_num));
> +	} else {
> +		data = readl(plat->base +
> +					ZYNQ_GPIO_DATA_RO_OFFSET(bank_num));
> +	}
>   	return (data >> bank_pin_num) & 1;
>   }
>   

I have looked at Linux kernel and there is different logic. It should be syncup.
Second. I agree with Mike that what we need is to read actual value not really 
value which has been written.

Thanks,
Michal