[PATCH 0/4] gpio: sunxi: Handle pin configuration flags

[PATCH 0/4] gpio: sunxi: Handle pin configuration flags

[Samuel Holland]

This series updates the sunxi GPIO driver to handle pin pull-up/down,
so consumer drivers do not need to call the non-DM sunxi_gpio_set_*
functions. As an example, the last patch updates the MMC driver to use
this functionality. The helpers added here will also be used for the
upcoming DM_PINCTRL driver.


Samuel Holland (4):
  sunxi: gpio: Return void from setter functions
  sunxi: gpio: Add per-bank drive and pull setters
  gpio: sunxi: Implement .set_flags
  mmc: sunxi: Use DM_GPIO flags to set pull-up

 arch/arm/include/asm/arch-sunxi/gpio.h |  6 ++-
 arch/arm/mach-sunxi/pinmux.c           | 28 +++++++-----
 drivers/gpio/sunxi_gpio.c              | 62 +++++++++++---------------
 drivers/mmc/sunxi_mmc.c                |  8 +---
 4 files changed, 51 insertions(+), 53 deletions(-)

-- 
2.32.0

[PATCH 1/4] sunxi: gpio: Return void from setter functions

[Samuel Holland]

The return values of these functions are always zero, and they are
never checked. Since they are not needed, remove them.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 arch/arm/include/asm/arch-sunxi/gpio.h | 4 ++--
 arch/arm/mach-sunxi/pinmux.c           | 8 ++------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h
index f3ab1aea0e..2b72b2263b 100644
--- a/arch/arm/include/asm/arch-sunxi/gpio.h
+++ b/arch/arm/include/asm/arch-sunxi/gpio.h
@@ -226,8 +226,8 @@ void sunxi_gpio_set_cfgbank(struct sunxi_gpio *pio, int bank_offset, u32 val);
 void sunxi_gpio_set_cfgpin(u32 pin, u32 val);
 int sunxi_gpio_get_cfgbank(struct sunxi_gpio *pio, int bank_offset);
 int sunxi_gpio_get_cfgpin(u32 pin);
-int sunxi_gpio_set_drv(u32 pin, u32 val);
-int sunxi_gpio_set_pull(u32 pin, u32 val);
+void sunxi_gpio_set_drv(u32 pin, u32 val);
+void sunxi_gpio_set_pull(u32 pin, u32 val);
 int sunxi_name_to_gpio(const char *name);
 
 #if !defined CONFIG_SPL_BUILD && defined CONFIG_AXP_GPIO
diff --git a/arch/arm/mach-sunxi/pinmux.c b/arch/arm/mach-sunxi/pinmux.c
index 642483f06c..cf9d9daf7c 100644
--- a/arch/arm/mach-sunxi/pinmux.c
+++ b/arch/arm/mach-sunxi/pinmux.c
@@ -45,7 +45,7 @@ int sunxi_gpio_get_cfgpin(u32 pin)
 	return sunxi_gpio_get_cfgbank(pio, pin);
 }
 
-int sunxi_gpio_set_drv(u32 pin, u32 val)
+void sunxi_gpio_set_drv(u32 pin, u32 val)
 {
 	u32 bank = GPIO_BANK(pin);
 	u32 index = GPIO_DRV_INDEX(pin);
@@ -53,11 +53,9 @@ int sunxi_gpio_set_drv(u32 pin, u32 val)
 	struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
 
 	clrsetbits_le32(&pio->drv[0] + index, 0x3 << offset, val << offset);
-
-	return 0;
 }
 
-int sunxi_gpio_set_pull(u32 pin, u32 val)
+void sunxi_gpio_set_pull(u32 pin, u32 val)
 {
 	u32 bank = GPIO_BANK(pin);
 	u32 index = GPIO_PULL_INDEX(pin);
@@ -65,6 +63,4 @@ int sunxi_gpio_set_pull(u32 pin, u32 val)
 	struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
 
 	clrsetbits_le32(&pio->pull[0] + index, 0x3 << offset, val << offset);
-
-	return 0;
 }
-- 
2.32.0

[PATCH 2/4] sunxi: gpio: Add per-bank drive and pull setters

[Samuel Holland]

The GPIO and pinctrl drivers need these setters for pin configuration.
Since they are DM drivers, they should not be using hardcoded base
addresses. Factor out variants of the setter functions which take a
pointer to the GPIO bank's MMIO registers.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 arch/arm/include/asm/arch-sunxi/gpio.h |  2 ++
 arch/arm/mach-sunxi/pinmux.c           | 20 ++++++++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h
index 2b72b2263b..106605adf5 100644
--- a/arch/arm/include/asm/arch-sunxi/gpio.h
+++ b/arch/arm/include/asm/arch-sunxi/gpio.h
@@ -227,7 +227,9 @@ void sunxi_gpio_set_cfgpin(u32 pin, u32 val);
 int sunxi_gpio_get_cfgbank(struct sunxi_gpio *pio, int bank_offset);
 int sunxi_gpio_get_cfgpin(u32 pin);
 void sunxi_gpio_set_drv(u32 pin, u32 val);
+void sunxi_gpio_set_drv_bank(struct sunxi_gpio *pio, u32 bank_offset, u32 val);
 void sunxi_gpio_set_pull(u32 pin, u32 val);
+void sunxi_gpio_set_pull_bank(struct sunxi_gpio *pio, int bank_offset, u32 val);
 int sunxi_name_to_gpio(const char *name);
 
 #if !defined CONFIG_SPL_BUILD && defined CONFIG_AXP_GPIO
diff --git a/arch/arm/mach-sunxi/pinmux.c b/arch/arm/mach-sunxi/pinmux.c
index cf9d9daf7c..b2093b623a 100644
--- a/arch/arm/mach-sunxi/pinmux.c
+++ b/arch/arm/mach-sunxi/pinmux.c
@@ -48,19 +48,31 @@ int sunxi_gpio_get_cfgpin(u32 pin)
 void sunxi_gpio_set_drv(u32 pin, u32 val)
 {
 	u32 bank = GPIO_BANK(pin);
-	u32 index = GPIO_DRV_INDEX(pin);
-	u32 offset = GPIO_DRV_OFFSET(pin);
 	struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
 
+	sunxi_gpio_set_drv_bank(pio, pin, val);
+}
+
+void sunxi_gpio_set_drv_bank(struct sunxi_gpio *pio, u32 bank_offset, u32 val)
+{
+	u32 index = GPIO_DRV_INDEX(bank_offset);
+	u32 offset = GPIO_DRV_OFFSET(bank_offset);
+
 	clrsetbits_le32(&pio->drv[0] + index, 0x3 << offset, val << offset);
 }
 
 void sunxi_gpio_set_pull(u32 pin, u32 val)
 {
 	u32 bank = GPIO_BANK(pin);
-	u32 index = GPIO_PULL_INDEX(pin);
-	u32 offset = GPIO_PULL_OFFSET(pin);
 	struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
 
+	sunxi_gpio_set_pull_bank(pio, pin, val);
+}
+
+void sunxi_gpio_set_pull_bank(struct sunxi_gpio *pio, int bank_offset, u32 val)
+{
+	u32 index = GPIO_PULL_INDEX(bank_offset);
+	u32 offset = GPIO_PULL_OFFSET(bank_offset);
+
 	clrsetbits_le32(&pio->pull[0] + index, 0x3 << offset, val << offset);
 }
-- 
2.32.0

[PATCH 3/4] gpio: sunxi: Implement .set_flags

[Samuel Holland]

This, along with gpio_flags_xlate(), allows the GPIO driver to handle
pull-up/down flags provided by consumer drivers or in the device tree.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/gpio/sunxi_gpio.c | 62 +++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 35 deletions(-)

diff --git a/drivers/gpio/sunxi_gpio.c b/drivers/gpio/sunxi_gpio.c
index cdbc40d48f..92fee0118d 100644
--- a/drivers/gpio/sunxi_gpio.c
+++ b/drivers/gpio/sunxi_gpio.c
@@ -139,27 +139,6 @@ int sunxi_name_to_gpio(const char *name)
 	return ret ? ret : gpio;
 }
 
-static int sunxi_gpio_direction_input(struct udevice *dev, unsigned offset)
-{
-	struct sunxi_gpio_plat *plat = dev_get_plat(dev);
-
-	sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT);
-
-	return 0;
-}
-
-static int sunxi_gpio_direction_output(struct udevice *dev, unsigned offset,
-				       int value)
-{
-	struct sunxi_gpio_plat *plat = dev_get_plat(dev);
-	u32 num = GPIO_NUM(offset);
-
-	sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT);
-	clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0);
-
-	return 0;
-}
-
 static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset)
 {
 	struct sunxi_gpio_plat *plat = dev_get_plat(dev);
@@ -172,16 +151,6 @@ static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset)
 	return dat & 0x1;
 }
 
-static int sunxi_gpio_set_value(struct udevice *dev, unsigned offset,
-				int value)
-{
-	struct sunxi_gpio_plat *plat = dev_get_plat(dev);
-	u32 num = GPIO_NUM(offset);
-
-	clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0);
-	return 0;
-}
-
 static int sunxi_gpio_get_function(struct udevice *dev, unsigned offset)
 {
 	struct sunxi_gpio_plat *plat = dev_get_plat(dev);
@@ -205,18 +174,41 @@ static int sunxi_gpio_xlate(struct udevice *dev, struct gpio_desc *desc,
 	if (ret)
 		return ret;
 	desc->offset = args->args[1];
-	desc->flags = args->args[2] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;
+	desc->flags = gpio_flags_xlate(args->args[2]);
+
+	return 0;
+}
+
+static int sunxi_gpio_set_flags(struct udevice *dev, unsigned int offset,
+				ulong flags)
+{
+	struct sunxi_gpio_plat *plat = dev_get_plat(dev);
+
+	if (flags & GPIOD_IS_OUT) {
+		u32 value = !!(flags & GPIOD_IS_OUT_ACTIVE);
+		u32 num = GPIO_NUM(offset);
+
+		clrsetbits_le32(&plat->regs->dat, 1 << num, value << num);
+		sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT);
+	} else if (flags & GPIOD_IS_IN) {
+		u32 pull = 0;
+
+		if (flags & GPIOD_PULL_UP)
+			pull = 1;
+		else if (flags & GPIOD_PULL_DOWN)
+			pull = 2;
+		sunxi_gpio_set_pull_bank(plat->regs, offset, pull);
+		sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT);
+	}
 
 	return 0;
 }
 
 static const struct dm_gpio_ops gpio_sunxi_ops = {
-	.direction_input	= sunxi_gpio_direction_input,
-	.direction_output	= sunxi_gpio_direction_output,
 	.get_value		= sunxi_gpio_get_value,
-	.set_value		= sunxi_gpio_set_value,
 	.get_function		= sunxi_gpio_get_function,
 	.xlate			= sunxi_gpio_xlate,
+	.set_flags		= sunxi_gpio_set_flags,
 };
 
 /**
-- 
2.32.0

[PATCH 4/4] mmc: sunxi: Use DM_GPIO flags to set pull-up

[Samuel Holland]

Now that the sunxi_gpio driver handles pull-up/down via the driver
model, pin configuration does not need a platform-specific function.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/mmc/sunxi_mmc.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
index c170c16d5a..955b29826f 100644
--- a/drivers/mmc/sunxi_mmc.c
+++ b/drivers/mmc/sunxi_mmc.c
@@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev)
 		return ret;
 
 	/* This GPIO is optional */
