Message-ID: <CACRpkdaYcis2r6eNDfdXV9zcXof6x_XfGHFADJ2RojxLNp7C-A@mail.gmail.com>
Date: Tue, 1 Oct 2024 15:15:43 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Andrei Stefanescu <andrei.stefanescu@....nxp.com>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Chester Lin <chester62515@...il.com>, Matthias Brugger <mbrugger@...e.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	linux-gpio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	NXP S32 Linux Team <s32@....com>, Christophe Lizzi <clizzi@...hat.com>, Alberto Ruiz <aruizrui@...hat.com>, 
	Enric Balletbo <eballetb@...hat.com>
Subject: Re: [PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

Hi Andrei,

thanks for your patch!

Sorry for being so late in giving any deeper review of it.

On Thu, Sep 26, 2024 at 4:32 PM Andrei Stefanescu
<andrei.stefanescu@....nxp.com> 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: Ghennadi Procopciuc <ghennadi.procopciuc@....com>
> Signed-off-by: Larisa Grigore <larisa.grigore@....com>
> Signed-off-by: Phu Luu An <phu.luuan@....com>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@....nxp.com>

(...)

> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/pinctrl/consumer.h>

Hm, are you sure you don't want one of those combined
GPIO+pinctrl drivers? Look in drivers/pinctrl for examples such
as drivers/pinctrl/pinctrl-stmfx.c

> +/* PGPDOs are 16bit registers that come in big endian
> + * order if they are grouped in pairs of two.
> + *
> + * For example, the order is PGPDO1, PGPDO0, PGPDO3, PGPDO2...
> + */
> +#define SIUL2_PGPDO(N)         (((N) ^ 1) * 2)
> +#define S32G2_SIUL2_NUM                2
> +#define S32G2_PADS_DTS_TAG_LEN 7
> +
> +#define SIUL2_GPIO_16_PAD_SIZE 16

You seem to be manipulating "pads" which is pin control territory.
That should be done in a pin control driver.

> +/**
> + * struct siul2_device_data  - platform data attached to the compatible.
> + * @pad_access: access table for I/O pads, consists of S32G2_SIUL2_NUM tables.
> + * @reset_cnt: reset the pin name counter to zero when switching to SIUL2_1.
> + */
> +struct siul2_device_data {
> +       const struct regmap_access_table **pad_access;
> +       const bool reset_cnt;

I don't understand the reset_cnt variable at all. Explain why it exists in the
kerneldoc please.

> +/**
> + * struct siul2_desc - describes a SIUL2 hw module.
> + * @pad_access: array of valid I/O pads.

Now that is really pin control isn't it.

> + * @opadmap: the regmap of the Parallel GPIO Pad Data Out Register.
> + * @ipadmap: the regmap of the Parallel GPIO Pad Data In Register.
> + * @gpio_base: the first GPIO pin.
> + * @gpio_num: the number of GPIO pins.
> + */
> +struct siul2_desc {
> +       const struct regmap_access_table *pad_access;
> +       struct regmap *opadmap;
> +       struct regmap *ipadmap;
> +       u32 gpio_base;
> +       u32 gpio_num;
> +};
(...)

> +static int siul2_get_gpio_pinspec(struct platform_device *pdev,
> +                                 struct of_phandle_args *pinspec,
> +                                 unsigned int range_index)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +
> +       return of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
> +                                               range_index, pinspec);
> +}

I do not see why a driver would parse gpio-ranges since the gpiolib
core should normally deal with the translation between GPIO lines
and pins.

This looks hacky and probably an indication that you want to use
a combined pinctrl+gpio driver so the different parts of the driver
can access the same structures easily without hacks.

> +static void siul2_gpio_set_direction(struct siul2_gpio_dev *dev,
> +                                    unsigned int gpio, int dir)
> +{
> +       guard(raw_spinlock_irqsave)(&dev->lock);
> +
> +       if (dir == GPIO_LINE_DIRECTION_IN)
> +               __clear_bit(gpio, dev->pin_dir_bitmap);
> +       else
> +               __set_bit(gpio, dev->pin_dir_bitmap);
> +}

This looks like a job for gpio regmap?

select GPIO_REGMAP,
look in other driver for examples of how to use it.

