Message-ID: <68b6b76d-f9a2-4207-a980-a319cd541381@oss.nxp.com>
Date: Wed, 2 Oct 2024 18:05:27 +0300
From: Andrei Stefanescu <andrei.stefanescu@....nxp.com>
To: Linus Walleij <linus.walleij@...aro.org>
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 Linus,

Thank you for the review!

On 01/10/2024 16:15, Linus Walleij wrote:
> 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>
> 
> (...)

I will add the Co-developed-by tags in v5.

> 
>> +#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

Thank you for the suggestion! I will merge the two drivers in v5.
> 
>> +/* 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.
> 

This is more of a case of bad naming, the registers used for
setting/reading a GPIO's value are called Parallel GPIO Pad Data
Output/Input (PGPDO/PGPDI) and they are 16bit wide. I will try to
find a better name for this.

>> +/**
>> + * 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.

It is used while generating the GPIO line names. The naming scheme is to
have: P + letter + number(0 to 15). The "reset_cnt" is used to tell if
the number should change to 0 when naming the SIUL21 GPIOs. For example,
for S32G2, the SIUL20 module has 102 pins named PA00, PA01, ..., PA15,
PB0, .., PG05. SIUL21 starts at 112 and reset_cnt is true so the first
pin will be PH00.

We have another platform which uses this driver and doesn't have the 102-111 gap
and the first SIUL21 pin is named PH06.

I will remove this. I figured out now that I can see if the first pin is
divisible by 16 and reset the counter in that case.

> 
>> +/**
>> + * 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.

This is again referring to the registers for setting the
the output value/reading the input value of a GPIO. I will
change the name in v5.

> 
>> + * @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.

We parse the gpio-ranges property because this driver handles
two GPIO hardware IPs in a single driver instance. This is because
both SIUL2 modules have pins with interrupt capability but the
registers to configure interrupts are part of SIUL21.
We use the gpio_base and gpio_num to generate the line names
and to select the correct regmap for PGPDOs/PGPDIs
(Parallel GPIO Pad Data Output/Input).

> 
>> +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.

We use the bitmap just to store the current direction of a pad.
I don't think we can use the gpio-regmap driver because we
rely on the pinctrl driver to configure the pin(pinmux&pinconf).

> 
>> +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.

The answer above applies here as well.

> 
>> +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.

We use this just to remove line names for reserved GPIOs(in our case
not present, we have some gaps).

> 
>> +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.

I can try to integrate this into gpiolib after I merge this driver.

> 
>> +       gc = &gpio_dev->gc;
> 
> No poking around in the gpiolib internals please.
> 
> Look at what other drivers do and do like they do.

I will fix in v5.

> 
> 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.

I will fix in v5.

> 
> Yours,
> Linus Walleij

Best regards,
Andrei

Powered by blists - more mailing lists