Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Sep 10, 2023 at 1:26 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 9/10/23 10:24, Andy Shevchenko wrote:
> > On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> Refactor x86_android_tablet_get_gpiod() to no longer use
> >> gpiolib private functions like gpiochip_find().
> >>
> >> As a bonus this allows specifying that the GPIO is active-low,
> >> like the /CE (charge enable) pin on the bq25892 charger on
> >> the Lenovo Yoga Tablet 3.
> >
> > The best patch in the series!
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> >
> > See below a couple of questions.
> >
> > ...
> >
> >> -int x86_android_tablet_get_gpiod(const char *label, int pin, struct gpio_desc **desc)
> >> +int x86_android_tablet_get_gpiod(const char *chip, int pin, const char *con_id,
> >> +                                bool active_low, enum gpiod_flags dflags,
> >> +                                struct gpio_desc **desc)
> >>  {
> >> +       struct gpiod_lookup_table *lookup;
> >>         struct gpio_desc *gpiod;
> >> -       struct gpio_chip *chip;
> >>
> >> -       chip = gpiochip_find((void *)label, gpiochip_find_match_label);
> >> -       if (!chip) {
> >> -               pr_err("error cannot find GPIO chip %s\n", label);
> >> -               return -ENODEV;
> >> -       }
> >> +       lookup = kzalloc(struct_size(lookup, table, 2), GFP_KERNEL);
> >> +       if (!lookup)
> >> +               return -ENOMEM;
> >> +
> >> +       lookup->dev_id = KBUILD_MODNAME;
> >> +       lookup->table[0].key = chip;
> >> +       lookup->table[0].chip_hwnum = pin;
> >> +       lookup->table[0].con_id = con_id;
> >> +       lookup->table[0].flags = active_low ? GPIO_ACTIVE_LOW : GPIO_ACTIVE_HIGH;
> >> +
> >> +       gpiod_add_lookup_table(lookup);
> >> +       gpiod = devm_gpiod_get(&x86_android_tablet_device->dev, con_id, dflags);
> >> +       gpiod_remove_lookup_table(lookup);
> >> +       kfree(lookup);
> >>
> >> -       gpiod = gpiochip_get_desc(chip, pin);
> >>         if (IS_ERR(gpiod)) {
> >> -               pr_err("error %ld getting GPIO %s %d\n", PTR_ERR(gpiod), label, pin);
> >> +               pr_err("error %ld getting GPIO %s %d\n", PTR_ERR(gpiod), chip, pin);
> >>                 return PTR_ERR(gpiod);
> >>         }
> >
> >> -       *desc = gpiod;
> >> +       if (desc)
> >> +               *desc = gpiod;
> >
> > Are we expecting that callers may not want the GPIO descriptor out of
> > this function?
> > Sounds a bit weird if so.
>
> Yes specifically the Charge-enable and OTG (Vboost enable) pins on the
> bq25892 charger on the Lenovo Yoga Tab 3 just need to be set to a fixed
> value, so we request the pins with GPIOD_OUT_LOW / _HIGH and then
> leave them at that value.
>

You mean you leak the descriptor? It would warrant either a comment or
storing the descriptor somewhere and cleaning it up if the device can
be unbound (I guess it can since the driver can be built as module).

Bart

> I think similar stuff may come up later, so it seems nice to be able
> to not have to pass an otherwise unused gpio_desc pointer.
>
>
> >
> >>         return 0;
> >>  }
> >
> > ...
> >
> >>          * The bq25890_charger driver controls these through I2C, but this only
> >>          * works if not overridden by the pins. Set these pins here:
> >> -        * 1. Set /CE to 0 to allow charging.
> >
> >> +        * 1. Set /CE to 1 to allow charging.
> >>          * 2. Set OTG to 0 disable V5 boost output since the 5V boost output of
> >>          *    the main "bq25892_1" charger is used when necessary.
> >
> > Shouldn't we use terminology "active"/"non-active" instead of plain 0
> > and 1 in the above?
>
> Well the flags are called GPIOD_OUT_HIGH / GPIOD_OUT_LOW and
> with gpiod_set_value 0/1 is used. I'm not in favor of adding
> "active"/"non-active" into the mix. That just adds a 3th way to
> say 0/low or 1/high.
>
> Regards,
>
> Hans
>
>
>
>
> >
> >>          */
> >
> > ...
> >
> >> -       ret = x86_android_tablet_get_gpiod("INT33FF:02", 22, &gpiod);
> >> +       ret = x86_android_tablet_get_gpiod("INT33FF:02", 22, "bq25892_0_ce",
> >> +                                          true, GPIOD_OUT_HIGH, NULL);
> >>         if (ret < 0)
> >>                 return ret;
> >
> > Hmm... Maybe better this function to return an error pointer or valid
> > pointer, and in the code you choose what to do with that?
> >
> > ...
> >
> >>         /* OTG pin */
> >> -       ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, &gpiod);
> >> +       ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, "bq25892_0_otg",
> >> +                                          false, GPIOD_OUT_LOW, NULL);
> >
> > Ditto.
> >
> >>         if (ret < 0)
> >>                 return ret;
> >
>




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux