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