-	if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
-				  GPIOD_IS_IN)) {
-		int cd_pin = gpio_get_number(&priv->cd_gpio);
-
-		sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP);
-	}
+	gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
+			     GPIOD_IS_IN | GPIOD_PULL_UP);
 
 	upriv->mmc = &plat->mmc;
 
-- 
2.32.0

Re: [PATCH 4/4] mmc: sunxi: Use DM_GPIO flags to set pull-up

[Jaehoon Chung]

Hi,

On 10/21/21 1:52 PM, Samuel Holland wrote:
> Now that the sunxi_gpio driver handles pull-up/down via the driver
> model, pin configuration does not need a platform-specific function.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/mmc/sunxi_mmc.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
> index c170c16d5a..955b29826f 100644
> --- a/drivers/mmc/sunxi_mmc.c
> +++ b/drivers/mmc/sunxi_mmc.c
> @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev)
>  		return ret;
>  
>  	/* This GPIO is optional */
> -	if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
> -				  GPIOD_IS_IN)) {
> -		int cd_pin = gpio_get_number(&priv->cd_gpio);
> -
> -		sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP);
> -	}
> +	gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
> +			     GPIOD_IS_IN | GPIOD_PULL_UP);

Is it right to set the pull-up?

Best Regards,
Jaehoon Chung

>  
>  	upriv->mmc = &plat->mmc;
>  
> 

Re: [PATCH 4/4] mmc: sunxi: Use DM_GPIO flags to set pull-up

[Andre Przywara]

On Fri, 22 Oct 2021 06:58:48 +0900
Jaehoon Chung <jh80.chung@samsung.com> wrote:

Hi Jaehoon,

thanks for having a look!

> Hi,
> 
> On 10/21/21 1:52 PM, Samuel Holland wrote:
> > Now that the sunxi_gpio driver handles pull-up/down via the driver
> > model, pin configuration does not need a platform-specific function.
> > 
> > Signed-off-by: Samuel Holland <samuel@sholland.org>
> > ---
> > 
> >  drivers/mmc/sunxi_mmc.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
> > index c170c16d5a..955b29826f 100644
> > --- a/drivers/mmc/sunxi_mmc.c
> > +++ b/drivers/mmc/sunxi_mmc.c
> > @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev)
> >  		return ret;
> >  
> >  	/* This GPIO is optional */
> > -	if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
> > -				  GPIOD_IS_IN)) {
> > -		int cd_pin = gpio_get_number(&priv->cd_gpio);
> > -
> > -		sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP);
> > -	}
> > +	gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
> > +			     GPIOD_IS_IN | GPIOD_PULL_UP);  
> 
> Is it right to set the pull-up?

You mean, unconditionally? I mean it's just copying the current
code, which does that (see the "minus" lines just above).

But I think you have a point: I don't see any pull up specified in any DT,
and I think most (if not all) boards have a discrete pull up resistor on
that line.

But I don't dare to touch that code - at least for this series, as it
works (TM) right now.
After the full DM_PINCTRL series this might be another story, though.

Cheers,
Andre

> 
> Best Regards,
> Jaehoon Chung
> 
> >  
> >  	upriv->mmc = &plat->mmc;
> >  
> >   
> 

Re: [PATCH 4/4] mmc: sunxi: Use DM_GPIO flags to set pull-up

[Jaehoon Chung]

Hi Andre,

On 10/22/21 6:00 PM, Andre Przywara wrote:
> On Fri, 22 Oct 2021 06:58:48 +0900
> Jaehoon Chung <jh80.chung@samsung.com> wrote:
> 
> Hi Jaehoon,
> 
> thanks for having a look!
> 
>> Hi,
>>
>> On 10/21/21 1:52 PM, Samuel Holland wrote:
>>> Now that the sunxi_gpio driver handles pull-up/down via the driver
>>> model, pin configuration does not need a platform-specific function.
>>>
>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>>> ---
>>>
>>>  drivers/mmc/sunxi_mmc.c | 8 ++------
>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
>>> index c170c16d5a..955b29826f 100644
>>> --- a/drivers/mmc/sunxi_mmc.c
>>> +++ b/drivers/mmc/sunxi_mmc.c
>>> @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev)
>>>  		return ret;
>>>  
>>>  	/* This GPIO is optional */
>>> -	if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
>>> -				  GPIOD_IS_IN)) {
>>> -		int cd_pin = gpio_get_number(&priv->cd_gpio);
>>> -
>>> -		sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP);
>>> -	}
>>> +	gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
>>> +			     GPIOD_IS_IN | GPIOD_PULL_UP);  
>>
>> Is it right to set the pull-up?
> 
> You mean, unconditionally? I mean it's just copying the current
> code, which does that (see the "minus" lines just above).

Yes, it looks like something strange. 
AFAIK, It can be changed that cd-gpios has dependent how to consist of H/W.
But it's not different with original behavior, as you mentioned.

> 
> But I think you have a point: I don't see any pull up specified in any DT,
> and I think most (if not all) boards have a discrete pull up resistor on
> that line.
> 
> But I don't dare to touch that code - at least for this series, as it
> works (TM) right now.
> After the full DM_PINCTRL series this might be another story, though.

Right. I understood exactly. Thanks for explanation.

P.S: Can I test sunxi patch with NeoPlus2 board(Allwinner H5)?

Best Regards,
Jaehoon Chung

> 
> Cheers,
> Andre
> 
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>  
>>>  	upriv->mmc = &plat->mmc;
>>>  
>>>   
>>
> 
> 

Re: [PATCH 4/4] mmc: sunxi: Use DM_GPIO flags to set pull-up

[Andre Przywara]

On Fri, 22 Oct 2021 19:10:20 +0900
Jaehoon Chung <jh80.chung@samsung.com> wrote:

> Hi Andre,
> 
> On 10/22/21 6:00 PM, Andre Przywara wrote:
> > On Fri, 22 Oct 2021 06:58:48 +0900
> > Jaehoon Chung <jh80.chung@samsung.com> wrote:
> > 
> > Hi Jaehoon,
> > 
> > thanks for having a look!
> >   
> >> Hi,
> >>
> >> On 10/21/21 1:52 PM, Samuel Holland wrote:  
> >>> Now that the sunxi_gpio driver handles pull-up/down via the driver
> >>> model, pin configuration does not need a platform-specific function.
> >>>
> >>> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >>> ---
> >>>
> >>>  drivers/mmc/sunxi_mmc.c | 8 ++------
> >>>  1 file changed, 2 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
> >>> index c170c16d5a..955b29826f 100644
> >>> --- a/drivers/mmc/sunxi_mmc.c
> >>> +++ b/drivers/mmc/sunxi_mmc.c
> >>> @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev)
> >>>  		return ret;
> >>>  
> >>>  	/* This GPIO is optional */
> >>> -	if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
> >>> -				  GPIOD_IS_IN)) {
> >>> -		int cd_pin = gpio_get_number(&priv->cd_gpio);
> >>> -
> >>> -		sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP);
> >>> -	}
> >>> +	gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
> >>> +			     GPIOD_IS_IN | GPIOD_PULL_UP);    
> >>
> >> Is it right to set the pull-up?  
> > 
> > You mean, unconditionally? I mean it's just copying the current
> > code, which does that (see the "minus" lines just above).  
> 
> Yes, it looks like something strange. 
> AFAIK, It can be changed that cd-gpios has dependent how to consist of H/W.
> But it's not different with original behavior, as you mentioned.
> 
> > 
> > But I think you have a point: I don't see any pull up specified in any DT,
> > and I think most (if not all) boards have a discrete pull up resistor on
> > that line.
> > 
> > But I don't dare to touch that code - at least for this series, as it
> > works (TM) right now.
> > After the full DM_PINCTRL series this might be another story, though.  
> 
> Right. I understood exactly. Thanks for explanation.
> 
> P.S: Can I test sunxi patch with NeoPlus2 board(Allwinner H5)?

Yes, please, any testing on any kind of Allwinner board is warmly
welcomed. There are 160 supported Allwinner boards in the tree, and even
though I have probably more of them than I want, it's only a fraction of
them. And experience show that some boards are subtly broken.
Also feel free to report any other broken things you see when testing!

Thanks,
Andre

> 
> Best Regards,
> Jaehoon Chung
> 
> > 
> > Cheers,
> > Andre
> >   
> >>
> >> Best Regards,
> >> Jaehoon Chung
> >>  
> >>>  
> >>>  	upriv->mmc = &plat->mmc;
> >>>  
> >>>     
> >>  
> > 
> >   
> 

Re: [PATCH 3/4] gpio: sunxi: Implement .set_flags

[Simon Glass]

On Wed, 20 Oct 2021 at 22:53, Samuel Holland <samuel@sholland.org> wrote:
>
> This, along with gpio_flags_xlate(), allows the GPIO driver to handle
> pull-up/down flags provided by consumer drivers or in the device tree.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>
>  drivers/gpio/sunxi_gpio.c | 62 +++++++++++++++++----------------------
>  1 file changed, 27 insertions(+), 35 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 1/4] sunxi: gpio: Return void from setter functions

[Heinrich Schuchardt]

On 10/21/21 06:52, Samuel Holland wrote:
> The return values of these functions are always zero, and they are
> never checked. Since they are not needed, remove them.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Re: [PATCH 2/4] sunxi: gpio: Add per-bank drive and pull setters

[Heinrich Schuchardt]

On 10/21/21 06:52, Samuel Holland wrote:
> The GPIO and pinctrl drivers need these setters for pin configuration.
> Since they are DM drivers, they should not be using hardcoded base
> addresses. Factor out variants of the setter functions which take a
> pointer to the GPIO bank's MMIO registers.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>   arch/arm/include/asm/arch-sunxi/gpio.h |  2 ++
>   arch/arm/mach-sunxi/pinmux.c           | 20 ++++++++++++++++----
>   2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h
> index 2b72b2263b..106605adf5 100644
> --- a/arch/arm/include/asm/arch-sunxi/gpio.h
> +++ b/arch/arm/include/asm/arch-sunxi/gpio.h
> @@ -227,7 +227,9 @@ void sunxi_gpio_set_cfgpin(u32 pin, u32 val);
>   int sunxi_gpio_get_cfgbank(struct sunxi_gpio *pio, int bank_offset);
>   int sunxi_gpio_get_cfgpin(u32 pin);
>   void sunxi_gpio_set_drv(u32 pin, u32 val);

Please, add Sphinx style documentation for the new functions, preferably 
in the header file. Cf.
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

> +void sunxi_gpio_set_drv_bank(struct sunxi_gpio *pio, u32 bank_offset, u32 val);
>   void sunxi_gpio_set_pull(u32 pin, u32 val);
> +void sunxi_gpio_set_pull_bank(struct sunxi_gpio *pio, int bank_offset, u32 val);
>   int sunxi_name_to_gpio(const char *name);
>   
>   #if !defined CONFIG_SPL_BUILD && defined CONFIG_AXP_GPIO
> diff --git a/arch/arm/mach-sunxi/pinmux.c b/arch/arm/mach-sunxi/pinmux.c
> index cf9d9daf7c..b2093b623a 100644
> --- a/arch/arm/mach-sunxi/pinmux.c
> +++ b/arch/arm/mach-sunxi/pinmux.c
> @@ -48,19 +48,31 @@ int sunxi_gpio_get_cfgpin(u32 pin)
>   void sunxi_gpio_set_drv(u32 pin, u32 val)
>   {
>   	u32 bank = GPIO_BANK(pin);
> -	u32 index = GPIO_DRV_INDEX(pin);
> -	u32 offset = GPIO_DRV_OFFSET(pin);
>   	struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
>   
> +	sunxi_gpio_set_drv_bank(pio, pin, val);
> +}
> +
> +void sunxi_gpio_set_drv_bank(struct sunxi_gpio *pio, u32 bank_offset, u32 val)
> +{
> +	u32 index = GPIO_DRV_INDEX(bank_offset);
> +	u32 offset = GPIO_DRV_OFFSET(bank_offset);
> +
>   	clrsetbits_le32(&pio->drv[0] + index, 0x3 << offset, val << offset);
>   }
>   
>   void sunxi_gpio_set_pull(u32 pin, u32 val)
>   {
>   	u32 bank = GPIO_BANK(pin);
> -	u32 index = GPIO_PULL_INDEX(pin);
> -	u32 offset = GPIO_PULL_OFFSET(pin);
>   	struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
>   
> +	sunxi_gpio_set_pull_bank(pio, pin, val);
> +}
> +
> +void sunxi_gpio_set_pull_bank(struct sunxi_gpio *pio, int bank_offset, u32 val)
> +{
> +	u32 index = GPIO_PULL_INDEX(bank_offset);
> +	u32 offset = GPIO_PULL_OFFSET(bank_offset);
> +
>   	clrsetbits_le32(&pio->pull[0] + index, 0x3 << offset, val << offset);

Please, simplify this:
%s/&pio->pull[0] + index/&pio->pull[index]/

Otherwise the change looks correct to me.

Best regards

Heinrich

>   }
> 

Re: [PATCH 3/4] gpio: sunxi: Implement .set_flags

[Heinrich Schuchardt]

On 10/21/21 06:52, Samuel Holland wrote:
> This, along with gpio_flags_xlate(), allows the GPIO driver to handle
> pull-up/down flags provided by consumer drivers or in the device tree.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> 
>   drivers/gpio/sunxi_gpio.c | 62 +++++++++++++++++----------------------
>   1 file changed, 27 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpio/sunxi_gpio.c b/drivers/gpio/sunxi_gpio.c
> index cdbc40d48f..92fee0118d 100644
> --- a/drivers/gpio/sunxi_gpio.c
> +++ b/drivers/gpio/sunxi_gpio.c
> @@ -139,27 +139,6 @@ int sunxi_name_to_gpio(const char *name)
>   	return ret ? ret : gpio;
>   }
>   
> -static int sunxi_gpio_direction_input(struct udevice *dev, unsigned offset)
> -{
> -	struct sunxi_gpio_plat *plat = dev_get_plat(dev);
> -
> -	sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT);
> -
> -	return 0;
> -}
> -
> -static int sunxi_gpio_direction_output(struct udevice *dev, unsigned offset,
> -				       int value)
> -{
> -	struct sunxi_gpio_plat *plat = dev_get_plat(dev);
> -	u32 num = GPIO_NUM(offset);
> -
> -	sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT);
> -	clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0);
> -
> -	return 0;
> -}
> -
>   static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset)
>   {
>   	struct sunxi_gpio_plat *plat = dev_get_plat(dev);
> @@ -172,16 +151,6 @@ static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset)
>   	return dat & 0x1;
>   }
>   
> -static int sunxi_gpio_set_value(struct udevice *dev, unsigned offset,
> -				int value)
> -{
> -	struct sunxi_gpio_plat *plat = dev_get_plat(dev);
> -	u32 num = GPIO_NUM(offset);
> -
> -	clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0);
> -	return 0;
> -}
> -
>   static int sunxi_gpio_get_function(struct udevice *dev, unsigned offset)
>   {
>   	struct sunxi_gpio_plat *plat = dev_get_plat(dev);
> @@ -205,18 +174,41 @@ static int sunxi_gpio_xlate(struct udevice *dev, struct gpio_desc *desc,
>   	if (ret)
>   		return ret;
>   	desc->offset = args->args[1];
> -	desc->flags = args->args[2] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;
> +	desc->flags = gpio_flags_xlate(args->args[2]);
> +
> +	return 0;
> +}
> +
> +static int sunxi_gpio_set_flags(struct udevice *dev, unsigned int offset,
> +				ulong flags)
> +{
> +	struct sunxi_gpio_plat *plat = dev_get_plat(dev);
> +
> +	if (flags & GPIOD_IS_OUT) {
> +		u32 value = !!(flags & GPIOD_IS_OUT_ACTIVE);
> +		u32 num = GPIO_NUM(offset);
> +
> +		clrsetbits_le32(&plat->regs->dat, 1 << num, value << num);
> +		sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT);
> +	} else if (flags & GPIOD_IS_IN) {
> +		u32 pull = 0;
> +
> +		if (flags & GPIOD_PULL_UP)
> +			pull = 1;
> +		else if (flags & GPIOD_PULL_DOWN)
> +			pull = 2;
> +		sunxi_gpio_set_pull_bank(plat->regs, offset, pull);
> +		sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT);
> +	}
>   
>   	return 0;
>   }
>   
>   static const struct dm_gpio_ops gpio_sunxi_ops = {
> -	.direction_input	= sunxi_gpio_direction_input,
> -	.direction_output	= sunxi_gpio_direction_output,

Removing these operations is not related to your commit message.

dm_set_gpio_value() seems to be using them.

>   	.get_value		= sunxi_gpio_get_value,
> -	.set_value		= sunxi_gpio_set_value,

Same here.

Best regards

Heinrich

>   	.get_function		= sunxi_gpio_get_function,
>   	.xlate			= sunxi_gpio_xlate,
> +	.set_flags		= sunxi_gpio_set_flags,
>   };
>   
>   /**
> 

Re: [PATCH 4/4] mmc: sunxi: Use DM_GPIO flags to set pull-up

[Heinrich Schuchardt]

On 10/21/21 06:52, Samuel Holland wrote:
> Now that the sunxi_gpio driver handles pull-up/down via the driver
> model, pin configuration does not need a platform-specific function.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

I tested on an OrangePi PC (orangepi_pc_defconfig). The 'mmc rescan' 
command detects correctly if an SD-card is present with this series applied.

Tested-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

> ---
> 
>   drivers/mmc/sunxi_mmc.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
> index c170c16d5a..955b29826f 100644
> --- a/drivers/mmc/sunxi_mmc.c
> +++ b/drivers/mmc/sunxi_mmc.c
> @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev)
>   		return ret;
>   
>   	/* This GPIO is optional */
> -	if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
> -				  GPIOD_IS_IN)) {
> -		int cd_pin = gpio_get_number(&priv->cd_gpio);
> -
> -		sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP);
> -	}
> +	gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
> +			     GPIOD_IS_IN | GPIOD_PULL_UP);
>   
>   	upriv->mmc = &plat->mmc;
>   
> 

