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

Linus Walleij
2020-02-21T16:26:31Z

Hi Asmaa,

thanks for your patch!

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

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

> +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