[PATCH 1/3] gpio: at91: Implement GPIOF_FUNC in get_function()

[PATCH 1/3] gpio: at91: Implement GPIOF_FUNC in get_function()

[Zixun LI]

This patch adds support for determining whether a gpio pin is mapped as
peripheral function.

Signed-off-by: Zixun LI <admin@hifiphile.com>
---
 drivers/gpio/at91_gpio.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
index 1409db5dc1..64a6e8a4a5 100644
--- a/drivers/gpio/at91_gpio.c
+++ b/drivers/gpio/at91_gpio.c
@@ -220,6 +220,15 @@ static bool at91_get_port_output(struct at91_port *at91_port, int offset)
 	val = readl(&at91_port->osr);
 	return val & mask;
 }
+
+static bool at91_get_port_pio(struct at91_port *at91_port, int offset)
+{
+	u32 mask, val;
+
+	mask = 1 << offset;
+	val = readl(&at91_port->psr);
+	return val & mask;
+}
 #endif
 
 static void at91_set_port_input(struct at91_port *at91_port, int offset,
@@ -550,7 +559,9 @@ static int at91_gpio_get_function(struct udevice *dev, unsigned offset)
 {
 	struct at91_port_priv *port = dev_get_priv(dev);
 
-	/* GPIOF_FUNC is not implemented yet */
+	if (!at91_get_port_pio(port->regs, offset))
+		return GPIOF_FUNC;
+
 	if (at91_get_port_output(port->regs, offset))
 		return GPIOF_OUTPUT;
 	else
-- 
2.46.2

[PATCH 2/3] gpio: at91: Implement ops set_flags

[Zixun LI]

Support GPIO configuration with following flags:
- in, out, out_active
- open_drain, pull_up

Signed-off-by: Zixun LI <admin@hifiphile.com>
---
 drivers/gpio/at91_gpio.c | 41 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
index 64a6e8a4a5..7828f9b447 100644
--- a/drivers/gpio/at91_gpio.c
+++ b/drivers/gpio/at91_gpio.c
@@ -229,6 +229,17 @@ static bool at91_get_port_pio(struct at91_port *at91_port, int offset)
 	val = readl(&at91_port->psr);
 	return val & mask;
 }
+
+static void at91_set_port_multi_drive(struct at91_port *at91_port, int offset, int is_on)
+{
+	u32 mask;
+
+	mask = 1 << offset;
+	if (is_on)
+		writel(mask, &at91_port->mder);
+	else
+		writel(mask, &at91_port->mddr);
+}
 #endif
 
 static void at91_set_port_input(struct at91_port *at91_port, int offset,
@@ -568,6 +579,35 @@ static int at91_gpio_get_function(struct udevice *dev, unsigned offset)
 		return GPIOF_INPUT;
 }
 
+static int at91_gpio_set_flags(struct udevice *dev, unsigned int offset,
+				ulong flags)
+{
+	struct at91_port_priv *port = dev_get_priv(dev);
+	ulong supported_mask;
+
+	supported_mask = GPIOD_OPEN_DRAIN | GPIOD_MASK_DIR | GPIOD_PULL_UP;
+	if (flags & ~supported_mask)
+		return -EINVAL;
+
+	if (flags & GPIOD_IS_OUT) {
+		bool value = flags & GPIOD_IS_OUT_ACTIVE;
+
+		if (flags & GPIOD_OPEN_DRAIN)
+			at91_set_port_multi_drive(port->regs, offset, true);
+		else
+			at91_set_port_multi_drive(port->regs, offset, false);
+
+		at91_set_port_output(port->regs, offset, value);
+
+	} else if (flags & GPIOD_IS_IN) {
+		at91_set_port_input(port->regs, offset, false);
+	}
+	if (flags & GPIOD_PULL_UP)
+		at91_set_port_pullup(port->regs, offset, true);
+
+	return 0;
+}
+
 static const char *at91_get_bank_name(uint32_t base_addr)
 {
 	switch (base_addr) {
@@ -596,6 +636,7 @@ static const struct dm_gpio_ops gpio_at91_ops = {
 	.get_value		= at91_gpio_get_value,
 	.set_value		= at91_gpio_set_value,
 	.get_function		= at91_gpio_get_function,
+	.set_flags		= at91_gpio_set_flags,
 };
 
 static int at91_gpio_probe(struct udevice *dev)
-- 
2.46.2

[PATCH 3/3] gpio: at91: Implement ops get_flags

[Zixun LI]

Add ops get_dir_flags() to read status from GPIO registers.

Signed-off-by: Zixun LI <admin@hifiphile.com>
---
 drivers/gpio/at91_gpio.c | 45 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
index 7828f9b447..c986007716 100644
--- a/drivers/gpio/at91_gpio.c
+++ b/drivers/gpio/at91_gpio.c
@@ -240,6 +240,24 @@ static void at91_set_port_multi_drive(struct at91_port *at91_port, int offset, i
 	else
 		writel(mask, &at91_port->mddr);
 }
+
+static bool at91_get_port_multi_drive(struct at91_port *at91_port, int offset)
+{
+	u32 mask, val;
+
+	mask = 1 << offset;
+	val = readl(&at91_port->mdsr);
+	return val & mask;
+}
+
+static bool at91_get_port_pullup(struct at91_port *at91_port, int offset)
+{
+	u32 mask, val;
+
+	mask = 1 << offset;
+	val = readl(&at91_port->pusr);
+	return val & mask;
+}
 #endif
 
 static void at91_set_port_input(struct at91_port *at91_port, int offset,
@@ -608,6 +626,32 @@ static int at91_gpio_set_flags(struct udevice *dev, unsigned int offset,
 	return 0;
 }
 
+static int at91_gpio_get_flags(struct udevice *dev, unsigned int offset,
+				ulong *flagsp)
+{
+	struct at91_port_priv *port = dev_get_priv(dev);
+	ulong dir_flags = 0;
+
+	if (at91_get_port_output(port->regs, offset)) {
+		dir_flags |= GPIOD_IS_OUT;
+
+		if (at91_get_port_multi_drive(port->regs, offset))
+			dir_flags |= GPIOD_OPEN_DRAIN;
+
+		if (at91_get_port_value(port->regs, offset))
+			dir_flags |= GPIOD_IS_OUT_ACTIVE;
+	} else {
+		dir_flags |= GPIOD_IS_IN;
+	}
+
+	if (at91_get_port_pullup(port->regs, offset))
+		dir_flags |= GPIOD_PULL_UP;
+
+	*flagsp = dir_flags;
+
+	return 0;
+}
+
 static const char *at91_get_bank_name(uint32_t base_addr)
 {
 	switch (base_addr) {
@@ -637,6 +681,7 @@ static const struct dm_gpio_ops gpio_at91_ops = {
 	.set_value		= at91_gpio_set_value,
 	.get_function		= at91_gpio_get_function,
 	.set_flags		= at91_gpio_set_flags,
+	.get_flags		= at91_gpio_get_flags,
 };
 
 static int at91_gpio_probe(struct udevice *dev)
-- 
2.46.2

Re: [PATCH 1/3] gpio: at91: Implement GPIOF_FUNC in get_function()

[Eugen Hristev]

Hello,

On 10/18/24 23:27, Zixun LI wrote:
> This patch adds support for determining whether a gpio pin is mapped as
> peripheral function.
> 
> Signed-off-by: Zixun LI <admin@hifiphile.com>
> ---
>  drivers/gpio/at91_gpio.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
> index 1409db5dc1..64a6e8a4a5 100644
> --- a/drivers/gpio/at91_gpio.c
> +++ b/drivers/gpio/at91_gpio.c
> @@ -220,6 +220,15 @@ static bool at91_get_port_output(struct at91_port *at91_port, int offset)
>  	val = readl(&at91_port->osr);
>  	return val & mask;
>  }
> +
> +static bool at91_get_port_pio(struct at91_port *at91_port, int offset)

The name get_port_pio is a bit confusing, can you rename it to something
more meaningful, like maybe is_periph_func or something ?

> +{
> +	u32 mask, val;
> +
> +	mask = 1 << offset;
> +	val = readl(&at91_port->psr);
> +	return val & mask;
> +}
>  #endif
>  
>  static void at91_set_port_input(struct at91_port *at91_port, int offset,
> @@ -550,7 +559,9 @@ static int at91_gpio_get_function(struct udevice *dev, unsigned offset)
>  {
>  	struct at91_port_priv *port = dev_get_priv(dev);
>  
> -	/* GPIOF_FUNC is not implemented yet */
> +	if (!at91_get_port_pio(port->regs, offset))
> +		return GPIOF_FUNC;
> +
>  	if (at91_get_port_output(port->regs, offset))
>  		return GPIOF_OUTPUT;
>  	else

Re: [PATCH 2/3] gpio: at91: Implement ops set_flags

[Eugen Hristev]

Hello,

On 10/18/24 23:27, Zixun LI wrote:
> Support GPIO configuration with following flags:
> - in, out, out_active
> - open_drain, pull_up
> 
> Signed-off-by: Zixun LI <admin@hifiphile.com>
> ---
>  drivers/gpio/at91_gpio.c | 41 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
> index 64a6e8a4a5..7828f9b447 100644
> --- a/drivers/gpio/at91_gpio.c
> +++ b/drivers/gpio/at91_gpio.c
> @@ -229,6 +229,17 @@ static bool at91_get_port_pio(struct at91_port *at91_port, int offset)
>  	val = readl(&at91_port->psr);
>  	return val & mask;
>  }
> +
> +static void at91_set_port_multi_drive(struct at91_port *at91_port, int offset, int is_on)
> +{
> +	u32 mask;
> +
> +	mask = 1 << offset;
> +	if (is_on)
> +		writel(mask, &at91_port->mder);
> +	else
> +		writel(mask, &at91_port->mddr);
> +}
>  #endif
>  
>  static void at91_set_port_input(struct at91_port *at91_port, int offset,
> @@ -568,6 +579,35 @@ static int at91_gpio_get_function(struct udevice *dev, unsigned offset)
>  		return GPIOF_INPUT;
>  }
>  
> +static int at91_gpio_set_flags(struct udevice *dev, unsigned int offset,
> +				ulong flags)
> +{
> +	struct at91_port_priv *port = dev_get_priv(dev);
> +	ulong supported_mask;
> +
> +	supported_mask = GPIOD_OPEN_DRAIN | GPIOD_MASK_DIR | GPIOD_PULL_UP;
> +	if (flags & ~supported_mask)
> +		return -EINVAL;

Does this change the current behavior? there is no set_flag ops
implemented, previously it would use a default that would just return
success regardless of the given flags parameters ?
Btw maybe ENOTSUPP ?

> +
> +	if (flags & GPIOD_IS_OUT) {
> +		bool value = flags & GPIOD_IS_OUT_ACTIVE;

Can you please declare this value at the start of the function, or
inline it below when it's being used

> +
> +		if (flags & GPIOD_OPEN_DRAIN)
> +			at91_set_port_multi_drive(port->regs, offset, true);
> +		else
> +			at91_set_port_multi_drive(port->regs, offset, false);
> +
> +		at91_set_port_output(port->regs, offset, value);
> +
> +	} else if (flags & GPIOD_IS_IN) {
> +		at91_set_port_input(port->regs, offset, false);
> +	}
> +	if (flags & GPIOD_PULL_UP)
> +		at91_set_port_pullup(port->regs, offset, true);
> +
> +	return 0;
> +}
> +
>  static const char *at91_get_bank_name(uint32_t base_addr)
>  {
>  	switch (base_addr) {
> @@ -596,6 +636,7 @@ static const struct dm_gpio_ops gpio_at91_ops = {
>  	.get_value		= at91_gpio_get_value,
>  	.set_value		= at91_gpio_set_value,
>  	.get_function		= at91_gpio_get_function,
> +	.set_flags		= at91_gpio_set_flags,
>  };
>  
>  static int at91_gpio_probe(struct udevice *dev)

Re: [PATCH 1/3] gpio: at91: Implement GPIOF_FUNC in get_function()

[Zixun LI]

Hello,


On Tue, Nov 12, 2024 at 2:13 PM Eugen Hristev <eugen.hristev@linaro.org> wrote:
>
>
> > +static bool at91_get_port_pio(struct at91_port *at91_port, int offset)
>
> The name get_port_pio is a bit confusing, can you rename it to something
> more meaningful, like maybe is_periph_func or something ?

How about at91_is_port_pio_active, more inline to the datasheet.

Re: [PATCH 1/3] gpio: at91: Implement GPIOF_FUNC in get_function()

[Eugen Hristev]

On 11/12/24 17:57, Zixun LI wrote:
> Hello,
> 
> 
> On Tue, Nov 12, 2024 at 2:13 PM Eugen Hristev <eugen.hristev@linaro.org> wrote:
>>
>>
>>> +static bool at91_get_port_pio(struct at91_port *at91_port, int offset)
>>
>> The name get_port_pio is a bit confusing, can you rename it to something
>> more meaningful, like maybe is_periph_func or something ?
> 
> How about at91_is_port_pio_active, more inline to the datasheet.


I am not sure. It appears confusing to me.
The pin is either in GPIO or peripheral function mode.
Being 'PIO active' appears to be something different, or common to both
options.
What does the datasheet say exactly ?

Re: [PATCH 2/3] gpio: at91: Implement ops set_flags

[Zixun LI]

Hi,

On Tue, Nov 12, 2024 at 2:21 PM Eugen Hristev <eugen.hristev@linaro.org> wrote:
>
> Does this change the current behavior? there is no set_flag ops
> implemented, previously it would use a default that would just return
> success regardless of the given flags parameters ?
> Btw maybe ENOTSUPP ?
>

Currently without .set_flags ops it just returns success regardless of the
given flags parameters. It's problematic especially when pull-up/down is used.
I've another patch to fix the silent failure:
https://lists.denx.de/pipermail/u-boot/2024-October/569044.html

Most drivers just ignore unsupported flags, I copied flag check from mcp230xx
driver. Is it better to return ENOTSUPP instead ?

> > +
> > +     if (flags & GPIOD_IS_OUT) {
> > +             bool value = flags & GPIOD_IS_OUT_ACTIVE;
>
> Can you please declare this value at the start of the function, or
> inline it below when it's being used
>

Will do.

Re: [PATCH 1/3] gpio: at91: Implement GPIOF_FUNC in get_function()

[Zixun LI]

On Tue, Nov 12, 2024 at 4:59 PM Eugen Hristev <eugen.hristev@linaro.org> wrote:
>
>
>
> On 11/12/24 17:57, Zixun LI wrote:
> > Hello,
> >
> >
> > On Tue, Nov 12, 2024 at 2:13 PM Eugen Hristev <eugen.hristev@linaro.org> wrote:
> >>
> >>
> >>> +static bool at91_get_port_pio(struct at91_port *at91_port, int offset)
> >>
> >> The name get_port_pio is a bit confusing, can you rename it to something
> >> more meaningful, like maybe is_periph_func or something ?
> >
> > How about at91_is_port_pio_active, more inline to the datasheet.
>
>
> I am not sure. It appears confusing to me.
> The pin is either in GPIO or peripheral function mode.
> Being 'PIO active' appears to be something different, or common to both
> options.
> What does the datasheet say exactly ?

From SAM9G25 datasheet, 22.6.3 PIO Status Register

0: PIO is inactive on the corresponding I/O line (peripheral is active).
1: PIO is active on the corresponding I/O line (peripheral is inactive).

"PIO" is the port controller itself but also means GPIO which is not very clear.

Or at91_is_port_gpio ?

Re: [PATCH 1/3] gpio: at91: Implement GPIOF_FUNC in get_function()

[Eugen Hristev]

On 11/12/24 18:17, Zixun LI wrote:
> On Tue, Nov 12, 2024 at 4:59 PM Eugen Hristev <eugen.hristev@linaro.org> wrote:
>>
>>
>>
>> On 11/12/24 17:57, Zixun LI wrote:
>>> Hello,
>>>
>>>
>>> On Tue, Nov 12, 2024 at 2:13 PM Eugen Hristev <eugen.hristev@linaro.org> wrote:
>>>>
>>>>
>>>>> +static bool at91_get_port_pio(struct at91_port *at91_port, int offset)
>>>>
>>>> The name get_port_pio is a bit confusing, can you rename it to something
>>>> more meaningful, like maybe is_periph_func or something ?
>>>
>>> How about at91_is_port_pio_active, more inline to the datasheet.
>>
>>
>> I am not sure. It appears confusing to me.
>> The pin is either in GPIO or peripheral function mode.
>> Being 'PIO active' appears to be something different, or common to both
>> options.
>> What does the datasheet say exactly ?
> 
> From SAM9G25 datasheet, 22.6.3 PIO Status Register
> 
> 0: PIO is inactive on the corresponding I/O line (peripheral is active).
> 1: PIO is active on the corresponding I/O line (peripheral is inactive).
> 
> "PIO" is the port controller itself but also means GPIO which is not very clear.
> 
> Or at91_is_port_gpio ?

That sounds better (and have the binary logic reversed)

IIRC on SAM datasheets and naming, 'pio' is a general term for a pin,
and this pin can either work as a GPIO or it can be tied to a specific
hardware block (hence the function)