| Message ID | 20240424185039.1707812-3-opendmb@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: brcmstb: add support for gpio-ranges | expand |
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>
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
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
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
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
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
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
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],
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(-)