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.przyw...@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;
