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

[Samuel Holland <samuel@sholland.org>]

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;