Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <cover.1582299415.git.Asmaa@mellanox.com> <d5d95e7b2823ef45878739ea338a1661df2e6a89.1582299415.git.Asmaa@mellanox.com>
In-Reply-To: <d5d95e7b2823ef45878739ea338a1661df2e6a89.1582299415.git.Asmaa@mellanox.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Fri, 21 Feb 2020 17:26:13 +0100
Message-ID: <CACRpkdYTRj-on1s55p7eHRdtO--=gJq4mhMeV3F9JbzoM_i6Xw@mail.gmail.com>
Subject: Re: [PATCH v1 1/1] gpio: add driver for Mellanox BlueField 2 GPIO controller
To:     Asmaa Mnebhi <Asmaa@mellanox.com>
Cc:     Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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