[U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs

[Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>]

Hi Michal,

Am 23.07.2018 um 13:43 schrieb Michal Simek:
> Reading registers for finding out output value is not working because
> input value is read instead in case of tristate.
>
> Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>   drivers/gpio/xilinx_gpio.c | 38 +++++++++++++++++++++++++++++++++-----
>   1 file changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c
> index 4da9ae114d87..9d3e9379d0e5 100644
> --- a/drivers/gpio/xilinx_gpio.c
> +++ b/drivers/gpio/xilinx_gpio.c
> @@ -358,6 +358,11 @@ struct xilinx_gpio_platdata {
>   	int bank_max[XILINX_GPIO_MAX_BANK];
>   	int bank_input[XILINX_GPIO_MAX_BANK];
>   	int bank_output[XILINX_GPIO_MAX_BANK];
> +	u32 dout_default[XILINX_GPIO_MAX_BANK];
> +};
> +
> +struct xilinx_gpio_privdata {
> +	u32 output_val[XILINX_GPIO_MAX_BANK];
>   };
>   
>   static int xilinx_gpio_get_bank_pin(unsigned offset, u32 *bank_num,
> @@ -387,6 +392,7 @@ static int xilinx_gpio_set_value(struct udevice *dev, unsigned offset,
>   				 int value)
>   {
>   	struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
> +	struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>   	int val, ret;
>   	u32 bank, pin;
>   
> @@ -394,19 +400,21 @@ static int xilinx_gpio_set_value(struct udevice *dev, unsigned offset,
>   	if (ret)
>   		return ret;
>   
> -	debug("%s: regs: %lx, value: %x, gpio: %x, bank %x, pin %x\n",
> -	      __func__, (ulong)platdata->regs, value, offset, bank, pin);
> +	val = priv->output_val[bank];
> +
> +	debug("%s: regs: %lx, value: %x, gpio: %x, bank %x, pin %x, out %x\n",
> +	      __func__, (ulong)platdata->regs, value, offset, bank, pin, val);
>   
>   	if (value) {
> -		val = readl(&platdata->regs->gpiodata + bank * 2);
>   		val = val | (1 << pin);
>   		writel(val, &platdata->regs->gpiodata + bank * 2);
>   	} else {
> -		val = readl(&platdata->regs->gpiodata + bank * 2);
>   		val = val & ~(1 << pin);
>   		writel(val, &platdata->regs->gpiodata + bank * 2);
>   	}

You could replace the two writel function calls by one.

>   
> +	priv->output_val[bank] = val;
> +
>   	return val;
>   };
>   
> @@ -441,6 +449,7 @@ static int xilinx_gpio_get_function(struct udevice *dev, unsigned offset)
>   static int xilinx_gpio_get_value(struct udevice *dev, unsigned offset)
>   {
>   	struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
> +	struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>   	int val, ret;
>   	u32 bank, pin;
>   
> @@ -451,7 +460,14 @@ static int xilinx_gpio_get_value(struct udevice *dev, unsigned offset)
>   	debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n", __func__,
>   	      (ulong)platdata->regs, offset, bank, pin);
>   
> -	val = readl(&platdata->regs->gpiodata + bank * 2);
> +	if (xilinx_gpio_get_function(dev, offset) == GPIOF_INPUT) {
> +		debug("%s: Read input value from reg\n", __func__);
> +		val = readl(&platdata->regs->gpiodata + bank * 2);
> +	} else {
> +		debug("%s: Read saved output value\n", __func__);
> +		val = priv->output_val[bank];
> +	}

Why you don't always read the data register? This doesn't work for three 
state outputs.

> +
>   	val = !!(val & (1 << pin));
>   
>   	return val;
> @@ -558,11 +574,17 @@ static int xilinx_gpio_probe(struct udevice *dev)
>   {
>   	struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
>   	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +	struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>   
>   	uc_priv->bank_name = dev->name;
>   
>   	uc_priv->gpio_count = platdata->bank_max[0] + platdata->bank_max[1];
>   
> +	priv->output_val[0] = platdata->dout_default[0];
> +
> +	if (platdata->bank_max[1])
> +		priv->output_val[1] = platdata->dout_default[1];
> +
>   	return 0;
>   }
>   
> @@ -579,6 +601,9 @@ static int xilinx_gpio_ofdata_to_platdata(struct udevice *dev)
>   						       "xlnx,all-inputs", 0);
>   	platdata->bank_output[0] = dev_read_u32_default(dev,
>   							"xlnx,all-outputs", 0);
> +	platdata->dout_default[0] = dev_read_u32_default(dev,
> +							 "xlnx,dout-default",
> +							 0);
>   
>   	is_dual = dev_read_u32_default(dev, "xlnx,is-dual", 0);
>   	if (is_dual) {
> @@ -588,6 +613,8 @@ static int xilinx_gpio_ofdata_to_platdata(struct udevice *dev)
>   						"xlnx,all-inputs-2", 0);
>   		platdata->bank_output[1] = dev_read_u32_default(dev,
>   						"xlnx,all-outputs-2", 0);
> +		platdata->dout_default[1] = dev_read_u32_default(dev,
> +						"xlnx,dout-default-2", 0);
>   	}
>   
>   	return 0;
> @@ -606,5 +633,6 @@ U_BOOT_DRIVER(xilinx_gpio) = {
>   	.ofdata_to_platdata = xilinx_gpio_ofdata_to_platdata,
>   	.probe = xilinx_gpio_probe,
>   	.platdata_auto_alloc_size = sizeof(struct xilinx_gpio_platdata),
> +	.priv_auto_alloc_size = sizeof(struct xilinx_gpio_privdata),
>   };
>   #endif

Best regards
   Stefan