Hi, On 9/11/23 16:04, Bartosz Golaszewski wrote: > 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. After your Acked-by for the first 2 patches this morning I assumed you were fine with the entire set, so I've already created a signed tag for it. Therefor I don't want to / cannot change the hashes of the commits, so a follow-up patch it is. I'll prep a patch adding the comment tomorrow. Note the original signed-tag + pull-req is still fine for coordination between the pdx86 code and the GPIO tree. Please let me know if you want an updated pull-req which includes the follow-up patch adding the comment. Regards, Hans
