[PATCH 0/3] gpio: brcmstb: add support for gpio-ranges

[PATCH 0/3] gpio: brcmstb: add support for gpio-ranges

[Doug Berger]

The Raspberry Pi 5 includes Broadcom STB GPIO IP as well as
Broadcom 2712 pin controller IP.

The community has expressed interest in linking the two drivers
with the "gpio-ranges" property in device tree. This commit
stack implements the necessary changes.

Doug Berger (3):
  dt-bindings: gpio: brcmstb: add gpio-ranges
  gpio: of: support gpio-ranges for multiple gpiochip devices
  gpio: brcmstb: add support for gpio-ranges

 .../bindings/gpio/brcm,brcmstb-gpio.yaml      |  3 +++
 drivers/gpio/gpio-brcmstb.c                   |  2 ++
 drivers/gpio/gpiolib-of.c                     | 23 +++++++++++++++++--
 3 files changed, 26 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/3] dt-bindings: gpio: brcmstb: add gpio-ranges

[Doug Berger]

Add optional gpio-ranges device-tree property to the Broadcom
Set-Top-Box GPIO controller.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.yaml b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.yaml
index a1e71c974e79..f096f286da19 100644
--- a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.yaml
+++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.yaml
@@ -62,6 +62,8 @@ properties:
 
   interrupt-controller: true
 
+  gpio-ranges: true
+
   wakeup-source:
     type: boolean
     description: >
@@ -88,6 +90,7 @@ examples:
         interrupt-parent = <&irq0_intc>;
         interrupts = <0x6>;
         brcm,gpio-bank-widths = <32 32 32 24>;
+        gpio-ranges = <&pinctrl 0 0 120>;
     };
 
     upg_gio_aon: gpio@f04172c0 {
-- 
2.34.1

Re: [PATCH 1/3] dt-bindings: gpio: brcmstb: add gpio-ranges

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 257 bytes --]

On 4/24/24 11:50, Doug Berger wrote:
> Add optional gpio-ranges device-tree property to the Broadcom
> Set-Top-Box GPIO controller.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Acked-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH 1/3] dt-bindings: gpio: brcmstb: add gpio-ranges

[Krzysztof Kozlowski]

On 24/04/2024 20:50, Doug Berger wrote:
> Add optional gpio-ranges device-tree property to the Broadcom
> Set-Top-Box GPIO controller.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> ---

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: gpio: brcmstb: add gpio-ranges

[Linus Walleij]

On Wed, Apr 24, 2024 at 8:51 PM Doug Berger <opendmb@gmail.com> wrote:

> Add optional gpio-ranges device-tree property to the Broadcom
> Set-Top-Box GPIO controller.
>
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 2/3] gpio: of: support gpio-ranges for multiple gpiochip devices

[Doug Berger]

Some drivers (e.g. gpio-mt7621 and gpio-brcmstb) have multiple
gpiochip banks within a single device. Unfortunately, the
gpio-ranges property of the device node was being applied to
every gpiochip of the device with device relative GPIO offset
values rather than gpiochip relative GPIO offset values.

This commit makes use of the gpio_chip offset value which can be
non-zero for such devices to split the device node gpio-ranges
property into GPIO offset ranges that can be applied to each
of the relevant gpiochips of the device.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/gpio/gpiolib-of.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index cb0cefaec37e..d75f6ee37028 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -1037,7 +1037,7 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
 	struct of_phandle_args pinspec;
 	struct pinctrl_dev *pctldev;
 	struct device_node *np;
-	int index = 0, ret;
+	int index = 0, ret, trim;
 	const char *name;
 	static const char group_names_propname[] = "gpio-ranges-group-names";
 	struct property *group_names;
@@ -1059,7 +1059,14 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
 		if (!pctldev)
 			return -EPROBE_DEFER;
 
