[PATCH 0/4] Fix Samsung pinctrl driver static allocation of GPIO base warning

[PATCH 0/4] Fix Samsung pinctrl driver static allocation of GPIO base warning

[Mateusz Majewski]

The object of this work is fixing the following warning, which appears
on all targets using that driver:

gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation.

This needs a small refactor to how we interact with the pinctrl
subsystem. Finally, we remove some bookkeeping that has only been
necessary to allocate GPIO bases correctly.

Mateusz Majewski (4):
  pinctrl: samsung: defer pinctrl_enable
  pinctrl: samsung: use add_pin_ranges method to add pinctrl ranges
  pinctrl: samsung: choose GPIO numberspace base dynamically
  pinctrl: samsung: do not offset pinctrl numberspaces

 drivers/pinctrl/samsung/pinctrl-samsung.c | 56 ++++++++++++-----------
 drivers/pinctrl/samsung/pinctrl-samsung.h |  4 +-
 2 files changed, 31 insertions(+), 29 deletions(-)

-- 
2.42.0

[PATCH 1/4] pinctrl: samsung: defer pinctrl_enable

[Mateusz Majewski]

dev_pinctrl_register function immediately enables the pinctrl subsystem,
which is unpreferable in general, since drivers might be unable to
handle calls immediately. Hence devm_pinctrl_register_and_init, which
does not call pinctrl_enable, is preferred.

In case of our driver using the old function does not seem to be
problematic for now, but will become an issue when we postpone parts of
pinctrl initialization in a future commit, and it is a good idea to move
off a deprecated-ish function anyway.

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index e54847040b4a..e496af72a587 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -904,11 +904,11 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
 	if (ret)
 		return ret;
 
