| Message ID | 20241017-gpio-ngpios-v2-0-cecfdb38d40e@linaro.org |
|---|---|
| Headers | show |
| Series | gpio: mmio: Support ngpios property | expand |
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> On Thu, 17 Oct 2024 08:46:07 +0200, Linus Walleij wrote: > I thought this generic property was already supported by the > generic MMIO bindings and code, but no. > > It's a pretty obvious usecase to just use some from 0..n > of a MMIO GPIO bank, so add support for this. > > > [...] Applied, thanks! [1/2] dt-bindings: gpio-mmio: Add ngpios property commit: b4c69d471b72aa70766d94a11c31bc4c13f29eac [2/2] gpio: mmio: Parse ngpios property commit: 1ed9f099323ed366291291826a8beb1a5adfc2c8 Best regards,
On Thu, Oct 17, 2024 at 08:46:07AM +0200, Linus Walleij wrote: > I thought this generic property was already supported by the > generic MMIO bindings and code, but no. I have two issues with the second patch here. First one is why? What the *practical* issue you have? Can you elaborate on that? Second one, is there any other way to avoid duplication of the code so we have one place of the property parsing? For the background I have to mention this commit: 55b2395e4e92 ("gpio: mmio: handle "ngpios" properly in bgpio_init()") If there is an issue, it should be fixed rather than adding a shortcut with not fully known consequences. TL;DR: on a brief look the second patch should be reverted (or simply dropped as it's easy to do while it's on top of the branch. > It's a pretty obvious usecase to just use some from 0..n > of a MMIO GPIO bank, so add support for this.
On Fri, Oct 18, 2024 at 1:30 PM Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > First one is why? What the *practical* issue you have? Can you elaborate > on that? Sure, there are these hardwares that probe directly from the gpio-mmio driver: Documentation/devicetree/bindings/gpio/gpio-mmio.yaml properties: compatible: enum: - brcm,bcm6345-gpio - ni,169445-nand-gpio - wd,mbl-gpio # Western Digital MyBook Live memory-mapped GPIO controller The practical issue is (similar to what was responded to Rob in patch 2/2) that non-existing GPIOs will get exposed to userspace. For patch 1/2 (adding the DT binding) it would be that without ngpios we do not model the hardware properly. The objection "it makes no harm to register GPIO lines for all bits in the register" can likewise be raised to the other 28 (if I count correctly) GPIO drivers that use this property (git grep ngpios drivers/gpio) and I think the train left the station long ago to object to the property in general, people don't want to expose non-existing GPIOs to the GPIO framework. > Second one, is there any other way to avoid duplication of the code so > we have one place of the property parsing? > > For the background I have to mention this commit: > 55b2395e4e92 ("gpio: mmio: handle "ngpios" properly in bgpio_init()") Oh well spotted! I completely missed the fact that we already added ngpios parsing elsewhere in the driver. Bartosz, can you please drop patch 2/2? Patch 1/2 is needed however: it is just documenting the behaviour that is already implemented. Yours, Linus Walleij
On Fri, Oct 18, 2024 at 1:49 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > Oh well spotted! I completely missed the fact that we already > added ngpios parsing elsewhere in the driver. > > Bartosz, can you please drop patch 2/2? > > Patch 1/2 is needed however: it is just documenting the behaviour > that is already implemented. > Done. Bart
On Fri, Oct 18, 2024 at 01:49:40PM +0200, Linus Walleij wrote: > On Fri, Oct 18, 2024 at 1:30 PM Andy Shevchenko > <andriy.shevchenko@intel.com> wrote: > > > First one is why? What the *practical* issue you have? Can you elaborate > > on that? > > Sure, there are these hardwares that probe directly from the > gpio-mmio driver: > Documentation/devicetree/bindings/gpio/gpio-mmio.yaml > properties: > compatible: > enum: > - brcm,bcm6345-gpio > - ni,169445-nand-gpio > - wd,mbl-gpio # Western Digital MyBook Live memory-mapped GPIO controller > > The practical issue is (similar to what was responded to Rob > in patch 2/2) that non-existing GPIOs will get exposed to userspace. > > For patch 1/2 (adding the DT binding) it would be that without > ngpios we do not model the hardware properly. > > The objection "it makes no harm to register GPIO lines > for all bits in the register" can likewise be raised to the > other 28 (if I count correctly) GPIO drivers that use this > property (git grep ngpios drivers/gpio) and I think the train left the > station long ago to object to the property in general, people > don't want to expose non-existing GPIOs to the GPIO > framework. Sorry that I likely wasn't clear enough. My question was if you really experienced any bugs in practice. The above is the theory part and I completely agree with. > > Second one, is there any other way to avoid duplication of the code so > > we have one place of the property parsing? > > > > For the background I have to mention this commit: > > 55b2395e4e92 ("gpio: mmio: handle "ngpios" properly in bgpio_init()") > > Oh well spotted! I completely missed the fact that we already > added ngpios parsing elsewhere in the driver. > > Bartosz, can you please drop patch 2/2? > Patch 1/2 is needed however: it is just documenting the behaviour > that is already implemented. I'm not agianst this, the first patch is the correct advertisement. My questioning was related solely to the second patch in the series.
I thought this generic property was already supported by the generic MMIO bindings and code, but no. It's a pretty obvious usecase to just use some from 0..n of a MMIO GPIO bank, so add support for this. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- Changes in v2: - Collect Rob's ACK on patch 1 - The pdata member is called ngpio not ngpios which shows up if I compile the gpio-mmio driver with the *right* flags. - Link to v1: https://lore.kernel.org/r/20241016-gpio-ngpios-v1-0-f16cf154f715@linaro.org --- Linus Walleij (2): dt-bindings: gpio-mmio: Add ngpios property gpio: mmio: Parse ngpios property Documentation/devicetree/bindings/gpio/gpio-mmio.yaml | 13 ++++++++++++- drivers/gpio/gpio-mmio.c | 4 ++++ 2 files changed, 16 insertions(+), 1 deletion(-) --- base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc change-id: 20241016-gpio-ngpios-0cad57f0a757 Best regards,