+		/* Ignore ranges outside of this GPIO chip */
+		if (pinspec.args[0] >= (chip->offset + chip->ngpio))
+			continue;
+		if (pinspec.args[0] + pinspec.args[2] <= chip->offset)
+			continue;
+
 		if (pinspec.args[2]) {
+			/* npins != 0: linear range */
 			if (group_names) {
 				of_property_read_string_index(np,
 						group_names_propname,
@@ -1070,7 +1077,19 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
 					break;
 				}
 			}
-			/* npins != 0: linear range */
+
+			/* Trim the range to fit this GPIO chip */
+			if (chip->offset > pinspec.args[0]) {
+				trim = chip->offset - pinspec.args[0];
+				pinspec.args[2] -= trim;
+				pinspec.args[1] += trim;
+				pinspec.args[0] = 0;
+			} else {
+				pinspec.args[0] -= chip->offset;
+			}
+			if ((pinspec.args[0] + pinspec.args[2]) > chip->ngpio)
+				pinspec.args[2] = chip->ngpio - pinspec.args[0];
+
 			ret = gpiochip_add_pin_range(chip,
 					pinctrl_dev_get_devname(pctldev),
 					pinspec.args[0],
-- 
2.34.1

Re: [PATCH 2/3] gpio: of: support gpio-ranges for multiple gpiochip devices

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

On 4/24/24 11:50, Doug Berger wrote:
> Some drivers (e.g. gpio-mt7621 and gpio-brcmstb) have multiple
> gpiochip banks within a single device. Unfortunately, the
> gpio-ranges property of the device node was being applied to
> every gpiochip of the device with device relative GPIO offset
> values rather than gpiochip relative GPIO offset values.
> 
> This commit makes use of the gpio_chip offset value which can be
> non-zero for such devices to split the device node gpio-ranges
> property into GPIO offset ranges that can be applied to each
> of the relevant gpiochips of the device.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Acked-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH 2/3] gpio: of: support gpio-ranges for multiple gpiochip devices

[Bartosz Golaszewski]

On Wed, Apr 24, 2024 at 8:51 PM Doug Berger <opendmb@gmail.com> wrote:
>
> Some drivers (e.g. gpio-mt7621 and gpio-brcmstb) have multiple
> gpiochip banks within a single device. Unfortunately, the
> gpio-ranges property of the device node was being applied to
> every gpiochip of the device with device relative GPIO offset
> values rather than gpiochip relative GPIO offset values.
>
> This commit makes use of the gpio_chip offset value which can be
> non-zero for such devices to split the device node gpio-ranges
> property into GPIO offset ranges that can be applied to each
> of the relevant gpiochips of the device.
>
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> ---

This is a good improvement, thanks!

Bart

Re: [PATCH 2/3] gpio: of: support gpio-ranges for multiple gpiochip devices

[Linus Walleij]

Hi Dough,

thanks for your patch!

I'm a bit confused here:

On Wed, Apr 24, 2024 at 8:51 PM Doug Berger <opendmb@gmail.com> wrote:


> +               /* Ignore ranges outside of this GPIO chip */
> +               if (pinspec.args[0] >= (chip->offset + chip->ngpio))
> +                       continue;
> +               if (pinspec.args[0] + pinspec.args[2] <= chip->offset)
> +                       continue;

Here pinspec.args[0] and [2] comes directly from the device tree.

The documentation in Documentation/devicetree/bindings/gpio/gpio.txt
says:

> 2.2) Ordinary (numerical) GPIO ranges
> -------------------------------------
>
> It is useful to represent which GPIOs correspond to which pins on which pin
> controllers. The gpio-ranges property described below represents this with
> a discrete set of ranges mapping pins from the pin controller local number space
> to pins in the GPIO controller local number space.
>
> The format is: <[pin controller phandle], [GPIO controller offset],
>                 [pin controller offset], [number of pins]>;
>
> The GPIO controller offset pertains to the GPIO controller node containing the
> range definition.

So I do not understand how pinspec[0] and [2] can ever be compared
to something involving chip->offset which is a Linux-specific offset.

It rather looks like you are trying to accomodate the Linux numberspace
in the ranges, which it was explicitly designed to avoid.

I just don't get it.

So NACK until I understand what is going on here.

Yours,
Linus Walleij

Re: [PATCH 2/3] gpio: of: support gpio-ranges for multiple gpiochip devices

[Doug Berger]

On 5/3/2024 1:25 AM, Linus Walleij wrote:
> Hi Dough,
> 
> thanks for your patch!
Thanks for your review!

> 
> I'm a bit confused here:
"Communication is hard" and I may be confused about your confusion, but 
hopefully we can work it out.

> 
> On Wed, Apr 24, 2024 at 8:51 PM Doug Berger <opendmb@gmail.com> wrote:
> 
> 
>> +               /* Ignore ranges outside of this GPIO chip */
>> +               if (pinspec.args[0] >= (chip->offset + chip->ngpio))
>> +                       continue;
>> +               if (pinspec.args[0] + pinspec.args[2] <= chip->offset)
>> +                       continue;
> 
> Here pinspec.args[0] and [2] comes directly from the device tree.
> 
> The documentation in Documentation/devicetree/bindings/gpio/gpio.txt
> says:
> 
>> 2.2) Ordinary (numerical) GPIO ranges
>> -------------------------------------
>>
>> It is useful to represent which GPIOs correspond to which pins on which pin
>> controllers. The gpio-ranges property described below represents this with
>> a discrete set of ranges mapping pins from the pin controller local number space
>> to pins in the GPIO controller local number space.
>>
>> The format is: <[pin controller phandle], [GPIO controller offset],
>>                  [pin controller offset], [number of pins]>;
>>
>> The GPIO controller offset pertains to the GPIO controller node containing the
>> range definition.
I think we are in agreement here. For extra clarity, I will add that in 
my understanding pinspec.args[0] corresponds to [GPIO controller offset] 
and pinspec.args[2] corresponds to [number of pins].

