Messages in this thread | | | From | Linus Walleij <> | Date | Fri, 21 Feb 2020 17:26:13 +0100 | Subject | Re: [PATCH v1 1/1] gpio: add driver for Mellanox BlueField 2 GPIO controller |
| |
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
| |