| Message ID | 20240117083251.53868-1-hector.palacios@digi.com |
|---|---|
| Headers | show |
| Series | gpio: support i.MX93 truly available GPIO pins | expand |
On 17/01/2024 09:32, Hector Palacios wrote: > Some SoCs, such as i.MX93, don't have all 32 pins available > per port. Allow optional generic 'ngpios' property to be > specified from the device tree and default to 32 if the > property does not exist. > > Signed-off-by: Hector Palacios <hector.palacios@digi.com> > --- This is a friendly reminder during the review process. It seems my or other reviewer's previous comments were not fully addressed. Maybe the feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. Best regards, Krzysztof
Hello Andy, On 1/17/24 21:51, Andy Shevchenko wrote: >> Some SoCs, such as i.MX93, don't have all 32 pins available >> per port. Allow optional generic 'ngpios' property to be >> specified from the device tree and default to >> VF610_GPIO_PER_PORT (32) if the property does not exist. > > ... > >> + ret = device_property_read_u32(dev, "ngpios", &ngpios); >> + if (ret || ngpios > VF610_GPIO_PER_PORT) >> + gc->ngpio = VF610_GPIO_PER_PORT; >> + else >> + gc->ngpio = (u16)ngpios; > > This property is being read by the GPIOLIB core. Why do you need to repeat this? My apologies; I had not seen this. I'll use gpiochip_get_ngpios() on the next iteration. Thank you!
On 1/18/24 13:03, Bartosz Golaszewski wrote: > > On Thu, Jan 18, 2024 at 10:04 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> >> On Thu, Jan 18, 2024 at 10:25 AM Hector Palacios >> <hector.palacios@digi.com> wrote: >>> On 1/17/24 21:51, Andy Shevchenko wrote: >>>>> Some SoCs, such as i.MX93, don't have all 32 pins available >>>>> per port. Allow optional generic 'ngpios' property to be >>>>> specified from the device tree and default to >>>>> VF610_GPIO_PER_PORT (32) if the property does not exist. >> >> ... >> >>>>> + ret = device_property_read_u32(dev, "ngpios", &ngpios); >>>>> + if (ret || ngpios > VF610_GPIO_PER_PORT) >>>>> + gc->ngpio = VF610_GPIO_PER_PORT; >>>>> + else >>>>> + gc->ngpio = (u16)ngpios; >>>> >>>> This property is being read by the GPIOLIB core. Why do you need to repeat this? >>> >>> My apologies; I had not seen this. >>> I'll use gpiochip_get_ngpios() on the next iteration. >> >> But still why? >> https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L867 >> >> It's called for every driver. >> >> Maybe it's needed to be refactored to allow fallbacks? Then can the >> GPIO MMIO case also be updated? >> > > I guess it's because Hector wants to set an upper limit on the number of GPIOs? I think Andy is suggesting to rework the gpio-vf610 driver to use bgpio_chip struct (it doesn't currently), and then I guess the 'ngpio' property gets read automatically if you call bgpio_init().
All four GPIO ports of i.MX93 SoC show 32 pins available, but not every port has 32 pins. Add support on the GPIO driver to 'ngpios' property and set the truly available pins on the SoC device tree. v3 * Move DT bindings to a patch of its own * Improve reasoning for adding support in driver v2 * Add 'ngpios' property to DT binding for proper validation Hector Palacios (3): gpio: vf610: add support to DT 'ngpios' property dt-bindings: gpio: vf610: add optional 'ngpios' arm64: dts: imx93: specify available 'ngpios' per GPIO port Documentation/devicetree/bindings/gpio/gpio-vf610.yaml | 6 ++++++ arch/arm64/boot/dts/freescale/imx93.dtsi | 4 ++++ drivers/gpio/gpio-vf610.c | 7 ++++++- 3 files changed, 16 insertions(+), 1 deletion(-)