> 
> So I do not understand how pinspec[0] and [2] can ever be compared
> to something involving chip->offset which is a Linux-specific offset.
> 
> It rather looks like you are trying to accomodate the Linux numberspace
> in the ranges, which it was explicitly designed to avoid.
The struct gpio_chip documentation in include/linux/gpio/driver.h says:

 > * @offset: when multiple gpio chips belong to the same device this
 > *	can be used as offset within the device so friendly names can
 > *	be properly assigned.

It is my understanding that this value represents the offset of a 
gpiochip relative to the GPIO controller device defined by the GPIO 
controller node in device tree. This puts it in the same number space as 
[GPIO controller offset]. I believe it was introduced for the specific 
purpose of translating [GPIO controller offset] values into 
Linux-specific offsets, which is why it is being reused for that purpose 
in this patch.

For GPIO Controllers that contain a single gpiochip the 'offset' member 
is 0 and the device tree node offsets can be applied directly to the 
gpiochip. However, when a GPIO Controller contains multiple gpiochips, 
the device tree node offsets must be translated to each individual gpiochip.

> 
> I just don't get it.
> 
> So NACK until I understand what is going on here.
> 
> Yours,
> Linus Walleij
I hope it makes sense now, but if not please help me understand what I 
may be missing.

Thanks,
     Doug

Re: [PATCH 2/3] gpio: of: support gpio-ranges for multiple gpiochip devices

[Bartosz Golaszewski]

On Fri, May 3, 2024 at 10:21 PM Doug Berger <opendmb@gmail.com> wrote:
>
> On 5/3/2024 1:25 AM, Linus Walleij wrote:
> > Hi Dough,
> >
> > thanks for your patch!
> Thanks for your review!
>
> >
> > I'm a bit confused here:
> "Communication is hard" and I may be confused about your confusion, but
> hopefully we can work it out.
>
> >
> > On Wed, Apr 24, 2024 at 8:51 PM Doug Berger <opendmb@gmail.com> wrote:
> >
> >
> >> +               /* Ignore ranges outside of this GPIO chip */
> >> +               if (pinspec.args[0] >= (chip->offset + chip->ngpio))
> >> +                       continue;
> >> +               if (pinspec.args[0] + pinspec.args[2] <= chip->offset)
> >> +                       continue;
> >
> > Here pinspec.args[0] and [2] comes directly from the device tree.
> >
> > The documentation in Documentation/devicetree/bindings/gpio/gpio.txt
> > says:
> >
> >> 2.2) Ordinary (numerical) GPIO ranges
> >> -------------------------------------
> >>
> >> It is useful to represent which GPIOs correspond to which pins on which pin
> >> controllers. The gpio-ranges property described below represents this with
> >> a discrete set of ranges mapping pins from the pin controller local number space
> >> to pins in the GPIO controller local number space.
> >>
> >> The format is: <[pin controller phandle], [GPIO controller offset],
> >>                  [pin controller offset], [number of pins]>;
> >>
> >> The GPIO controller offset pertains to the GPIO controller node containing the
> >> range definition.
> I think we are in agreement here. For extra clarity, I will add that in
> my understanding pinspec.args[0] corresponds to [GPIO controller offset]
> and pinspec.args[2] corresponds to [number of pins].
>
> >
> > So I do not understand how pinspec[0] and [2] can ever be compared
> > to something involving chip->offset which is a Linux-specific offset.
> >
> > It rather looks like you are trying to accomodate the Linux numberspace
> > in the ranges, which it was explicitly designed to avoid.
> The struct gpio_chip documentation in include/linux/gpio/driver.h says:
>
>  > * @offset: when multiple gpio chips belong to the same device this
>  > *    can be used as offset within the device so friendly names can
>  > *    be properly assigned.
>
> It is my understanding that this value represents the offset of a
> gpiochip relative to the GPIO controller device defined by the GPIO
> controller node in device tree. This puts it in the same number space as
> [GPIO controller offset]. I believe it was introduced for the specific
> purpose of translating [GPIO controller offset] values into
> Linux-specific offsets, which is why it is being reused for that purpose
> in this patch.
>
> For GPIO Controllers that contain a single gpiochip the 'offset' member
> is 0 and the device tree node offsets can be applied directly to the
> gpiochip. However, when a GPIO Controller contains multiple gpiochips,
> the device tree node offsets must be translated to each individual gpiochip.
>
> >
> > I just don't get it.
> >
> > So NACK until I understand what is going on here.
> >
> > Yours,
> > Linus Walleij
> I hope it makes sense now, but if not please help me understand what I
> may be missing.
>
> Thanks,
>      Doug
>

