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 > >
