Re: [PATCH v1 1/1] gpio: add driver for Mellanox BlueField 2 GPIO controller

[Linus Walleij]

Hi Asmaa,

thanks for your patch!

On Fri, Feb 21, 2020 at 4:40 PM Asmaa Mnebhi <Asmaa@xxxxxxxxxxxx> wrote:

> This patch adds support for the GPIO controller used by
> Mellanox BlueField 2 SOCs.
>
> Signed-off-by: Asmaa Mnebhi <Asmaa@xxxxxxxxxxxx>
(...)

> +config GPIO_MLXBF2
> +       tristate "Mellanox BlueField 2 SoC GPIO"
> +       depends on (MELLANOX_PLATFORM && ARM64 && ACPI) || (64BIT && COMPILE_TEST)
> +       select GPIO_GENERIC

This selects GPIO_GENERIC but does not make use of it?

> +/*
> + * gpio[x] block registers and their offset
> + */
> +#define YU_GPIO_DATAOUT                        0x00
> +#define YU_GPIO_DATAIN                 0x04
> +#define YU_GPIO_MODE1                  0x08
> +#define YU_GPIO_MODE0                  0x0c
> +#define YU_GPIO_DATASET                        0x14
> +#define YU_GPIO_DATACLEAR              0x18
> +#define YU_GPIO_MODE1_CLEAR            0x50
> +#define YU_GPIO_MODE0_SET              0x54
> +#define YU_GPIO_MODE0_CLEAR            0x58

This however looks a lot like it could use GPIO_GENERIC.

> +       /* Must hold this lock to modify shared data. */
> +       spinlock_t lock;

Incidentally GPIO_GENERIC accessors also contains
a lock so you don't need to implement that either.

> +       /* All 3 YU GPIO block address */
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, GPIO_BLOCKS);
> +       if (!res)
> +               return -ENODEV;

If there are 3 GPIO blocks, simply spawn 3 struct gpio_chip's so you
can use the generic accessors?

> +       /* YU ARM GPIO Lock address */
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, ARM_GPIO_LOCK);
> +       if (!res)
> +               return -ENOMEM;

If the direction setting needs some special lock/unlock to happen, why not
just override the .set/get_direction() callbacks?

I would recommend:

- Split in one gpio_chip per instance
- Look at drivers/gpio/gpio-ftgpio010.c on how to populate callbacks
  for each with bgpio_init()
- In struct mlxbf2_gpio_state create some custom direction
  callbacks to store what comes back in direction_input/output
  and indirect the calls from direction_input/output to these
  with locking folded in.

Yours,
Linus Walleij