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;
> >
>
