[PATCH 1/3] gpio: at91: Implement GPIOF_FUNC in get_function()
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
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)
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);elseat91_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)
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.
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)
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
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.
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 ?
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 ?
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)
participants (2)
-
Eugen Hristev -
Zixun LI