Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Andrei,

thanks for your patch!

On Mon, Aug 26, 2024 at 10:43 AM Andrei Stefanescu
<andrei.stefanescu@xxxxxxxxxxx> wrote:

> Add the GPIO driver for S32G2/S32G3 SoCs. This driver uses the SIUL2
> (System Integration Unit Lite2) hardware module. There are two
> SIUL2 hardware modules present, SIUL2_0(controlling GPIOs 0-101) and
> SIUL2_1 for the rest.
>
> The GPIOs are not fully contiguous, there are some gaps:
> - GPIO102 up to GPIO111(inclusive) are invalid
> - GPIO123 up to GPIO143(inclusive) are invalid
>
> Some GPIOs are input only(i.e. GPI182) though this restriction
> is not yet enforced in code.
>
> This patch adds basic GPIO functionality(no interrupts, no
> suspend/resume functions).
>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@xxxxxxxxxxx>

As Krzysztof points out: the driver is based on something really old and
needs an overhaul. Luckily GPIO drivers are not that big so it should be
pretty straight-forward.

> +config GPIO_SIUL2_S32G2
> +        tristate "GPIO driver for S32G2/S32G3"
> +        depends on OF_GPIO

select REGMAP?

You are using it.

> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/gpio.h>

Drop this include.

> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/bitmap.h>

Really?

> +       raw_spin_lock_irqsave(&dev->lock, flags);
> +
> +       if (dir == GPIO_LINE_DIRECTION_IN)
> +               bitmap_clear(dev->pin_dir_bitmap, gpio, 1);
> +       else
> +               bitmap_set(dev->pin_dir_bitmap, gpio, 1);

This is just an unsigned long, just use the nonatomic
__clear_bit() and __set_bit()
from <linux/bitops.h>.

> +       gc->set = siul2_gpio_set;
> +       gc->get = siul2_gpio_get;
> +       gc->set_config = siul2_set_config;
> +       gc->request = siul2_gpio_request;
> +       gc->free = siul2_gpio_free;
> +       gc->direction_output = siul2_gpio_dir_out;
> +       gc->direction_input = siul2_gpio_dir_in;
> +       gc->get_direction = siul2_gpio_get_dir;

Since it is backed by proper pin control I would expect
a generic .set_config() implementation, but no hurry with that
I suppose.

Yours,
Linus Walleij