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]

 



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.

Regards,

Hans




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




[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux