Re: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Andy Shevchenko]

On Fri, Feb 24, 2023 at 4:42 PM Asmaa Mnebhi <asmaa@xxxxxxxxxx> wrote:
>
> > > > > > > Ah that’s my bad. The property should be called "ngpios" like
> > > > > > > in the DT
> > > > > > documentation. Will fix.
> > > > > >
> > > > > > And why do you need it? What's a corner case that the GPIO
> > > > > > library doesn't handle yet?
> > > > >
> > > > > We have 2 gpiochips, gpiochip 0 supports 32 gpio pins and gpiochip
> > > > > 1
> > > > supports only 24 pins.
> > > > > If I remove the logic from gpio-mlxbf3.c, the gpiolib.c logic will
> > > > > correctly set
> > > > the ngpios = 32 for gpiochip 0 but will wrongly set ngpios=32 for gpiogchip
> > 1:
> > > >
> > > > So, either you need to have two entries in DT per chip or ngpios should be
> > 56.
> > > >
> > > I already have 2 entries in my ACPI table, in the first entry, ngpios = 32 and
> > in the second entry ngpios = 24.
> >
> > Can you show the DSDT excerpt of this device? (Also including the pieces for
> > pin control)
>
> Our ACPI tables are internal:
>
> Device(GPI0) {
>         Name(_HID, "MLNXBF33")
>         Name(_UID, Zero)
>         Name(_CCA, 1)
>         Name(_CRS, ResourceTemplate() {
>           // for fw_gpio[0] yu block
>          Memory32Fixed(ReadWrite, 0x13401100, 0x00000080)
>          // for yu_gpio_causereg0 block
>          Memory32Fixed(ReadWrite, 0x13401c00, 0x00000020)
>          Interrupt(ResourceConsumer, Edge, ActiveHigh, Shared)
>            { BF_RSH0_YU }
>        })
>        Name(_DSD, Package() {
>          ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>          Package() {
>            Package () { "ngpios", 32 }, // Number of GPIO pins on gpio block 0
>            Package () { "gpio-reserved-ranges", Package () {5, 1, 7, 3, 11, 31}},
>          }
>        })
>      }
>  Device(GPI1) {
>        Name(_HID, "MLNXBF33")
>        Name(_UID, 1)
>        Name(_CCA, 1)
>        Name(_CRS, ResourceTemplate() {
>          // for fw_gpio[1] yu block
>          Memory32Fixed(ReadWrite, 0x13401180, 0x00000080)
>          // for yu_gpio_causereg1 block
>          Memory32Fixed(ReadWrite, 0x13401c20, 0x00000020)
>        })
>        Name(_DSD, Package() {
>          ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>          Package() {
>            Package () { "ngpios", 24 }, // Number of GPIO pins on gpio block 1
>            Package () { "gpio-reserved-ranges", Package () {0, 21}},
>          }
>        })
>   }
>
>     Device(PIN0) {
>       Name(_HID, "MLNXBF34")
>       Name(_UID, Zero)
>       Name(_CCA, 1)
>       Name(_CRS, ResourceTemplate() {
>         // for fw_gpio[0] and fw_gpio[1] yu blocks
>         Memory32Fixed(ReadWrite, 0x13401100, 0x00000100)
>       })
>      }

So far I see no issues with the tables. Thanks for sharing!

> > > Gpiochip_add_data_with_keys only reads the ngpios property if (ngpios
> > > == 0) which is not the case when bgpio_init is called. bgpio_init uses "sz"
> > argument to populate the ngpio in bgpio_init, which is not what we want.
> >
> > Maybe bgpio_init() is not a good API for your case?
>
> At the moment, bgpio_init handles all the direction in/out get/set gpio value for us.
> So I can either remove bgpio_init so that we get the correct ngpios property, but will have to define get_gpio, set_gpio, dir in and dir out.
> Or I can keep bgpio_init and device_property_read_u32 in the gpio-mlxbf3.c driver.

So far it seems the issue is in bgpio_init() that doesn't handle
ngpios properly. Maybe we need to fix this first?

Btw, have you considered using the gpio-regmap approach?

-- 
With Best Regards,
Andy Shevchenko