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.
> 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?
> */
...
> - 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;
--
With Best Regards,
Andy Shevchenko
