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. Regards, Hans
