Hi Andrei,

thanks for your patch!

On Mon, Aug 26, 2024 at 10:43 AM Andrei Stefanescu
[off-list ref] 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 <redacted>

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