[PATCH v3 0/3] gpio: support i.MX93 truly available GPIO pins

[PATCH v3 0/3] gpio: support i.MX93 truly available GPIO pins

[Hector Palacios]

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(-)

[PATCH v3 1/3] gpio: vf610: add support to DT 'ngpios' property

[Hector Palacios]

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(-)

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;

Re: [PATCH v3 1/3] gpio: vf610: add support to DT 'ngpios' property

[Andy Shevchenko]

On Wed, Jan 17, 2024 at 10:35 AM Hector Palacios
<hector.palacios@digi.com> 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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] gpio: vf610: add support to DT 'ngpios' property

[Hector Palacios]

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!
-- 
Héctor Palacios

Re: [PATCH v3 1/3] gpio: vf610: add support to DT 'ngpios' property

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] gpio: vf610: add support to DT 'ngpios' property

[Bartosz Golaszewski]

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

Re: [PATCH v3 1/3] gpio: vf610: add support to DT 'ngpios' property

[Hector Palacios]

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().
-- 
Héctor Palacios

Re: [PATCH v3 1/3] gpio: vf610: add support to DT 'ngpios' property

[Bartosz Golaszewski]

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

[PATCH v3 2/3] dt-bindings: gpio: vf610: add optional 'ngpios'

[Hector Palacios]

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>
---
 Documentation/devicetree/bindings/gpio/gpio-vf610.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-vf610.yaml b/Documentation/devicetree/bindings/gpio/gpio-vf610.yaml
index a27f92950257..ba4ebdbc5546 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-vf610.yaml
+++ b/Documentation/devicetree/bindings/gpio/gpio-vf610.yaml
@@ -65,6 +65,12 @@ properties:
     minItems: 1
     maxItems: 4
 
+  ngpios:
+    description: The number of GPIO pins of the port
+    minimum: 1
+    maximum: 32
+    default: 32
+
 patternProperties:
   "^.+-hog(-[0-9]+)?$":
     type: object

Re: [PATCH v3 2/3] dt-bindings: gpio: vf610: add optional 'ngpios'

[Krzysztof Kozlowski]

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

[PATCH v3 3/3] arm64: dts: imx93: specify available 'ngpios' per GPIO port

[Hector Palacios]

According to NXP HRM for i.MX93, the following GPIO pins are available:
- GPIO1: 16 pins (0..15)
- GPIO2: 30 pins (0..29)
- GPIO3: 32 pins (0..31)
- GPIO4: 30 pins (0..29)

Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
 arch/arm64/boot/dts/freescale/imx93.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx93.dtsi b/arch/arm64/boot/dts/freescale/imx93.dtsi
index 34c0540276d1..7eb2cab7c749 100644
--- a/arch/arm64/boot/dts/freescale/imx93.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx93.dtsi
@@ -970,6 +970,7 @@ gpio2: gpio@43810000 {
 				 <&clk IMX93_CLK_GPIO2_GATE>;
 			clock-names = "gpio", "port";
 			gpio-ranges = <&iomuxc 0 4 30>;
+			ngpios = <30>;
 		};
 
 		gpio3: gpio@43820000 {
@@ -986,6 +987,7 @@ gpio3: gpio@43820000 {
 			clock-names = "gpio", "port";
 			gpio-ranges = <&iomuxc 0 84 8>, <&iomuxc 8 66 18>,
 				      <&iomuxc 26 34 2>, <&iomuxc 28 0 4>;
+			ngpios = <32>;
 		};
 
 		gpio4: gpio@43830000 {
@@ -1001,6 +1003,7 @@ gpio4: gpio@43830000 {
 				 <&clk IMX93_CLK_GPIO4_GATE>;
 			clock-names = "gpio", "port";
 			gpio-ranges = <&iomuxc 0 38 28>, <&iomuxc 28 36 2>;
+			ngpios = <30>;
 		};
 
 		gpio1: gpio@47400000 {
@@ -1016,6 +1019,7 @@ gpio1: gpio@47400000 {
 				 <&clk IMX93_CLK_GPIO1_GATE>;
 			clock-names = "gpio", "port";
 			gpio-ranges = <&iomuxc 0 92 16>;
+			ngpios = <16>;
 		};
 
 		ocotp: efuse@47510000 {