| Message ID | c399da9deab3ede9b0c4d4680d8ac508707aa8c3.1644903104.git.lukas@wunner.de |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: bcm2835: Use bcm2835 gpio_chip label for bcm2711 | expand |
Hi Lukas, Am 15.02.22 um 06:52 schrieb Lukas Wunner: > Commit b1d84a3d0a26 ("pinctrl: bcm2835: Add support for all GPIOs on > BCM2711") used a different label for the bcm2711 gpio_chip vis-à-vis > the bcm2835. > > That breaks compatibility for GPIO_LOOKUP_IDX() and GPIO_HOG() clauses > when porting from older Raspberry Pi Compute Modules to the CM4 or CM4S. i've some questions: could you explain the breakage more in detail, is it kernel or user space? A little bit off topic, but what is this CM4S? Is it special version of the CM4? Can you provide a link or something? > > The name change seems unwarranted given it's essentially the same > hardware, so use the old name instead. I disagree at this point. The pinctrl of bcm2835 and bcm2711 are different. For example the bcm2835 has only 54 GPIOs while the bcm2711 has 58. Best regards > > For consistency, modify the pinctrl_desc and pinctrl_gpio_range names > as well. (It looks like they're only used by debugfs.) > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: Stefan Wahren <stefan.wahren@i2se.com> > --- > drivers/pinctrl/bcm/pinctrl-bcm2835.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c > index 47e433e09c5c..41d0f32b9d66 100644 > --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c > +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c > @@ -375,7 +375,7 @@ static const struct gpio_chip bcm2835_gpio_chip = { > }; > > static const struct gpio_chip bcm2711_gpio_chip = { > - .label = "pinctrl-bcm2711", > + .label = MODULE_NAME, > .owner = THIS_MODULE, > .request = gpiochip_generic_request, > .free = gpiochip_generic_free, > @@ -1134,7 +1134,7 @@ static const struct pinctrl_desc bcm2835_pinctrl_desc = { > }; > > static const struct pinctrl_desc bcm2711_pinctrl_desc = { > - .name = "pinctrl-bcm2711", > + .name = MODULE_NAME, > .pins = bcm2835_gpio_pins, > .npins = BCM2711_NUM_GPIOS, > .pctlops = &bcm2835_pctl_ops, > @@ -1149,7 +1149,7 @@ static const struct pinctrl_gpio_range bcm2835_pinctrl_gpio_range = { > }; > > static const struct pinctrl_gpio_range bcm2711_pinctrl_gpio_range = { > - .name = "pinctrl-bcm2711", > + .name = MODULE_NAME, > .npins = BCM2711_NUM_GPIOS, > }; >
On Tue, Feb 15, 2022 at 01:00:47PM +0100, Stefan Wahren wrote: > Am 15.02.22 um 06:52 schrieb Lukas Wunner: > > Commit b1d84a3d0a26 ("pinctrl: bcm2835: Add support for all GPIOs on > > BCM2711") used a different label for the bcm2711 gpio_chip vis-à-vis > > the bcm2835. > > > > That breaks compatibility for GPIO_LOOKUP_IDX() and GPIO_HOG() clauses > > when porting from older Raspberry Pi Compute Modules to the CM4 or CM4S. > > could you explain the breakage more in detail, is it kernel or user space? This kernel module (which is sought to be upstreamed mid-term) requests GPIOs at runtime for a chardev: https://github.com/RevolutionPi/piControl/blob/master/revpi_core.c#L50 That fails on BCM2711 because a different label name was used, even though the pin-controller is otherwise compatible to BCM2835. > A little bit off topic, but what is this CM4S? Is it special version of > the CM4? Can you provide a link or something? BCM2711 in a CM1/CM3-compatible form factor. There is no public documentation at this point besides the device-tree overlay and what's being discussed in the forums and on GitHub: https://github.com/raspberrypi/linux/blob/rpi-5.15.y/arch/arm/boot/dts/bcm2711-rpi-cm4s.dts https://forums.raspberrypi.com/viewtopic.php?t=325975 https://github.com/search?q=cm4s&type=commits > > The name change seems unwarranted given it's essentially the same > > hardware, so use the old name instead. > > I disagree at this point. The pinctrl of bcm2835 and bcm2711 are > different. For example the bcm2835 has only 54 GPIOs while the bcm2711 > has 58. Four additional GPIOs don't justify a different label name given the pin-controller otherwise behaves the same. We also had minimal differences in pin assignment on BCM2835/6/7 and that didn't justify a different label name either. Thanks, Lukas
Hi, Am 15.02.22 um 15:44 schrieb Lukas Wunner: > On Tue, Feb 15, 2022 at 01:00:47PM +0100, Stefan Wahren wrote: >> Am 15.02.22 um 06:52 schrieb Lukas Wunner: >>> Commit b1d84a3d0a26 ("pinctrl: bcm2835: Add support for all GPIOs on >>> BCM2711") used a different label for the bcm2711 gpio_chip vis-à-vis >>> the bcm2835. >>> >>> That breaks compatibility for GPIO_LOOKUP_IDX() and GPIO_HOG() clauses >>> when porting from older Raspberry Pi Compute Modules to the CM4 or CM4S. >> could you explain the breakage more in detail, is it kernel or user space? > This kernel module (which is sought to be upstreamed mid-term) > requests GPIOs at runtime for a chardev: > > https://github.com/RevolutionPi/piControl/blob/master/revpi_core.c#L50 > > That fails on BCM2711 because a different label name was used, > even though the pin-controller is otherwise compatible to BCM2835. sorry, but you cannot blame the mainline kernel for assumptions in out of tree drivers. What happens if another driver relies on the existing labeling? How about detecting the platform via devicetree functions in your driver? > > >> A little bit off topic, but what is this CM4S? Is it special version of >> the CM4? Can you provide a link or something? > BCM2711 in a CM1/CM3-compatible form factor. There is no public > documentation at this point besides the device-tree overlay and > what's being discussed in the forums and on GitHub: > > https://github.com/raspberrypi/linux/blob/rpi-5.15.y/arch/arm/boot/dts/bcm2711-rpi-cm4s.dts > https://forums.raspberrypi.com/viewtopic.php?t=325975 > https://github.com/search?q=cm4s&type=commits Thanks a lot. > >>> The name change seems unwarranted given it's essentially the same >>> hardware, so use the old name instead. >> I disagree at this point. The pinctrl of bcm2835 and bcm2711 are >> different. For example the bcm2835 has only 54 GPIOs while the bcm2711 >> has 58. > Four additional GPIOs don't justify a different label name given the > pin-controller otherwise behaves the same. We also had minimal > differences in pin assignment on BCM2835/6/7 and that didn't > justify a different label name either. No, the GPIO pins of BCM2835/6/7 SoC are always identical from driver point of view, because they use the same devicetree compatible. It's the Raspberry Pi board which connect the GPIOs differently, that was one of the reasons for introduction of GPIO line names via devicetree. I understand your pain, but i cannot give an Ack to this change. Best regards > > Thanks, > > Lukas
On 2/15/22 8:56 AM, Stefan Wahren wrote: > Hi, > > Am 15.02.22 um 15:44 schrieb Lukas Wunner: > >> On Tue, Feb 15, 2022 at 01:00:47PM +0100, Stefan Wahren wrote: >>> Am 15.02.22 um 06:52 schrieb Lukas Wunner: >>>> Commit b1d84a3d0a26 ("pinctrl: bcm2835: Add support for all GPIOs on >>>> BCM2711") used a different label for the bcm2711 gpio_chip vis-à-vis >>>> the bcm2835. >>>> >>>> That breaks compatibility for GPIO_LOOKUP_IDX() and GPIO_HOG() clauses >>>> when porting from older Raspberry Pi Compute Modules to the CM4 or CM4S. >>> could you explain the breakage more in detail, is it kernel or user space? >> This kernel module (which is sought to be upstreamed mid-term) >> requests GPIOs at runtime for a chardev: >> >> https://github.com/RevolutionPi/piControl/blob/master/revpi_core.c#L50 >> >> That fails on BCM2711 because a different label name was used, >> even though the pin-controller is otherwise compatible to BCM2835. > > sorry, but you cannot blame the mainline kernel for assumptions in out > of tree drivers. What happens if another driver relies on the existing > labeling? > > How about detecting the platform via devicetree functions in your driver? > >> >> >>> A little bit off topic, but what is this CM4S? Is it special version of >>> the CM4? Can you provide a link or something? >> BCM2711 in a CM1/CM3-compatible form factor. There is no public >> documentation at this point besides the device-tree overlay and >> what's being discussed in the forums and on GitHub: >> >> https://github.com/raspberrypi/linux/blob/rpi-5.15.y/arch/arm/boot/dts/bcm2711-rpi-cm4s.dts >> https://forums.raspberrypi.com/viewtopic.php?t=325975 >> https://github.com/search?q=cm4s&type=commits > Thanks a lot. >> >>>> The name change seems unwarranted given it's essentially the same >>>> hardware, so use the old name instead. >>> I disagree at this point. The pinctrl of bcm2835 and bcm2711 are >>> different. For example the bcm2835 has only 54 GPIOs while the bcm2711 >>> has 58. >> Four additional GPIOs don't justify a different label name given the >> pin-controller otherwise behaves the same. We also had minimal >> differences in pin assignment on BCM2835/6/7 and that didn't >> justify a different label name either. > > No, the GPIO pins of BCM2835/6/7 SoC are always identical from driver > point of view, because they use the same devicetree compatible. It's the > Raspberry Pi board which connect the GPIOs differently, that was one of > the reasons for introduction of GPIO line names via devicetree. > > I understand your pain, but i cannot give an Ack to this change. I agree with Stefan here, besides changing the driver name now would mean potentially breaking user-space since the driver name is visible in a variety of places. Seems to me like this is too late, we should have caught this during the introduction of 2711. -- Florian
On Tue, Feb 15, 2022 at 09:17:49AM -0800, Florian Fainelli wrote: > >>> Am 15.02.22 um 06:52 schrieb Lukas Wunner: > >>>> Commit b1d84a3d0a26 ("pinctrl: bcm2835: Add support for all GPIOs on > >>>> BCM2711") used a different label for the bcm2711 gpio_chip vis-à-vis > >>>> the bcm2835. [...] > I agree with Stefan here, besides changing the driver name now would > mean potentially breaking user-space since the driver name is visible in > a variety of places. Seems to me like this is too late, we should have > caught this during the introduction of 2711. This isn't about the driver name but the gpio_chip label. The .name attribute of bcm2711_pinctrl_desc and bcm2711_pinctrl_gpio_range is only visible in debugfs, which doesn't count as user-space ABI. The .label attribute of bcm2711_gpio_chip is indeed visible in sysfs and could in theory be used by udev rules, though I doubt it. It definitely was a mistake not to use the same label as pinctrl-bcm2835. Using a different label hinges on the notion that it's a different chip, and while that may apply for the 4B+ and CM4, the assumption falls apart with the CM4S which seeks to be a drop-in replacement for CM1/CM3, but really is not because of mistakes like this one. We're likely not the only ones bitten by this, just the first to report. Thanks, Lukas
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c index 47e433e09c5c..41d0f32b9d66 100644 --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c @@ -375,7 +375,7 @@ static const struct gpio_chip bcm2835_gpio_chip = { }; static const struct gpio_chip bcm2711_gpio_chip = { - .label = "pinctrl-bcm2711", + .label = MODULE_NAME, .owner = THIS_MODULE, .request = gpiochip_generic_request, .free = gpiochip_generic_free, @@ -1134,7 +1134,7 @@ static const struct pinctrl_desc bcm2835_pinctrl_desc = { }; static const struct pinctrl_desc bcm2711_pinctrl_desc = { - .name = "pinctrl-bcm2711", + .name = MODULE_NAME, .pins = bcm2835_gpio_pins, .npins = BCM2711_NUM_GPIOS, .pctlops = &bcm2835_pctl_ops, @@ -1149,7 +1149,7 @@ static const struct pinctrl_gpio_range bcm2835_pinctrl_gpio_range = { }; static const struct pinctrl_gpio_range bcm2711_pinctrl_gpio_range = { - .name = "pinctrl-bcm2711", + .name = MODULE_NAME, .npins = BCM2711_NUM_GPIOS, };
Commit b1d84a3d0a26 ("pinctrl: bcm2835: Add support for all GPIOs on BCM2711") used a different label for the bcm2711 gpio_chip vis-à-vis the bcm2835. That breaks compatibility for GPIO_LOOKUP_IDX() and GPIO_HOG() clauses when porting from older Raspberry Pi Compute Modules to the CM4 or CM4S. The name change seems unwarranted given it's essentially the same hardware, so use the old name instead. For consistency, modify the pinctrl_desc and pinctrl_gpio_range names as well. (It looks like they're only used by debugfs.) Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: Stefan Wahren <stefan.wahren@i2se.com> --- drivers/pinctrl/bcm/pinctrl-bcm2835.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)