-	drvdata->pctl_dev = devm_pinctrl_register(&pdev->dev, ctrldesc,
-						  drvdata);
-	if (IS_ERR(drvdata->pctl_dev)) {
+	ret = devm_pinctrl_register_and_init(&pdev->dev, ctrldesc, drvdata,
+					     &drvdata->pctl_dev);
+	if (ret) {
 		dev_err(&pdev->dev, "could not register pinctrl driver\n");
-		return PTR_ERR(drvdata->pctl_dev);
+		return ret;
 	}
 
 	for (bank = 0; bank < drvdata->nr_banks; ++bank) {
@@ -1176,6 +1176,10 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_unregister;
 
+	ret = pinctrl_enable(drvdata->pctl_dev);
+	if (ret)
+		goto err_unregister;
+
 	platform_set_drvdata(pdev, drvdata);
 
 	return 0;
-- 
2.42.0

Re: [PATCH 1/4] pinctrl: samsung: defer pinctrl_enable

[Sam Protsenko]

On Fri, Oct 6, 2023 at 8:01 AM Mateusz Majewski <m.majewski2@samsung.com> wrote:
>
> dev_pinctrl_register function immediately enables the pinctrl subsystem,

Nitpick: dev -> devm

> which is unpreferable in general, since drivers might be unable to
> handle calls immediately. Hence devm_pinctrl_register_and_init, which
> does not call pinctrl_enable, is preferred.
>
> In case of our driver using the old function does not seem to be
> problematic for now, but will become an issue when we postpone parts of
> pinctrl initialization in a future commit, and it is a good idea to move
> off a deprecated-ish function anyway.
>
> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index e54847040b4a..e496af72a587 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -904,11 +904,11 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
>         if (ret)
>                 return ret;
>
> -       drvdata->pctl_dev = devm_pinctrl_register(&pdev->dev, ctrldesc,
> -                                                 drvdata);
> -       if (IS_ERR(drvdata->pctl_dev)) {
> +       ret = devm_pinctrl_register_and_init(&pdev->dev, ctrldesc, drvdata,
> +                                            &drvdata->pctl_dev);
> +       if (ret) {
>                 dev_err(&pdev->dev, "could not register pinctrl driver\n");
> -               return PTR_ERR(drvdata->pctl_dev);
> +               return ret;
>         }
>
>         for (bank = 0; bank < drvdata->nr_banks; ++bank) {
> @@ -1176,6 +1176,10 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
>         if (ret)
>                 goto err_unregister;
>
> +       ret = pinctrl_enable(drvdata->pctl_dev);
> +       if (ret)
> +               goto err_unregister;
> +
>         platform_set_drvdata(pdev, drvdata);
>
>         return 0;
> --
> 2.42.0
>

[PATCH 2/4] pinctrl: samsung: use add_pin_ranges method to add pinctrl ranges

[Mateusz Majewski]

This is preferable since we can read the base in the global GPIO
numberspace from the chip instead of needing to select it ourselves.

Past versions could not do this, since they needed to add all the ranges
before enabling the pinctrl subsystem, which was done before registering
the GPIO chip. However, right now we enable the pinctrl subsystem after
registering the chip and so this became possible.

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 31 +++++++++++++----------
 drivers/pinctrl/samsung/pinctrl-samsung.h |  2 ++
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index e496af72a587..511a3ac51f76 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -665,6 +665,21 @@ static int samsung_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
 	return (virq) ? : -ENXIO;
 }
 
+static int samsung_add_pin_ranges(struct gpio_chip *gc)
+{
+	struct samsung_pin_bank *bank = gpiochip_get_data(gc);
+
+	bank->grange.name = bank->name;
+	bank->grange.id = bank->id;
+	bank->grange.pin_base = bank->drvdata->pin_base + bank->pin_base;
+	bank->grange.base = gc->base;
+	bank->grange.npins = bank->nr_pins;
+	bank->grange.gc = &bank->gpio_chip;
+	pinctrl_add_gpio_range(bank->drvdata->pctl_dev, &bank->grange);
+
+	return 0;
+}
+
 static struct samsung_pin_group *samsung_pinctrl_create_groups(
 				struct device *dev,
 				struct samsung_pinctrl_drv_data *drvdata,
@@ -892,6 +907,7 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
 	/* for each pin, the name of the pin is pin-bank name + pin number */
 	for (bank = 0; bank < drvdata->nr_banks; bank++) {
 		pin_bank = &drvdata->pin_banks[bank];
+		pin_bank->id = bank;
 		for (pin = 0; pin < pin_bank->nr_pins; pin++) {
 			sprintf(pin_names, "%s-%d", pin_bank->name, pin);
 			pdesc = pindesc + pin_bank->pin_base + pin;
@@ -911,18 +927,6 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
 		return ret;
 	}
 
-	for (bank = 0; bank < drvdata->nr_banks; ++bank) {
-		pin_bank = &drvdata->pin_banks[bank];
-		pin_bank->grange.name = pin_bank->name;
-		pin_bank->grange.id = bank;
-		pin_bank->grange.pin_base = drvdata->pin_base
-						+ pin_bank->pin_base;
-		pin_bank->grange.base = pin_bank->grange.pin_base;
-		pin_bank->grange.npins = pin_bank->nr_pins;
-		pin_bank->grange.gc = &pin_bank->gpio_chip;
-		pinctrl_add_gpio_range(drvdata->pctl_dev, &pin_bank->grange);
-	}
-
 	return 0;
 }
 
@@ -947,6 +951,7 @@ static const struct gpio_chip samsung_gpiolib_chip = {
 	.direction_input = samsung_gpio_direction_input,
 	.direction_output = samsung_gpio_direction_output,
 	.to_irq = samsung_gpio_to_irq,
+	.add_pin_ranges = samsung_add_pin_ranges,
 	.owner = THIS_MODULE,
 };
 
@@ -963,7 +968,7 @@ static int samsung_gpiolib_register(struct platform_device *pdev,
 		bank->gpio_chip = samsung_gpiolib_chip;
 
 		gc = &bank->gpio_chip;
-		gc->base = bank->grange.base;
+		gc->base = drvdata->pin_base + bank->pin_base;
 		gc->ngpio = bank->nr_pins;
 		gc->parent = &pdev->dev;
 		gc->fwnode = bank->fwnode;
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index 9af93e3d8d9f..173db20f70d3 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -148,6 +148,7 @@ struct samsung_pin_bank_data {
  * @eint_mask: bit mask of pins which support EINT function.
  * @eint_offset: SoC-specific EINT register or interrupt offset of bank.
  * @name: name to be prefixed for each pin in this pin bank.
+ * @id: id of the bank, propagated to the pin range.
  * @pin_base: starting pin number of the bank.
  * @soc_priv: per-bank private data for SoC-specific code.
  * @of_node: OF node of the bank.
@@ -170,6 +171,7 @@ struct samsung_pin_bank {
 	u32		eint_mask;
 	u32		eint_offset;
 	const char	*name;
+	u32		id;
 
 	u32		pin_base;
 	void		*soc_priv;
-- 
2.42.0

[PATCH 3/4] pinctrl: samsung: choose GPIO numberspace base dynamically

[Mateusz Majewski]

Selecting it statically is deprecated and results in a warning while
booting the system:

gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation.

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 511a3ac51f76..952aeeebb679 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -968,7 +968,7 @@ static int samsung_gpiolib_register(struct platform_device *pdev,
 		bank->gpio_chip = samsung_gpiolib_chip;
 
 		gc = &bank->gpio_chip;
-		gc->base = drvdata->pin_base + bank->pin_base;
+		gc->base = -1; /* Dynamic allocation */
 		gc->ngpio = bank->nr_pins;
 		gc->parent = &pdev->dev;
 		gc->fwnode = bank->fwnode;
-- 
2.42.0

[PATCH 4/4] pinctrl: samsung: do not offset pinctrl numberspaces

[Mateusz Majewski]

Past versions of this driver have manually calculated base values for
both the pinctrl numberspace and the global GPIO numberspace, giving
both the same values. This was necessary for the global GPIO
numberspace, since its values need to be unique system-wide. However, it
was not necessary for the pinctrl numberspace, since its values only
need to be unique for a single instance of the pinctrl device. It was
just convenient to use the same values for both spaces.

Right now those calculations are only used for the pinctrl numberspace,
since GPIO numberspace bases are selected by the GPIO subsystem.
Therefore, those calculations are unnecessary.

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 15 ++++-----------
 drivers/pinctrl/samsung/pinctrl-samsung.h |  2 --
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 952aeeebb679..79babbb39ced 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -45,8 +45,6 @@ static struct pin_config {
 	{ "samsung,pin-val", PINCFG_TYPE_DAT },
 };
 
-static unsigned int pin_base;
-
 static int samsung_get_group_count(struct pinctrl_dev *pctldev)
 {
 	struct samsung_pinctrl_drv_data *pmx = pinctrl_dev_get_drvdata(pctldev);
@@ -389,8 +387,7 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
 	func = &drvdata->pmx_functions[selector];
 	grp = &drvdata->pin_groups[group];
 
-	pin_to_reg_bank(drvdata, grp->pins[0] - drvdata->pin_base,
-			&reg, &pin_offset, &bank);
+	pin_to_reg_bank(drvdata, grp->pins[0], &reg, &pin_offset, &bank);
 	type = bank->type;
 	mask = (1 << type->fld_width[PINCFG_TYPE_FUNC]) - 1;
 	shift = pin_offset * type->fld_width[PINCFG_TYPE_FUNC];
@@ -441,8 +438,7 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
 	unsigned long flags;
 
 	drvdata = pinctrl_dev_get_drvdata(pctldev);
-	pin_to_reg_bank(drvdata, pin - drvdata->pin_base, &reg_base,
-					&pin_offset, &bank);
+	pin_to_reg_bank(drvdata, pin, &reg_base, &pin_offset, &bank);
 	type = bank->type;
 
 	if (cfg_type >= PINCFG_TYPE_NUM || !type->fld_width[cfg_type])
@@ -671,7 +667,7 @@ static int samsung_add_pin_ranges(struct gpio_chip *gc)
 
 	bank->grange.name = bank->name;
 	bank->grange.id = bank->id;
-	bank->grange.pin_base = bank->drvdata->pin_base + bank->pin_base;
+	bank->grange.pin_base = bank->pin_base;
 	bank->grange.base = gc->base;
 	bank->grange.npins = bank->nr_pins;
 	bank->grange.gc = &bank->gpio_chip;
@@ -891,7 +887,7 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
 
 	/* dynamically populate the pin number and pin name for pindesc */
 	for (pin = 0, pdesc = pindesc; pin < ctrldesc->npins; pin++, pdesc++)
-		pdesc->number = pin + drvdata->pin_base;
+		pdesc->number = pin;
 
 	/*
 	 * allocate space for storing the dynamically generated names for all
@@ -1129,9 +1125,6 @@ samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
 
 	samsung_banks_node_get(&pdev->dev, d);
 
-	d->pin_base = pin_base;
-	pin_base += d->nr_pins;
-
 	return ctrl;
 }
 
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index 173db20f70d3..9b3db50adef3 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -269,7 +269,6 @@ struct samsung_pin_ctrl {
  * @nr_groups: number of such pin groups.
  * @pmx_functions: list of pin functions available to the driver.
  * @nr_function: number of such pin functions.
- * @pin_base: starting system wide pin number.
  * @nr_pins: number of pins supported by the controller.
  * @retention_ctrl: retention control runtime data.
  * @suspend: platform specific suspend callback, executed during pin controller
@@ -293,7 +292,6 @@ struct samsung_pinctrl_drv_data {
 
 	struct samsung_pin_bank		*pin_banks;
 	unsigned int			nr_banks;
-	unsigned int			pin_base;
 	unsigned int			nr_pins;
 
 	struct samsung_retention_ctrl	*retention_ctrl;
-- 
2.42.0

Re: [PATCH 0/4] Fix Samsung pinctrl driver static allocation of GPIO base warning

[Sam Protsenko]

On Fri, Oct 6, 2023 at 8:01 AM Mateusz Majewski <m.majewski2@samsung.com> wrote:
>
> The object of this work is fixing the following warning, which appears
> on all targets using that driver:
>
> gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation.
>
> This needs a small refactor to how we interact with the pinctrl
> subsystem. Finally, we remove some bookkeeping that has only been
> necessary to allocate GPIO bases correctly.
>
> Mateusz Majewski (4):
>   pinctrl: samsung: defer pinctrl_enable
>   pinctrl: samsung: use add_pin_ranges method to add pinctrl ranges
>   pinctrl: samsung: choose GPIO numberspace base dynamically
>   pinctrl: samsung: do not offset pinctrl numberspaces
>
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 56 ++++++++++++-----------
>  drivers/pinctrl/samsung/pinctrl-samsung.h |  4 +-
>  2 files changed, 31 insertions(+), 29 deletions(-)
>
> --

Hi Mateusz,

Thank you for handling this! Those deprecation warnings have been
bugging me for some time :) While testing this series on my E850-96
board (Exynos850 based), I noticed some changes in
/sys/kernel/debug/gpio file, like these:

8<------------------------------------------------------------------------------------------>8
-gpiochip0: GPIOs 0-7, parent: platform/11850000.pinctrl, gpa0:
- gpio-7   (                    |Volume Up           ) in  hi IRQ ACTIVE LOW
+gpiochip0: GPIOs 512-519, parent: platform/11850000.pinctrl, gpa0:
+ gpio-519 (                    |Volume Up           ) in  hi IRQ ACTIVE LOW

-gpiochip1: GPIOs 8-15, parent: platform/11850000.pinctrl, gpa1:
- gpio-8   (                    |Volume Down         ) in  hi IRQ ACTIVE LOW
+gpiochip1: GPIOs 520-527, parent: platform/11850000.pinctrl, gpa1:
+ gpio-520 (                    |Volume Down         ) in  hi IRQ ACTIVE LOW

-gpiochip2: GPIOs 16-23, parent: platform/11850000.pinctrl, gpa2:
+gpiochip2: GPIOs 528-535, parent: platform/11850000.pinctrl, gpa2:

...
8<------------------------------------------------------------------------------------------>8

So basically it looks like all line numbers were offset by 512. Can
you please comment on this? Is it an intentional change, and why it's
happening?

Despite of that change, everything seems to be working fine. But I
kinda liked the numeration starting from 0 better :)

Thanks!

> 2.42.0
>

Re: [PATCH 0/4] Fix Samsung pinctrl driver static allocation of GPIO base warning

[Krzysztof Kozlowski]

On 07/10/2023 04:14, Sam Protsenko wrote:
> On Fri, Oct 6, 2023 at 8:01 AM Mateusz Majewski <m.majewski2@samsung.com> wrote:
>>
>> The object of this work is fixing the following warning, which appears
>> on all targets using that driver:
>>
>> gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation.
>>
>> This needs a small refactor to how we interact with the pinctrl
>> subsystem. Finally, we remove some bookkeeping that has only been
>> necessary to allocate GPIO bases correctly.
>>
>> Mateusz Majewski (4):
>>   pinctrl: samsung: defer pinctrl_enable
>>   pinctrl: samsung: use add_pin_ranges method to add pinctrl ranges
>>   pinctrl: samsung: choose GPIO numberspace base dynamically
>>   pinctrl: samsung: do not offset pinctrl numberspaces
>>
>>  drivers/pinctrl/samsung/pinctrl-samsung.c | 56 ++++++++++++-----------
>>  drivers/pinctrl/samsung/pinctrl-samsung.h |  4 +-
>>  2 files changed, 31 insertions(+), 29 deletions(-)
>>
>> --
> 
> Hi Mateusz,
> 
> Thank you for handling this! Those deprecation warnings have been
> bugging me for some time :) While testing this series on my E850-96
> board (Exynos850 based), I noticed some changes in
> /sys/kernel/debug/gpio file, like these:
> 
> 8<------------------------------------------------------------------------------------------>8
> -gpiochip0: GPIOs 0-7, parent: platform/11850000.pinctrl, gpa0:
> - gpio-7   (                    |Volume Up           ) in  hi IRQ ACTIVE LOW
> +gpiochip0: GPIOs 512-519, parent: platform/11850000.pinctrl, gpa0:
> + gpio-519 (                    |Volume Up           ) in  hi IRQ ACTIVE LOW
> 
> -gpiochip1: GPIOs 8-15, parent: platform/11850000.pinctrl, gpa1:
> - gpio-8   (                    |Volume Down         ) in  hi IRQ ACTIVE LOW
> +gpiochip1: GPIOs 520-527, parent: platform/11850000.pinctrl, gpa1:
> + gpio-520 (                    |Volume Down         ) in  hi IRQ ACTIVE LOW
> 
> -gpiochip2: GPIOs 16-23, parent: platform/11850000.pinctrl, gpa2:
> +gpiochip2: GPIOs 528-535, parent: platform/11850000.pinctrl, gpa2:
> 
> ...
> 8<------------------------------------------------------------------------------------------>8
> 
> So basically it looks like all line numbers were offset by 512. Can
> you please comment on this? Is it an intentional change, and why it's
> happening?
> 
> Despite of that change, everything seems to be working fine. But I
> kinda liked the numeration starting from 0 better :)

Could it be the reason of dynamic allocation?


Best regards,
Krzysztof

Re: [PATCH 0/4] Fix Samsung pinctrl driver static allocation of GPIO base warning

[Sam Protsenko]

On Sun, Oct 8, 2023 at 8:09 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 07/10/2023 04:14, Sam Protsenko wrote:
> > On Fri, Oct 6, 2023 at 8:01 AM Mateusz Majewski <m.majewski2@samsung.com> wrote:
> >>
> >> The object of this work is fixing the following warning, which appears
> >> on all targets using that driver:
> >>
> >> gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation.
> >>
> >> This needs a small refactor to how we interact with the pinctrl
> >> subsystem. Finally, we remove some bookkeeping that has only been
> >> necessary to allocate GPIO bases correctly.
> >>
> >> Mateusz Majewski (4):
> >>   pinctrl: samsung: defer pinctrl_enable
> >>   pinctrl: samsung: use add_pin_ranges method to add pinctrl ranges
> >>   pinctrl: samsung: choose GPIO numberspace base dynamically
> >>   pinctrl: samsung: do not offset pinctrl numberspaces
> >>
> >>  drivers/pinctrl/samsung/pinctrl-samsung.c | 56 ++++++++++++-----------
> >>  drivers/pinctrl/samsung/pinctrl-samsung.h |  4 +-
> >>  2 files changed, 31 insertions(+), 29 deletions(-)
> >>
> >> --
> >
> > Hi Mateusz,
> >
> > Thank you for handling this! Those deprecation warnings have been
> > bugging me for some time :) While testing this series on my E850-96
> > board (Exynos850 based), I noticed some changes in
> > /sys/kernel/debug/gpio file, like these:
> >
> > 8<------------------------------------------------------------------------------------------>8
> > -gpiochip0: GPIOs 0-7, parent: platform/11850000.pinctrl, gpa0:
> > - gpio-7   (                    |Volume Up           ) in  hi IRQ ACTIVE LOW
> > +gpiochip0: GPIOs 512-519, parent: platform/11850000.pinctrl, gpa0:
> > + gpio-519 (                    |Volume Up           ) in  hi IRQ ACTIVE LOW
> >
> > -gpiochip1: GPIOs 8-15, parent: platform/11850000.pinctrl, gpa1:
> > - gpio-8   (                    |Volume Down         ) in  hi IRQ ACTIVE LOW
> > +gpiochip1: GPIOs 520-527, parent: platform/11850000.pinctrl, gpa1:
> > + gpio-520 (                    |Volume Down         ) in  hi IRQ ACTIVE LOW
> >
> > -gpiochip2: GPIOs 16-23, parent: platform/11850000.pinctrl, gpa2:
> > +gpiochip2: GPIOs 528-535, parent: platform/11850000.pinctrl, gpa2:
> >
> > ...
> > 8<------------------------------------------------------------------------------------------>8
> >
> > So basically it looks like all line numbers were offset by 512. Can
> > you please comment on this? Is it an intentional change, and why it's
> > happening?
> >
> > Despite of that change, everything seems to be working fine. But I
> > kinda liked the numeration starting from 0 better :)
>
> Could it be the reason of dynamic allocation?
>

I just asked because I didn't know :) But ok, if you want me to do
some digging... It seems like having GPIO_DYNAMIC_BASE=512 is not
necessarily the reason of dynamic allocation, but instead just a way
to keep 0-512 range for legacy GPIO drivers which might use that area
to allocate GPIO numbers statically. It's mentioned here:

    /*
     * At the end we want all GPIOs to be dynamically allocated from 0.
     * However, some legacy drivers still perform fixed allocation.
     * Until they are all fixed, leave 0-512 space for them.
     */
    #define GPIO_DYNAMIC_BASE    512

As mentioned in another comment in gpiochip_add_data_with_key(), that
numberspace shouldn't matter and in the end should go away, as GPIO
sysfs interface is pretty much deprecated at this point, and everybody
should stick to GPIO descriptors.

Anyway, now that it's clear that the base number change was intended
and shouldn't matter, for all patches in the series:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Tested-by: Sam Protsenko <semen.protsenko@linaro.org>

Thanks!

>
> Best regards,
> Krzysztof
>

Re: [PATCH 0/4] Fix Samsung pinctrl driver static allocation of GPIO base warning

[Krzysztof Kozlowski]

On 08/10/2023 20:45, Sam Protsenko wrote:

>>>
>>> Thank you for handling this! Those deprecation warnings have been
>>> bugging me for some time :) While testing this series on my E850-96
>>> board (Exynos850 based), I noticed some changes in
>>> /sys/kernel/debug/gpio file, like these:
>>>
>>> 8<------------------------------------------------------------------------------------------>8
>>> -gpiochip0: GPIOs 0-7, parent: platform/11850000.pinctrl, gpa0:
>>> - gpio-7   (                    |Volume Up           ) in  hi IRQ ACTIVE LOW
>>> +gpiochip0: GPIOs 512-519, parent: platform/11850000.pinctrl, gpa0:
>>> + gpio-519 (                    |Volume Up           ) in  hi IRQ ACTIVE LOW
>>>
>>> -gpiochip1: GPIOs 8-15, parent: platform/11850000.pinctrl, gpa1:
>>> - gpio-8   (                    |Volume Down         ) in  hi IRQ ACTIVE LOW
>>> +gpiochip1: GPIOs 520-527, parent: platform/11850000.pinctrl, gpa1:
>>> + gpio-520 (                    |Volume Down         ) in  hi IRQ ACTIVE LOW
>>>
>>> -gpiochip2: GPIOs 16-23, parent: platform/11850000.pinctrl, gpa2:
>>> +gpiochip2: GPIOs 528-535, parent: platform/11850000.pinctrl, gpa2:
>>>
>>> ...
>>> 8<------------------------------------------------------------------------------------------>8
>>>
>>> So basically it looks like all line numbers were offset by 512. Can
>>> you please comment on this? Is it an intentional change, and why it's
>>> happening?
>>>
>>> Despite of that change, everything seems to be working fine. But I
>>> kinda liked the numeration starting from 0 better :)
>>
>> Could it be the reason of dynamic allocation?
>>
> 
> I just asked because I didn't know :) But ok, if you want me to do
> some digging... It seems like having GPIO_DYNAMIC_BASE=512 is not
> necessarily the reason of dynamic allocation, but instead just a way
> to keep 0-512 range for legacy GPIO drivers which might use that area
> to allocate GPIO numbers statically. It's mentioned here:
> 
>     /*
>      * At the end we want all GPIOs to be dynamically allocated from 0.
>      * However, some legacy drivers still perform fixed allocation.
>      * Until they are all fixed, leave 0-512 space for them.
>      */
>     #define GPIO_DYNAMIC_BASE    512
> 
> As mentioned in another comment in gpiochip_add_data_with_key(), that
> numberspace shouldn't matter and in the end should go away, as GPIO
> sysfs interface is pretty much deprecated at this point, and everybody
> should stick to GPIO descriptors.
> 
> Anyway, now that it's clear that the base number change was intended
> and shouldn't matter, for all patches in the series:
> 
> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
> Tested-by: Sam Protsenko <semen.protsenko@linaro.org>

If all the GPIOs changed due to switch to dynamic allocation, aren't we
breaking all user-space users?

Best regards,
Krzysztof

Re: [PATCH 0/4] Fix Samsung pinctrl driver static allocation of GPIO base warning

[Marek Szyprowski]

On 09.10.2023 11:42, Krzysztof Kozlowski wrote:
> On 08/10/2023 20:45, Sam Protsenko wrote:
>>>> Thank you for handling this! Those deprecation warnings have been
>>>> bugging me for some time :) While testing this series on my E850-96
>>>> board (Exynos850 based), I noticed some changes in
>>>> /sys/kernel/debug/gpio file, like these:
>>>>
>>>> 8<------------------------------------------------------------------------------------------>8
>>>> -gpiochip0: GPIOs 0-7, parent: platform/11850000.pinctrl, gpa0:
>>>> - gpio-7   (                    |Volume Up           ) in  hi IRQ ACTIVE LOW
>>>> +gpiochip0: GPIOs 512-519, parent: platform/11850000.pinctrl, gpa0:
>>>> + gpio-519 (                    |Volume Up           ) in  hi IRQ ACTIVE LOW
>>>>
>>>> -gpiochip1: GPIOs 8-15, parent: platform/11850000.pinctrl, gpa1:
>>>> - gpio-8   (                    |Volume Down         ) in  hi IRQ ACTIVE LOW
>>>> +gpiochip1: GPIOs 520-527, parent: platform/11850000.pinctrl, gpa1:
>>>> + gpio-520 (                    |Volume Down         ) in  hi IRQ ACTIVE LOW
>>>>
>>>> -gpiochip2: GPIOs 16-23, parent: platform/11850000.pinctrl, gpa2:
>>>> +gpiochip2: GPIOs 528-535, parent: platform/11850000.pinctrl, gpa2:
>>>>
>>>> ...
>>>> 8<------------------------------------------------------------------------------------------>8
>>>>
>>>> So basically it looks like all line numbers were offset by 512. Can
>>>> you please comment on this? Is it an intentional change, and why it's
>>>> happening?
>>>>
>>>> Despite of that change, everything seems to be working fine. But I
>>>> kinda liked the numeration starting from 0 better :)
>>> Could it be the reason of dynamic allocation?
>>>
>> I just asked because I didn't know :) But ok, if you want me to do
>> some digging... It seems like having GPIO_DYNAMIC_BASE=512 is not
>> necessarily the reason of dynamic allocation, but instead just a way
>> to keep 0-512 range for legacy GPIO drivers which might use that area
>> to allocate GPIO numbers statically. It's mentioned here:
>>
>>      /*
>>       * At the end we want all GPIOs to be dynamically allocated from 0.
>>       * However, some legacy drivers still perform fixed allocation.
>>       * Until they are all fixed, leave 0-512 space for them.
>>       */
>>      #define GPIO_DYNAMIC_BASE    512
>>
>> As mentioned in another comment in gpiochip_add_data_with_key(), that
>> numberspace shouldn't matter and in the end should go away, as GPIO
>> sysfs interface is pretty much deprecated at this point, and everybody
>> should stick to GPIO descriptors.
>>
>> Anyway, now that it's clear that the base number change was intended
>> and shouldn't matter, for all patches in the series:
>>
>> Reviewed-by: Sam Protsenko<semen.protsenko@linaro.org>
>> Tested-by: Sam Protsenko<semen.protsenko@linaro.org>
> If all the GPIOs changed due to switch to dynamic allocation, aren't we
> breaking all user-space users?

This /sys based GPIO interface is deprecated, so I don't think that 
stable numbers is something that we should care.

Userspace, if still uses /sys interface, should depend on the GPIO bank 
name. I remember that the GPIO numbers varied between different kernel 
versions (also compared to the 'vendor kernels'), although I don't 
remember if this was Exynos related case or other.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 0/4] Fix Samsung pinctrl driver static allocation of GPIO base warning

[Marek Szyprowski]

On 06.10.2023 14:55, Mateusz Majewski wrote:
> The object of this work is fixing the following warning, which appears
> on all targets using that driver:
>
> gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation.
>
> This needs a small refactor to how we interact with the pinctrl
> subsystem. Finally, we remove some bookkeeping that has only been
> necessary to allocate GPIO bases correctly.
>
> Mateusz Majewski (4):
>    pinctrl: samsung: defer pinctrl_enable
>    pinctrl: samsung: use add_pin_ranges method to add pinctrl ranges
>    pinctrl: samsung: choose GPIO numberspace base dynamically
>    pinctrl: samsung: do not offset pinctrl numberspaces
>
>   drivers/pinctrl/samsung/pinctrl-samsung.c | 56 ++++++++++++-----------
>   drivers/pinctrl/samsung/pinctrl-samsung.h |  4 +-
>   2 files changed, 31 insertions(+), 29 deletions(-)

Just to let everyone know - I've tested this patchset on our test farm 
and found no regressions on various Exynos based boards.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 0/4] Fix Samsung pinctrl driver static allocation of GPIO base warning

[Krzysztof Kozlowski]

On Fri, 06 Oct 2023 14:55:53 +0200, Mateusz Majewski wrote:
> The object of this work is fixing the following warning, which appears
> on all targets using that driver:
> 
> gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation.
> 
> This needs a small refactor to how we interact with the pinctrl
> subsystem. Finally, we remove some bookkeeping that has only been
> necessary to allocate GPIO bases correctly.
> 
> [...]

Applied, thanks!

[1/4] pinctrl: samsung: defer pinctrl_enable
      https://git.kernel.org/pinctrl/samsung/c/2aca5c591ef4ecc4bcb9be3c9a9360d3d5238866
[2/4] pinctrl: samsung: use add_pin_ranges method to add pinctrl ranges
      https://git.kernel.org/pinctrl/samsung/c/bf128c1f0fe1fd4801fb84660c324095990c533a
[3/4] pinctrl: samsung: choose GPIO numberspace base dynamically
      https://git.kernel.org/pinctrl/samsung/c/deb79167e1dadc0ac0a9e3aa67130e60c5d011ef
[4/4] pinctrl: samsung: do not offset pinctrl numberspaces
      https://git.kernel.org/pinctrl/samsung/c/8aec97decfd0f444a69a765b2f00d64b42752824

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Re: [PATCH 0/4] Fix Samsung pinctrl driver static allocation of GPIO base warning

[Linus Walleij]

Hi Mateusz,

On Fri, Oct 6, 2023 at 3:00 PM Mateusz Majewski <m.majewski2@samsung.com> wrote:

> The object of this work is fixing the following warning, which appears
> on all targets using that driver:
>
> gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation.
>
> This needs a small refactor to how we interact with the pinctrl
> subsystem. Finally, we remove some bookkeeping that has only been
> necessary to allocate GPIO bases correctly.

I see that Krzysztof has already taken care of this series so I just
wait for a pull request (some days work is a bliss, thanks Krzysztof!)

Yours,
Linus Walleij