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