| Message ID | 20240117083251.53868-2-hector.palacios@digi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio: support i.MX93 truly available GPIO pins | expand |
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 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?
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? Bart > -- > With Best Regards, > Andy Shevchenko
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().
On Thu, Jan 18, 2024 at 1:45 PM Hector Palacios <hector.palacios@digi.com> wrote: > > 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(). No, Andy said (and even provided a link to the code) that "ngpios" is read ALWAYS when a new GPIO chip is registered with the GPIO core. It's just that it doesn't impose any limits but that could be addressed with imposing an upper limit in DT bindings maybe? Bart
diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c index 07e5e6323e86..4abdf75e9a0a 100644 --- a/drivers/gpio/gpio-vf610.c +++ b/drivers/gpio/gpio-vf610.c @@ -276,6 +276,7 @@ static int vf610_gpio_probe(struct platform_device *pdev) struct vf610_gpio_port *port; struct gpio_chip *gc; struct gpio_irq_chip *girq; + u32 ngpios; int i; int ret; bool dual_base; @@ -353,7 +354,11 @@ static int vf610_gpio_probe(struct platform_device *pdev) gc = &port->gc; gc->parent = dev; gc->label = dev_name(dev); - gc->ngpio = VF610_GPIO_PER_PORT; + 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; gc->base = -1; gc->request = gpiochip_generic_request;
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. Signed-off-by: Hector Palacios <hector.palacios@digi.com> --- drivers/gpio/gpio-vf610.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)