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
