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

[Asmaa Mnebhi <asmaa@xxxxxxxxxx>]

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

> 
> > 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.
Please let me know which approach you prefer.