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




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

  Powered by Linux