> +static int siul2_get_direction(struct siul2_gpio_dev *dev,
> +                              unsigned int gpio)
> +{
> +       return test_bit(gpio, dev->pin_dir_bitmap) ? GPIO_LINE_DIRECTION_OUT :
> +                                                    GPIO_LINE_DIRECTION_IN;
> +}

Also looks like a reimplementation of GPIO_REGMAP.

> +static int siul2_gpio_dir_out(struct gpio_chip *chip, unsigned int gpio,
> +                             int val)
> +{
> +       struct siul2_gpio_dev *gpio_dev;
> +       int ret = 0;
> +
> +       gpio_dev = to_siul2_gpio_dev(chip);
> +       siul2_gpio_set_val(chip, gpio, val);
> +
> +       ret = pinctrl_gpio_direction_output(chip, gpio);
(...)

This is nice, pin control is properly used as the back-end.

> +static int siul2_gpio_remove_reserved_names(struct device *dev,
> +                                           struct siul2_gpio_dev *gpio_dev,
> +                                           char **names)
> +{
> +       struct device_node *np = dev->of_node;
> +       int num_ranges, i, j, ret;
> +       u32 base_gpio, num_gpio;
> +
> +       /* Parse the gpio-reserved-ranges to know which GPIOs to exclude. */
> +
> +       num_ranges = of_property_count_u32_elems(dev->of_node,
> +                                                "gpio-reserved-ranges");
> +
> +       /* The "gpio-reserved-ranges" is optional. */
> +       if (num_ranges < 0)
> +               return 0;
> +       num_ranges /= 2;
> +
> +       for (i = 0; i < num_ranges; i++) {
> +               ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
> +                                                i * 2, &base_gpio);
> +               if (ret) {
> +                       dev_err(dev, "Could not parse the start GPIO: %d\n",
> +                               ret);
> +                       return ret;
> +               }
> +
> +               ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
> +                                                i * 2 + 1, &num_gpio);
> +               if (ret) {
> +                       dev_err(dev, "Could not parse num. GPIOs: %d\n", ret);
> +                       return ret;
> +               }
> +
> +               if (base_gpio + num_gpio > gpio_dev->gc.ngpio) {
> +                       dev_err(dev, "Reserved GPIOs outside of GPIO range\n");
> +                       return -EINVAL;
> +               }
> +
> +               /* Remove names set for reserved GPIOs. */
> +               for (j = base_gpio; j < base_gpio + num_gpio; j++) {
> +                       devm_kfree(dev, names[j]);
> +                       names[j] = NULL;
> +               }
> +       }
> +
> +       return 0;
> +}

I don't get this. gpio-reserved-ranges is something that is parsed and
handled by gpiolib. Read the code in gpiolib.c and you'll probably
figure out how to let gpiolib take care of this for you.

> +static int siul2_gpio_populate_names(struct device *dev,
> +                                    struct siul2_gpio_dev *gpio_dev)
> +{
> +       unsigned int num_index = 0;
> +       char ch_index = 'A';
> +       char **names;
> +       int i, ret;
> +
> +       names = devm_kcalloc(dev, gpio_dev->gc.ngpio, sizeof(*names),
> +                            GFP_KERNEL);
> +       if (!names)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < S32G2_SIUL2_NUM; i++) {
> +               ret = siul2_gen_names(dev, gpio_dev->siul2[i].gpio_num,
> +                                     names + gpio_dev->siul2[i].gpio_base,
> +                                     &ch_index, &num_index);
> +               if (ret) {
> +                       dev_err(dev, "Could not set names for SIUL2_%d GPIOs\n",
> +                               i);
> +                       return ret;
> +               }
> +
> +               if (gpio_dev->platdata->reset_cnt)
> +                       num_index = 0;
> +
> +               ch_index++;
> +       }
> +
> +       ret = siul2_gpio_remove_reserved_names(dev, gpio_dev, names);
> +       if (ret)
> +               return ret;
> +
> +       gpio_dev->gc.names = (const char *const *)names;
> +
> +       return 0;
> +}

Interesting!

I'm not against, on the contrary this looks really helpful to users.

> +       gc = &gpio_dev->gc;

No poking around in the gpiolib internals please.

Look at what other drivers do and do like they do.

On top of these comments:
everywhere you do [raw_spin_]locks: try to use guards or
scoped guards instead. git grep guard drivers/gpio for
many examples.

Yours,
Linus Walleij

Powered by blists - more mailing lists