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 Mon, Sep 11, 2023 at 3:53 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi Bart,
>
> On 9/11/23 15:37, Bartosz Golaszewski wrote:
> > On Mon, Sep 11, 2023 at 3:32 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> Hi,
> >>
> >> On 9/11/23 15:18, Bartosz Golaszewski wrote:
> >>> On Mon, Sep 11, 2023 at 3:08 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> On 9/11/23 14:50, Bartosz Golaszewski wrote:
> >>>>> On Sat, Sep 9, 2023 at 4: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.
> >>>>>>
> >>>>>> Reported-by: Bartosz Golaszewski <brgl@xxxxxxxx>
> >>>>>> Closes: https://lore.kernel.org/platform-driver-x86/20230905185309.131295-12-brgl@xxxxxxxx/
> >>>>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >>>>>> ---
> >>>>>>  .../platform/x86/x86-android-tablets/asus.c   |  1 +
> >>>>>>  .../platform/x86/x86-android-tablets/core.c   | 51 +++++++++++--------
> >>>>>>  .../platform/x86/x86-android-tablets/lenovo.c | 28 +++++-----
> >>>>>>  .../platform/x86/x86-android-tablets/other.c  |  6 +++
> >>>>>>  .../x86-android-tablets/x86-android-tablets.h |  6 ++-
> >>>>>>  5 files changed, 55 insertions(+), 37 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/platform/x86/x86-android-tablets/asus.c b/drivers/platform/x86/x86-android-tablets/asus.c
> >>>>>> index f9c4083be86d..227afbb51078 100644
> >>>>>> --- a/drivers/platform/x86/x86-android-tablets/asus.c
> >>>>>> +++ b/drivers/platform/x86/x86-android-tablets/asus.c
> >>>>>> @@ -303,6 +303,7 @@ static const struct x86_i2c_client_info asus_tf103c_i2c_clients[] __initconst =
> >>>>>>                         .index = 28,
> >>>>>>                         .trigger = ACPI_EDGE_SENSITIVE,
> >>>>>>                         .polarity = ACPI_ACTIVE_LOW,
> >>>>>> +                       .con_id = "atmel_mxt_ts_irq",
> >>>>>>                 },
> >>>>>>         },
> >>>>>>  };
> >>>>>> diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
> >>>>>> index 3d3101b2848f..673f3a14941b 100644
> >>>>>> --- a/drivers/platform/x86/x86-android-tablets/core.c
> >>>>>> +++ b/drivers/platform/x86/x86-android-tablets/core.c
> >>>>>> @@ -12,7 +12,7 @@
> >>>>>>
> >>>>>>  #include <linux/acpi.h>
> >>>>>>  #include <linux/dmi.h>
> >>>>>> -#include <linux/gpio/driver.h>
> >>>>>> +#include <linux/gpio/consumer.h>
> >>>>>>  #include <linux/gpio/machine.h>
> >>>>>>  #include <linux/irq.h>
> >>>>>>  #include <linux/module.h>
> >>>>>> @@ -21,35 +21,39 @@
> >>>>>>  #include <linux/string.h>
> >>>>>>
> >>>>>>  #include "x86-android-tablets.h"
> >>>>>> -/* For gpiochip_get_desc() which is EXPORT_SYMBOL_GPL() */
> >>>>>> -#include "../../../gpio/gpiolib.h"
> >>>>>> -#include "../../../gpio/gpiolib-acpi.h"
> >>>>>>
> >>>>>>  static struct platform_device *x86_android_tablet_device;
> >>>>>>
> >>>>>> -static int gpiochip_find_match_label(struct gpio_chip *gc, void *data)
> >>>>>> -{
> >>>>>> -       return gc->label && !strcmp(gc->label, data);
> >>>>>> -}
> >>>>>> -
> >>>>>> -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);
> >>>>>>
> >>>>>
> >>>>> Any reason for not creating static lookup tables for GPIOs here?
> >>>>
> >>>> Not sure what you mean with static?
> >>>>
> >>>> I guess you mean using global or stack memory instead of kmalloc() ?
> >>>>
> >>>> gcc did not like me putting a struct with a variable length array
> >>>> at the end on the stack, so I went with a kzalloc using the
> >>>> struct_size() helper for structs with variable length arrays instead.
> >>>>
> >>>> Note this only runs once at boot, so the small extra cost of
> >>>> the malloc + free is not really a big deal here.
> >>>>
> >>>> I did not try making it global data as that would make the function
> >>>> non re-entrant. Not that it is used in a re-entrant way anywhere,
> >>>> but generally I try to avoid creating code which is not re-entrant.
> >>>>
> >>>
> >>> I meant static-per-compilation-unit.
> >>
> >> I see.
> >>
> >>> It doesn't have to be a variable
> >>> length array either. Typically GPIO lookups are static arrays that are
> >>> added once and never removed.
> >>
> >> Right.
> >>
> >>> The SPI example I posted elsewhere is
> >>> different as it addresses a device quirk and cannot be generalized
> >>> like this. How many GPIOs would you need to describe for this
> >>> use-case? If it's just a few, then I'd go with static lookup tables.
> >>> If it's way more than disregard this comment.
> >>
> >> ATM x86_android_tablet_get_gpiod() gets called for 24 GPIOs,
> >> so more the just a few.
> >
> > For different devices? As in: dev_id would differ? If not then I'd go
> > with a static table, you can use GPIO_LOOKUP() macro and have one line
> > per GPIO.
>
> I know GPIO_LOOKUP() is already used for normal GPIO lookup cases
> like e.g. a reset pin where the gpiod_get() is done by the i2c_driver
> for the i2c_client.
>
> > If there are more devices, then I agree - let's keep dynamic
> > allocation.
>
> These are used to lookup GPIOs which the code needs access to *before*
> instantiating the actual device consuming the GPIO.
>
> Specifically these GPIOS either become i2c_board_info.irq which is
> passed to i2c_client_new() to instantiate i2c_client-s; or
> desc_to_gpio() is called on them for old fashioned platform-data
> which still uses old style GPIO numbers (gpio_keys platform data).
>
> Needing these GPIOs before instantiating their actual consumer
> devices is the whole reason why the code used to call gpiolib
> private APIs in the first place.
>
> Note since the consuming device is not instantiated yet there really
> is no dev_id. Instead the newly added x86_android_tablets
> platform_device gets used as dev_id so that the code has a device
> to do the lookups on to remove the gpiolib private API use.
>
> This trick with using the x86_android_tablets pdev is why the whole
> lookup is done dynamically.
>
> > Just please: add a comment why you're doing it this way so that people
> > don't just copy and paste it elsewhere.
>
> Ok, I can do a follow-up patch adding a comment above
> x86_android_tablet_get_gpiod() explaining that it should only
> be used for GPIOs which are needed before their consumer
> device has been instantiated.
>

I'd just fold it into the existing patch but do as you prefer.

Bart

> Regards,
>
> Hans
>
>




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

  Powered by Linux