[RFC,04/17] pinctrl: sunxi: add GPIO in/out wrappers

So far we were open-coding the pincontroller's GPIO output/input access
in each function using that.

Provide two functions that wrap that nicely, so users don't need to know
about the internals, and we can abstract the new D1 pinctrl more easily.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/include/asm/arch-sunxi/gpio.h |  2 ++
 arch/arm/mach-sunxi/pinmux.c           | 10 ++++++++++
 drivers/gpio/sunxi_gpio.c              | 26 +++++---------------------
 3 files changed, 17 insertions(+), 21 deletions(-)

On 12/5/22 18:45, Andre Przywara wrote:
> So far we were open-coding the pincontroller's GPIO output/input access
> in each function using that.
> 
> Provide two functions that wrap that nicely, so users don't need to know
> about the internals, and we can abstract the new D1 pinctrl more easily.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm/include/asm/arch-sunxi/gpio.h |  2 ++
>  arch/arm/mach-sunxi/pinmux.c           | 10 ++++++++++
>  drivers/gpio/sunxi_gpio.c              | 26 +++++---------------------
>  3 files changed, 17 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h
> index 8333810a69f..42ca03d8c18 100644
> --- a/arch/arm/include/asm/arch-sunxi/gpio.h
> +++ b/arch/arm/include/asm/arch-sunxi/gpio.h
> @@ -211,6 +211,8 @@ void sunxi_gpio_set_cfgbank(void *bank_base, int pin_offset, u32 val);
>  void sunxi_gpio_set_cfgpin(u32 pin, u32 val);
>  int sunxi_gpio_get_cfgbank(void *bank_base, int pin_offset);
>  int sunxi_gpio_get_cfgpin(u32 pin);
> +void sunxi_gpio_set_output_bank(void *bank_base, u32 clear_mask, u32 set_mask);
> +u32 sunxi_gpio_get_output_bank(void *bank_base);
>  void sunxi_gpio_set_drv(u32 pin, u32 val);
>  void sunxi_gpio_set_drv_bank(void *bank_base, u32 pin_offset, u32 val);
>  void sunxi_gpio_set_pull(u32 pin, u32 val);
> diff --git a/arch/arm/mach-sunxi/pinmux.c b/arch/arm/mach-sunxi/pinmux.c
> index b650f6b1aea..91acbf9269f 100644
> --- a/arch/arm/mach-sunxi/pinmux.c
> +++ b/arch/arm/mach-sunxi/pinmux.c
> @@ -46,6 +46,16 @@ int sunxi_gpio_get_cfgpin(u32 pin)
>  	return sunxi_gpio_get_cfgbank(bank_base, pin % 32);
>  }
>  
> +void sunxi_gpio_set_output_bank(void *bank_base, u32 clear_mask, u32 set_mask)
> +{
> +	clrsetbits_le32(bank_base + GPIO_DAT_REG_OFFSET, clear_mask, set_mask);
> +}
> +
> +u32 sunxi_gpio_get_output_bank(void *bank_base)
> +{
> +	return readl(bank_base + GPIO_DAT_REG_OFFSET);
> +}
> +
>  void sunxi_gpio_set_drv(u32 pin, u32 val)
>  {
>  	u32 bank = GPIO_BANK(pin);
> diff --git a/drivers/gpio/sunxi_gpio.c b/drivers/gpio/sunxi_gpio.c
> index 1bf691a204a..767996c10fc 100644
> --- a/drivers/gpio/sunxi_gpio.c
> +++ b/drivers/gpio/sunxi_gpio.c
> @@ -21,33 +21,22 @@
>  #if !CONFIG_IS_ENABLED(DM_GPIO)
>  static int sunxi_gpio_output(u32 pin, u32 val)
>  {
> -	u32 dat;
>  	u32 bank = GPIO_BANK(pin);
>  	u32 num = GPIO_NUM(pin);
>  	void *pio = BANK_TO_GPIO(bank);
>  
> -	dat = readl(pio + 0x10);
> -	if (val)
> -		dat |= 0x1 << num;
> -	else
> -		dat &= ~(0x1 << num);
> -
> -	writel(dat, pio + 0x10);
> -
> +	sunxi_gpio_set_output_bank(pio, val ? 0 : 1U << num,
> +					val ? 1U << num : 0);
>  	return 0;
>  }
>  
>  static int sunxi_gpio_input(u32 pin)
>  {
> -	u32 dat;
>  	u32 bank = GPIO_BANK(pin);
>  	u32 num = GPIO_NUM(pin);
>  	void *pio = BANK_TO_GPIO(bank);
>  
> -	dat = readl(pio + 0x10);
> -	dat >>= num;
> -
> -	return dat & 0x1;
> +	return (sunxi_gpio_get_output_bank(pio) >> num) & 0x1;
>  }

I would suggest putting this change before patch 3. And I would suggest
following the existing pattern of functions, with an inner one taking
(bank pointer, pin offset, value), and a wrapper calling BANK_TO_GPIO.
This would consolidate the shifting/masking as well.

If you move these two functions to pinmux.c, then all of the
BANK_TO_GPIO callers are in that file, and you can move BANK_TO_GPIO to
pinmux.c as well when you remove the struct and touch all of the call
sites anyway.

Regards,
Samuel

>  
>  int gpio_request(unsigned gpio, const char *label)
> @@ -136,12 +125,8 @@ static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset)
>  {
>  	struct sunxi_gpio_plat *plat = dev_get_plat(dev);
>  	u32 num = GPIO_NUM(offset);
> -	unsigned dat;
> -
> -	dat = readl(plat->regs + GPIO_DAT_REG_OFFSET);
> -	dat >>= num;
>  
> -	return dat & 0x1;
> +	return (sunxi_gpio_get_output_bank(plat->regs) >> num) & 0x1;
>  }
>  
>  static int sunxi_gpio_get_function(struct udevice *dev, unsigned offset)
> @@ -181,8 +166,7 @@ static int sunxi_gpio_set_flags(struct udevice *dev, unsigned int offset,
>  		u32 value = !!(flags & GPIOD_IS_OUT_ACTIVE);
>  		u32 num = GPIO_NUM(offset);
>  
> -		clrsetbits_le32(plat->regs + GPIO_DAT_REG_OFFSET,
> -				1 << num, value << num);
> +		sunxi_gpio_set_output_bank(plat->regs, 1U << num, value << num);
>  		sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT);
>  	} else if (flags & GPIOD_IS_IN) {
>  		u32 pull = 0;