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

Eugen Hristev
2024-11-12T13:21:48Z

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)