Linus,

Please let me know if this is still a NAK, if so, I'll drop this
series from my tree at least for this release.

Bart

Re: [PATCH 2/3] gpio: of: support gpio-ranges for multiple gpiochip devices

[Linus Walleij]

On Sun, May 5, 2024 at 2:25 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Please let me know if this is still a NAK, if so, I'll drop this
> series from my tree at least for this release.

No I was just wrong, I had missed Sergio's addition of local
offset for several-gpiochip-per-device handling completely
and confused .offset with .base...

Yours,
Linus Walleij

Re: [PATCH 2/3] gpio: of: support gpio-ranges for multiple gpiochip devices

[Linus Walleij]

On Fri, May 3, 2024 at 10:21 PM Doug Berger <opendmb@gmail.com> wrote:
> On 5/3/2024 1:25 AM, Linus Walleij wrote:

> > It rather looks like you are trying to accomodate the Linux numberspace
> > in the ranges, which it was explicitly designed to avoid.

> The struct gpio_chip documentation in include/linux/gpio/driver.h says:
>
>  > * @offset: when multiple gpio chips belong to the same device this
>  > *    can be used as offset within the device so friendly names can
>  > *    be properly assigned.
>
> It is my understanding that this value represents the offset of a
> gpiochip relative to the GPIO controller device defined by the GPIO
> controller node in device tree. This puts it in the same number space as
> [GPIO controller offset]. I believe it was introduced for the specific
> purpose of translating [GPIO controller offset] values into
> Linux-specific offsets, which is why it is being reused for that purpose
> in this patch.

You're right, I had it confused with .base!

It's because I missed it completely when this new property was added
in 2021.

Sorry for the fuzz.

Yours,
Linus Walleij

[PATCH 3/3] gpio: brcmstb: add support for gpio-ranges

[Doug Berger]

A pin controller device mapped with the gpio-ranges property
will need implementations of the .request and .free members of
the gpiochip.

Signed-off-by: Doug Berger <opendmb@gmail.com>
Tested-by: Phil Elwell <phil@raspberrypi.com>
---
 drivers/gpio/gpio-brcmstb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index 790cb278b72a..8dce78ea7139 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -694,6 +694,8 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		/* not all ngpio lines are valid, will use bank width later */
 		gc->ngpio = MAX_GPIO_PER_BANK;
 		gc->offset = bank->id * MAX_GPIO_PER_BANK;
+		gc->request = gpiochip_generic_request;
+		gc->free = gpiochip_generic_free;
 		if (priv->parent_irq > 0)
 			gc->to_irq = brcmstb_gpio_to_irq;
 
-- 
2.34.1

Re: [PATCH 3/3] gpio: brcmstb: add support for gpio-ranges

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

On 4/24/24 11:50, Doug Berger wrote:
> A pin controller device mapped with the gpio-ranges property
> will need implementations of the .request and .free members of
> the gpiochip.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> Tested-by: Phil Elwell <phil@raspberrypi.com>

Acked-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH 0/3] gpio: brcmstb: add support for gpio-ranges

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Wed, 24 Apr 2024 11:50:36 -0700, Doug Berger wrote:
> The Raspberry Pi 5 includes Broadcom STB GPIO IP as well as
> Broadcom 2712 pin controller IP.
> 
> The community has expressed interest in linking the two drivers
> with the "gpio-ranges" property in device tree. This commit
> stack implements the necessary changes.
> 
> [...]

Applied, thanks!

[1/3] dt-bindings: gpio: brcmstb: add gpio-ranges
      commit: 7c66f8173360556ac0c3c38a91234af5a0a5a4a9
[2/3] gpio: of: support gpio-ranges for multiple gpiochip devices
      commit: e818cd3c8a345c046edff00b5ad0be4d39f7e4d4
[3/3] gpio: brcmstb: add support for gpio-ranges
      commit: 5539287ca65686f478e058a1e939294cb5682426

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH 0/3] gpio: brcmstb: add support for gpio-ranges

[Linus Walleij]

On Fri, Apr 26, 2024 at 9:33 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> [2/3] gpio: of: support gpio-ranges for multiple gpiochip devices
>       commit: e818cd3c8a345c046edff00b5ad0be4d39f7e4d4

I'm not sure this is a good idea. To me it looks like the commit violates
the device tree bindings, which says offsets are on the local GPIO chip
and turn them into offsets in the Linux GPIO numberspace.

Yours,
Linus Walleij