Re: [PATCH 3/4] gpio: sunxi: Implement .set_flags

[Samuel Holland]

On 11/5/21 9:43 AM, Heinrich Schuchardt wrote:
> On 10/21/21 06:52, Samuel Holland wrote:
>> This, along with gpio_flags_xlate(), allows the GPIO driver to handle
>> pull-up/down flags provided by consumer drivers or in the device tree.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>   drivers/gpio/sunxi_gpio.c | 62 +++++++++++++++++----------------------
>>   1 file changed, 27 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpio/sunxi_gpio.c b/drivers/gpio/sunxi_gpio.c
>> index cdbc40d48f..92fee0118d 100644
>> --- a/drivers/gpio/sunxi_gpio.c
>> +++ b/drivers/gpio/sunxi_gpio.c
>> @@ -139,27 +139,6 @@ int sunxi_name_to_gpio(const char *name)
>>       return ret ? ret : gpio;
>>   }
>>   -static int sunxi_gpio_direction_input(struct udevice *dev, unsigned
>> offset)
>> -{
>> -    struct sunxi_gpio_plat *plat = dev_get_plat(dev);
>> -
>> -    sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT);
>> -
>> -    return 0;
>> -}
>> -
>> -static int sunxi_gpio_direction_output(struct udevice *dev, unsigned
>> offset,
>> -                       int value)
>> -{
>> -    struct sunxi_gpio_plat *plat = dev_get_plat(dev);
>> -    u32 num = GPIO_NUM(offset);
>> -
>> -    sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT);
>> -    clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0);
>> -
>> -    return 0;
>> -}
>> -
>>   static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset)
>>   {
>>       struct sunxi_gpio_plat *plat = dev_get_plat(dev);
>> @@ -172,16 +151,6 @@ static int sunxi_gpio_get_value(struct udevice
>> *dev, unsigned offset)
>>       return dat & 0x1;
>>   }
>>   -static int sunxi_gpio_set_value(struct udevice *dev, unsigned offset,
>> -                int value)
>> -{
>> -    struct sunxi_gpio_plat *plat = dev_get_plat(dev);
>> -    u32 num = GPIO_NUM(offset);
>> -
>> -    clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0);
>> -    return 0;
>> -}
>> -
>>   static int sunxi_gpio_get_function(struct udevice *dev, unsigned
>> offset)
>>   {
>>       struct sunxi_gpio_plat *plat = dev_get_plat(dev);
>> @@ -205,18 +174,41 @@ static int sunxi_gpio_xlate(struct udevice *dev,
>> struct gpio_desc *desc,
>>       if (ret)
>>           return ret;
>>       desc->offset = args->args[1];
>> -    desc->flags = args->args[2] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW
>> : 0;
>> +    desc->flags = gpio_flags_xlate(args->args[2]);
>> +
>> +    return 0;
>> +}
>> +
>> +static int sunxi_gpio_set_flags(struct udevice *dev, unsigned int
>> offset,
>> +                ulong flags)
>> +{
>> +    struct sunxi_gpio_plat *plat = dev_get_plat(dev);
>> +
>> +    if (flags & GPIOD_IS_OUT) {
>> +        u32 value = !!(flags & GPIOD_IS_OUT_ACTIVE);
>> +        u32 num = GPIO_NUM(offset);
>> +
>> +        clrsetbits_le32(&plat->regs->dat, 1 << num, value << num);
>> +        sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT);
>> +    } else if (flags & GPIOD_IS_IN) {
>> +        u32 pull = 0;
>> +
>> +        if (flags & GPIOD_PULL_UP)
>> +            pull = 1;
>> +        else if (flags & GPIOD_PULL_DOWN)
>> +            pull = 2;
>> +        sunxi_gpio_set_pull_bank(plat->regs, offset, pull);
>> +        sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT);
>> +    }
>>         return 0;
>>   }
>>     static const struct dm_gpio_ops gpio_sunxi_ops = {
>> -    .direction_input    = sunxi_gpio_direction_input,
>> -    .direction_output    = sunxi_gpio_direction_output,
> 
> Removing these operations is not related to your commit message.
> 
> dm_gpio_set_value() seems to be using them.

It does not use any of these operations when set_flags is provided. The
same applies to _dm_gpio_set_flags().

asm-generic/gpio.h says about set_flags:
> This method is required and should be implemented by new drivers. At
> some point, it will supersede direction_input() and
> direction_output(), which wil be removed.

So I believe it is intended to replace the other three operations.

Regards,
Samuel

>>       .get_value        = sunxi_gpio_get_value,
>> -    .set_value        = sunxi_gpio_set_value,
> 
> Same here.
> 
> Best regards
> 
> Heinrich
> 
>>       .get_function        = sunxi_gpio_get_function,
>>       .xlate            = sunxi_gpio_xlate,
>> +    .set_flags        = sunxi_gpio_set_flags,
>>   };
>>     /**
>>
> 

Re: [PATCH 2/4] sunxi: gpio: Add per-bank drive and pull setters

[Andre Przywara]

On Fri, 5 Nov 2021 15:12:16 +0100
Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:

> On 10/21/21 06:52, Samuel Holland wrote:
> > The GPIO and pinctrl drivers need these setters for pin configuration.
> > Since they are DM drivers, they should not be using hardcoded base
> > addresses. Factor out variants of the setter functions which take a
> > pointer to the GPIO bank's MMIO registers.
> > 
> > Signed-off-by: Samuel Holland <samuel@sholland.org>
> > ---
> > 
> >   arch/arm/include/asm/arch-sunxi/gpio.h |  2 ++
> >   arch/arm/mach-sunxi/pinmux.c           | 20 ++++++++++++++++----
> >   2 files changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h
> > index 2b72b2263b..106605adf5 100644
> > --- a/arch/arm/include/asm/arch-sunxi/gpio.h
> > +++ b/arch/arm/include/asm/arch-sunxi/gpio.h
> > @@ -227,7 +227,9 @@ void sunxi_gpio_set_cfgpin(u32 pin, u32 val);
> >   int sunxi_gpio_get_cfgbank(struct sunxi_gpio *pio, int bank_offset);
> >   int sunxi_gpio_get_cfgpin(u32 pin);
> >   void sunxi_gpio_set_drv(u32 pin, u32 val);  
> 
> Please, add Sphinx style documentation for the new functions, preferably 
> in the header file. Cf.
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

I don't think this is necessary or useful, really. Those function are
not really an advertised interface, more something we use internally,
mostly for our SPL.

> > +void sunxi_gpio_set_drv_bank(struct sunxi_gpio *pio, u32 bank_offset, u32 val);
> >   void sunxi_gpio_set_pull(u32 pin, u32 val);
> > +void sunxi_gpio_set_pull_bank(struct sunxi_gpio *pio, int bank_offset, u32 val);
> >   int sunxi_name_to_gpio(const char *name);
> >   
> >   #if !defined CONFIG_SPL_BUILD && defined CONFIG_AXP_GPIO
> > diff --git a/arch/arm/mach-sunxi/pinmux.c b/arch/arm/mach-sunxi/pinmux.c
> > index cf9d9daf7c..b2093b623a 100644
> > --- a/arch/arm/mach-sunxi/pinmux.c
> > +++ b/arch/arm/mach-sunxi/pinmux.c
> > @@ -48,19 +48,31 @@ int sunxi_gpio_get_cfgpin(u32 pin)
> >   void sunxi_gpio_set_drv(u32 pin, u32 val)
> >   {
> >   	u32 bank = GPIO_BANK(pin);
> > -	u32 index = GPIO_DRV_INDEX(pin);
> > -	u32 offset = GPIO_DRV_OFFSET(pin);
> >   	struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
> >   
> > +	sunxi_gpio_set_drv_bank(pio, pin, val);
> > +}
> > +
> > +void sunxi_gpio_set_drv_bank(struct sunxi_gpio *pio, u32 bank_offset, u32 val)
> > +{
> > +	u32 index = GPIO_DRV_INDEX(bank_offset);
> > +	u32 offset = GPIO_DRV_OFFSET(bank_offset);
> > +
> >   	clrsetbits_le32(&pio->drv[0] + index, 0x3 << offset, val << offset);
> >   }
> >   
> >   void sunxi_gpio_set_pull(u32 pin, u32 val)
> >   {
> >   	u32 bank = GPIO_BANK(pin);
> > -	u32 index = GPIO_PULL_INDEX(pin);
> > -	u32 offset = GPIO_PULL_OFFSET(pin);
> >   	struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
> >   
> > +	sunxi_gpio_set_pull_bank(pio, pin, val);
> > +}
> > +
> > +void sunxi_gpio_set_pull_bank(struct sunxi_gpio *pio, int bank_offset, u32 val)
> > +{
> > +	u32 index = GPIO_PULL_INDEX(bank_offset);
> > +	u32 offset = GPIO_PULL_OFFSET(bank_offset);
> > +
> >   	clrsetbits_le32(&pio->pull[0] + index, 0x3 << offset, val << offset);  
> 
> Please, simplify this:
> %s/&pio->pull[0] + index/&pio->pull[index]/

Fixing this up locally.

Cheers,
Andre

> 
> Otherwise the change looks correct to me.
> 
> Best regards
> 
> Heinrich
> 
> >   }
> >   
> 

Re: [PATCH 3/4] gpio: sunxi: Implement .set_flags

[Andre Przywara]

On Fri, 5 Nov 2021 16:46:07 -0500
Samuel Holland <samuel@sholland.org> wrote:

> On 11/5/21 9:43 AM, Heinrich Schuchardt wrote:
> > On 10/21/21 06:52, Samuel Holland wrote:  
> >> This, along with gpio_flags_xlate(), allows the GPIO driver to handle
> >> pull-up/down flags provided by consumer drivers or in the device tree.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> Reviewed-by: Simon Glass <sjg@chromium.org>
> >> ---
> >>
> >>   drivers/gpio/sunxi_gpio.c | 62 +++++++++++++++++----------------------
> >>   1 file changed, 27 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/drivers/gpio/sunxi_gpio.c b/drivers/gpio/sunxi_gpio.c
> >> index cdbc40d48f..92fee0118d 100644
> >> --- a/drivers/gpio/sunxi_gpio.c
> >> +++ b/drivers/gpio/sunxi_gpio.c
> >> @@ -139,27 +139,6 @@ int sunxi_name_to_gpio(const char *name)
> >>       return ret ? ret : gpio;
> >>   }
> >>   -static int sunxi_gpio_direction_input(struct udevice *dev, unsigned
> >> offset)
> >> -{
> >> -    struct sunxi_gpio_plat *plat = dev_get_plat(dev);
> >> -
> >> -    sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT);
> >> -
> >> -    return 0;
> >> -}
> >> -
> >> -static int sunxi_gpio_direction_output(struct udevice *dev, unsigned
> >> offset,
> >> -                       int value)
> >> -{
> >> -    struct sunxi_gpio_plat *plat = dev_get_plat(dev);
> >> -    u32 num = GPIO_NUM(offset);
> >> -
> >> -    sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT);
> >> -    clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0);
> >> -
> >> -    return 0;
> >> -}
> >> -
> >>   static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset)
> >>   {
> >>       struct sunxi_gpio_plat *plat = dev_get_plat(dev);
> >> @@ -172,16 +151,6 @@ static int sunxi_gpio_get_value(struct udevice
> >> *dev, unsigned offset)
> >>       return dat & 0x1;
> >>   }
> >>   -static int sunxi_gpio_set_value(struct udevice *dev, unsigned offset,
> >> -                int value)
> >> -{
> >> -    struct sunxi_gpio_plat *plat = dev_get_plat(dev);
> >> -    u32 num = GPIO_NUM(offset);
> >> -
> >> -    clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0);
> >> -    return 0;
> >> -}
> >> -
> >>   static int sunxi_gpio_get_function(struct udevice *dev, unsigned
> >> offset)
> >>   {
> >>       struct sunxi_gpio_plat *plat = dev_get_plat(dev);
> >> @@ -205,18 +174,41 @@ static int sunxi_gpio_xlate(struct udevice *dev,
> >> struct gpio_desc *desc,
> >>       if (ret)
> >>           return ret;
> >>       desc->offset = args->args[1];
> >> -    desc->flags = args->args[2] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW
> >> : 0;
> >> +    desc->flags = gpio_flags_xlate(args->args[2]);
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int sunxi_gpio_set_flags(struct udevice *dev, unsigned int
> >> offset,
> >> +                ulong flags)
> >> +{
> >> +    struct sunxi_gpio_plat *plat = dev_get_plat(dev);
> >> +
> >> +    if (flags & GPIOD_IS_OUT) {
> >> +        u32 value = !!(flags & GPIOD_IS_OUT_ACTIVE);
> >> +        u32 num = GPIO_NUM(offset);
> >> +
> >> +        clrsetbits_le32(&plat->regs->dat, 1 << num, value << num);
> >> +        sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT);
> >> +    } else if (flags & GPIOD_IS_IN) {
> >> +        u32 pull = 0;
> >> +
> >> +        if (flags & GPIOD_PULL_UP)
> >> +            pull = 1;
> >> +        else if (flags & GPIOD_PULL_DOWN)
> >> +            pull = 2;
> >> +        sunxi_gpio_set_pull_bank(plat->regs, offset, pull);
> >> +        sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT);
> >> +    }
> >>         return 0;
> >>   }
> >>     static const struct dm_gpio_ops gpio_sunxi_ops = {
> >> -    .direction_input    = sunxi_gpio_direction_input,
> >> -    .direction_output    = sunxi_gpio_direction_output,  
> > 
> > Removing these operations is not related to your commit message.
> > 
> > dm_gpio_set_value() seems to be using them.  
> 
> It does not use any of these operations when set_flags is provided. The
> same applies to _dm_gpio_set_flags().
> 
> asm-generic/gpio.h says about set_flags:
> > This method is required and should be implemented by new drivers. At
> > some point, it will supersede direction_input() and
> > direction_output(), which wil be removed.  
> 
> So I believe it is intended to replace the other three operations.

That's what I get from the code and this header file comment as well.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> 
> Regards,
> Samuel
> 
> >>       .get_value        = sunxi_gpio_get_value,
> >> -    .set_value        = sunxi_gpio_set_value,  
> > 
> > Same here.
> > 
> > Best regards
> > 
> > Heinrich
> >   
> >>       .get_function        = sunxi_gpio_get_function,
> >>       .xlate            = sunxi_gpio_xlate,
> >> +    .set_flags        = sunxi_gpio_set_flags,
> >>   };
> >>     /**
> >>  
> >   
>