Re: [PATCH v2 1/2] Support NVIDIA BlueField-3 GPIO controller

[Linus Walleij]

Hi Asmaa,

thanks for your patch!

On Wed, Feb 8, 2023 at 7:57 PM Asmaa Mnebhi <asmaa@xxxxxxxxxx> wrote:

> This patch adds support for the BlueField-3 SoC GPIO driver
> which allows:
> - setting certain GPIOs as interrupts from other dependent drivers
> - ability to manipulate certain GPIO pins via libgpiod tools for instance
>
> BlueField-3 has 56 GPIOs but the user is only allowed to change some
> of them into GPIO mode. Use valid_mask to make it impossible to alter
> the rest of the GPIOs.
>
> Signed-off-by: Asmaa Mnebhi <asmaa@xxxxxxxxxx>

(...)

> +struct mlxbf3_gpio_context {
> +       struct gpio_chip gc;
> +       struct irq_chip irq_chip;
> +
> +       /* YU GPIO block address */
> +       void __iomem *gpio_io;
> +
> +       /* YU GPIO cause block address */
> +       void __iomem *gpio_cause_io;

I especially like that you use the word "cause" (as in cause-and-effect)
over the imprecise and overused word "reason" (brrrr)

> +static int mlxbf3_gpio_direction_input(struct gpio_chip *chip,
> +                                      unsigned int offset)
> +{
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> +       writel(BIT(offset), gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_CLEAR);
> +
> +       raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> +       return 0;
> +}
> +
> +static int mlxbf3_gpio_direction_output(struct gpio_chip *chip,
> +                                       unsigned int offset,
> +                                       int value)
> +{
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> +       writel(BIT(offset), gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_SET);
> +
> +       if (value)
> +               writel(BIT(offset), gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_SET);
> +       else
> +               writel(BIT(offset), gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_CLEAR);
> +
> +       raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> +       return 0;
> +}

GPIO_GENERIC / bgpio should be able to handle these too.

> +       ret = bgpio_init(gc, dev, 4,
> +                       gs->gpio_io + MLXBF_GPIO_READ_DATA_IN,
> +                       gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_SET,
> +                       gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_CLEAR,
> +                       NULL, NULL, 0);

Instead of NULL, NULL, 0

use

gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_SET,
gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_CLEAR,
0);

The generic library will make sure to also set the output value when
changing direction to output.

Yours,
Linus Walleij