| Message ID | 20230909141816.58358-7-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | x86-android-tablets: Stop using gpiolib private APIs | expand |
On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@redhat.com> 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. The best patch in the series! Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> See below a couple of questions. ... > -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); > > - 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); > } > - *desc = gpiod; > + if (desc) > + *desc = gpiod; Are we expecting that callers may not want the GPIO descriptor out of this function? Sounds a bit weird if so. > return 0; > } ... > * The bq25890_charger driver controls these through I2C, but this only > * works if not overridden by the pins. Set these pins here: > - * 1. Set /CE to 0 to allow charging. > + * 1. Set /CE to 1 to allow charging. > * 2. Set OTG to 0 disable V5 boost output since the 5V boost output of > * the main "bq25892_1" charger is used when necessary. Shouldn't we use terminology "active"/"non-active" instead of plain 0 and 1 in the above? > */ ... > - ret = x86_android_tablet_get_gpiod("INT33FF:02", 22, &gpiod); > + ret = x86_android_tablet_get_gpiod("INT33FF:02", 22, "bq25892_0_ce", > + true, GPIOD_OUT_HIGH, NULL); > if (ret < 0) > return ret; Hmm... Maybe better this function to return an error pointer or valid pointer, and in the code you choose what to do with that? ... > /* OTG pin */ > - ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, &gpiod); > + ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, "bq25892_0_otg", > + false, GPIOD_OUT_LOW, NULL); Ditto. > if (ret < 0) > return ret;
Hi, On 9/10/23 10:24, Andy Shevchenko wrote: > On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@redhat.com> 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. > > The best patch in the series! > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > See below a couple of questions. > > ... > >> -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); >> >> - 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); >> } > >> - *desc = gpiod; >> + if (desc) >> + *desc = gpiod; > > Are we expecting that callers may not want the GPIO descriptor out of > this function? > Sounds a bit weird if so. Yes specifically the Charge-enable and OTG (Vboost enable) pins on the bq25892 charger on the Lenovo Yoga Tab 3 just need to be set to a fixed value, so we request the pins with GPIOD_OUT_LOW / _HIGH and then leave them at that value. I think similar stuff may come up later, so it seems nice to be able to not have to pass an otherwise unused gpio_desc pointer. > >> return 0; >> } > > ... > >> * The bq25890_charger driver controls these through I2C, but this only >> * works if not overridden by the pins. Set these pins here: >> - * 1. Set /CE to 0 to allow charging. > >> + * 1. Set /CE to 1 to allow charging. >> * 2. Set OTG to 0 disable V5 boost output since the 5V boost output of >> * the main "bq25892_1" charger is used when necessary. > > Shouldn't we use terminology "active"/"non-active" instead of plain 0 > and 1 in the above? Well the flags are called GPIOD_OUT_HIGH / GPIOD_OUT_LOW and with gpiod_set_value 0/1 is used. I'm not in favor of adding "active"/"non-active" into the mix. That just adds a 3th way to say 0/low or 1/high. Regards, Hans > >> */ > > ... > >> - ret = x86_android_tablet_get_gpiod("INT33FF:02", 22, &gpiod); >> + ret = x86_android_tablet_get_gpiod("INT33FF:02", 22, "bq25892_0_ce", >> + true, GPIOD_OUT_HIGH, NULL); >> if (ret < 0) >> return ret; > > Hmm... Maybe better this function to return an error pointer or valid > pointer, and in the code you choose what to do with that? > > ... > >> /* OTG pin */ >> - ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, &gpiod); >> + ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, "bq25892_0_otg", >> + false, GPIOD_OUT_LOW, NULL); > > Ditto. > >> if (ret < 0) >> return ret; >
Hi Andy, I missed 1 remark: On 9/10/23 10:24, Andy Shevchenko wrote: >> - ret = x86_android_tablet_get_gpiod("INT33FF:02", 22, &gpiod); >> + ret = x86_android_tablet_get_gpiod("INT33FF:02", 22, "bq25892_0_ce", >> + true, GPIOD_OUT_HIGH, NULL); >> if (ret < 0) >> return ret; > > Hmm... Maybe better this function to return an error pointer or valid > pointer, and in the code you choose what to do with that? > > ... > >> /* OTG pin */ >> - ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, &gpiod); >> + ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, "bq25892_0_otg", >> + false, GPIOD_OUT_LOW, NULL); > > Ditto. Yes I did consider that, but x86_android_tablet_get_gpiod() already followed the return-by-reference model and I did not want to add changing that into the changes this patch already makes. Regards, Hans
On Sun, Sep 10, 2023 at 1:26 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 9/10/23 10:24, Andy Shevchenko wrote: > > On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@redhat.com> 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. > > > > The best patch in the series! > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > See below a couple of questions. > > > > ... > > > >> -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); > >> > >> - 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); > >> } > > > >> - *desc = gpiod; > >> + if (desc) > >> + *desc = gpiod; > > > > Are we expecting that callers may not want the GPIO descriptor out of > > this function? > > Sounds a bit weird if so. > > Yes specifically the Charge-enable and OTG (Vboost enable) pins on the > bq25892 charger on the Lenovo Yoga Tab 3 just need to be set to a fixed > value, so we request the pins with GPIOD_OUT_LOW / _HIGH and then > leave them at that value. > You mean you leak the descriptor? It would warrant either a comment or storing the descriptor somewhere and cleaning it up if the device can be unbound (I guess it can since the driver can be built as module). Bart > I think similar stuff may come up later, so it seems nice to be able > to not have to pass an otherwise unused gpio_desc pointer. > > > > > >> return 0; > >> } > > > > ... > > > >> * The bq25890_charger driver controls these through I2C, but this only > >> * works if not overridden by the pins. Set these pins here: > >> - * 1. Set /CE to 0 to allow charging. > > > >> + * 1. Set /CE to 1 to allow charging. > >> * 2. Set OTG to 0 disable V5 boost output since the 5V boost output of > >> * the main "bq25892_1" charger is used when necessary. > > > > Shouldn't we use terminology "active"/"non-active" instead of plain 0 > > and 1 in the above? > > Well the flags are called GPIOD_OUT_HIGH / GPIOD_OUT_LOW and > with gpiod_set_value 0/1 is used. I'm not in favor of adding > "active"/"non-active" into the mix. That just adds a 3th way to > say 0/low or 1/high. > > Regards, > > Hans > > > > > > > >> */ > > > > ... > > > >> - ret = x86_android_tablet_get_gpiod("INT33FF:02", 22, &gpiod); > >> + ret = x86_android_tablet_get_gpiod("INT33FF:02", 22, "bq25892_0_ce", > >> + true, GPIOD_OUT_HIGH, NULL); > >> if (ret < 0) > >> return ret; > > > > Hmm... Maybe better this function to return an error pointer or valid > > pointer, and in the code you choose what to do with that? > > > > ... > > > >> /* OTG pin */ > >> - ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, &gpiod); > >> + ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, "bq25892_0_otg", > >> + false, GPIOD_OUT_LOW, NULL); > > > > Ditto. > > > >> if (ret < 0) > >> return ret; > > >
On Sat, Sep 9, 2023 at 4:18 PM Hans de Goede <hdegoede@redhat.com> 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@bgdev.pl> > Closes: https://lore.kernel.org/platform-driver-x86/20230905185309.131295-12-brgl@bgdev.pl/ > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > .../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? Bart > - 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); > } >
On Mon, Sep 11, 2023 at 02:49:44PM +0200, Bartosz Golaszewski wrote: > On Sun, Sep 10, 2023 at 1:26 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On 9/10/23 10:24, Andy Shevchenko wrote: > > > On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@redhat.com> wrote: ... > > >> + gpiod = devm_gpiod_get(&x86_android_tablet_device->dev, con_id, dflags); ^^^ > > >> - *desc = gpiod; > > >> + if (desc) > > >> + *desc = gpiod; > > > > > > Are we expecting that callers may not want the GPIO descriptor out of > > > this function? > > > Sounds a bit weird if so. > > > > Yes specifically the Charge-enable and OTG (Vboost enable) pins on the > > bq25892 charger on the Lenovo Yoga Tab 3 just need to be set to a fixed > > value, so we request the pins with GPIOD_OUT_LOW / _HIGH and then > > leave them at that value. > > You mean you leak the descriptor? It would warrant either a comment or > storing the descriptor somewhere and cleaning it up if the device can > be unbound (I guess it can since the driver can be built as module). Note devm_*() above.
Hi, On 9/11/23 14:56, Andy Shevchenko wrote: > On Mon, Sep 11, 2023 at 02:49:44PM +0200, Bartosz Golaszewski wrote: >> On Sun, Sep 10, 2023 at 1:26 PM Hans de Goede <hdegoede@redhat.com> wrote: >>> On 9/10/23 10:24, Andy Shevchenko wrote: >>>> On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@redhat.com> wrote: > > ... > >>>>> + gpiod = devm_gpiod_get(&x86_android_tablet_device->dev, con_id, dflags); > > ^^^ > >>>>> - *desc = gpiod; >>>>> + if (desc) >>>>> + *desc = gpiod; >>>> >>>> Are we expecting that callers may not want the GPIO descriptor out of >>>> this function? >>>> Sounds a bit weird if so. >>> >>> Yes specifically the Charge-enable and OTG (Vboost enable) pins on the >>> bq25892 charger on the Lenovo Yoga Tab 3 just need to be set to a fixed >>> value, so we request the pins with GPIOD_OUT_LOW / _HIGH and then >>> leave them at that value. >> >> You mean you leak the descriptor? It would warrant either a comment or >> storing the descriptor somewhere and cleaning it up if the device can >> be unbound (I guess it can since the driver can be built as module). > > Note devm_*() above. Right, the GPIOs will be free-ed on unbound through the devm framework. Regards, Hans
On Mon, Sep 11, 2023 at 2:56 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Sep 11, 2023 at 02:49:44PM +0200, Bartosz Golaszewski wrote: > > On Sun, Sep 10, 2023 at 1:26 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > On 9/10/23 10:24, Andy Shevchenko wrote: > > > > On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@redhat.com> wrote: > > ... > > > > >> + gpiod = devm_gpiod_get(&x86_android_tablet_device->dev, con_id, dflags); > > ^^^ > > > > >> - *desc = gpiod; > > > >> + if (desc) > > > >> + *desc = gpiod; > > > > > > > > Are we expecting that callers may not want the GPIO descriptor out of > > > > this function? > > > > Sounds a bit weird if so. > > > > > > Yes specifically the Charge-enable and OTG (Vboost enable) pins on the > > > bq25892 charger on the Lenovo Yoga Tab 3 just need to be set to a fixed > > > value, so we request the pins with GPIOD_OUT_LOW / _HIGH and then > > > leave them at that value. > > > > You mean you leak the descriptor? It would warrant either a comment or > > storing the descriptor somewhere and cleaning it up if the device can > > be unbound (I guess it can since the driver can be built as module). > > Note devm_*() above. > Ah, nevermind my comment then. Bart
Hi, On 9/11/23 14:50, Bartosz Golaszewski wrote: > On Sat, Sep 9, 2023 at 4:18 PM Hans de Goede <hdegoede@redhat.com> 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@bgdev.pl> >> Closes: https://lore.kernel.org/platform-driver-x86/20230905185309.131295-12-brgl@bgdev.pl/ >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> .../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); >> } >> >
On Mon, Sep 11, 2023 at 3:08 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 9/11/23 14:50, Bartosz Golaszewski wrote: > > On Sat, Sep 9, 2023 at 4:18 PM Hans de Goede <hdegoede@redhat.com> 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@bgdev.pl> > >> Closes: https://lore.kernel.org/platform-driver-x86/20230905185309.131295-12-brgl@bgdev.pl/ > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> .../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. It doesn't have to be a variable length array either. Typically GPIO lookups are static arrays that are added once and never removed. 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. Bart > 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); > >> } > >> > > >
Hi, On 9/11/23 15:18, Bartosz Golaszewski wrote: > On Mon, Sep 11, 2023 at 3:08 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 9/11/23 14:50, Bartosz Golaszewski wrote: >>> On Sat, Sep 9, 2023 at 4:18 PM Hans de Goede <hdegoede@redhat.com> 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@bgdev.pl> >>>> Closes: https://lore.kernel.org/platform-driver-x86/20230905185309.131295-12-brgl@bgdev.pl/ >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> .../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
On Mon, Sep 11, 2023 at 3:32 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 9/11/23 15:18, Bartosz Golaszewski wrote: > > On Mon, Sep 11, 2023 at 3:08 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi, > >> > >> On 9/11/23 14:50, Bartosz Golaszewski wrote: > >>> On Sat, Sep 9, 2023 at 4:18 PM Hans de Goede <hdegoede@redhat.com> 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@bgdev.pl> > >>>> Closes: https://lore.kernel.org/platform-driver-x86/20230905185309.131295-12-brgl@bgdev.pl/ > >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>>> --- > >>>> .../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. If there are more devices, then I agree - let's keep dynamic allocation. Just please: add a comment why you're doing it this way so that people don't just copy and paste it elsewhere. Bart. > > Regards, > > Hans > >
Hi Bart, On 9/11/23 15:37, Bartosz Golaszewski wrote: > On Mon, Sep 11, 2023 at 3:32 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 9/11/23 15:18, Bartosz Golaszewski wrote: >>> On Mon, Sep 11, 2023 at 3:08 PM Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>> Hi, >>>> >>>> On 9/11/23 14:50, Bartosz Golaszewski wrote: >>>>> On Sat, Sep 9, 2023 at 4:18 PM Hans de Goede <hdegoede@redhat.com> 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@bgdev.pl> >>>>>> Closes: https://lore.kernel.org/platform-driver-x86/20230905185309.131295-12-brgl@bgdev.pl/ >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>> --- >>>>>> .../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
On Mon, Sep 11, 2023 at 3:53 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> wrote: > >> > >> Hi, > >> > >> On 9/11/23 15:18, Bartosz Golaszewski wrote: > >>> On Mon, Sep 11, 2023 at 3:08 PM Hans de Goede <hdegoede@redhat.com> wrote: > >>>> > >>>> Hi, > >>>> > >>>> On 9/11/23 14:50, Bartosz Golaszewski wrote: > >>>>> On Sat, Sep 9, 2023 at 4:18 PM Hans de Goede <hdegoede@redhat.com> 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@bgdev.pl> > >>>>>> Closes: https://lore.kernel.org/platform-driver-x86/20230905185309.131295-12-brgl@bgdev.pl/ > >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>>>>> --- > >>>>>> .../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 > >
Hi, On 9/11/23 16:04, Bartosz Golaszewski wrote: > On Mon, Sep 11, 2023 at 3:53 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> wrote: >>>> >>>> Hi, >>>> >>>> On 9/11/23 15:18, Bartosz Golaszewski wrote: >>>>> On Mon, Sep 11, 2023 at 3:08 PM Hans de Goede <hdegoede@redhat.com> wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> On 9/11/23 14:50, Bartosz Golaszewski wrote: >>>>>>> On Sat, Sep 9, 2023 at 4:18 PM Hans de Goede <hdegoede@redhat.com> 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@bgdev.pl> >>>>>>>> Closes: https://lore.kernel.org/platform-driver-x86/20230905185309.131295-12-brgl@bgdev.pl/ >>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>> --- >>>>>>>> .../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
On Mon, Sep 11, 2023 at 04:12:00PM +0200, Hans de Goede wrote: > On 9/11/23 16:04, Bartosz Golaszewski wrote: ... > >>>>>>>> + 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; Actually you can use GPIO_LOOKUP() macro here as it provides a compound literal. lookup->table[0] = GPIO_LOOKUP(...); > Therefor I don't want to / cannot change the hashes of the commits, > so a follow-up patch it is.
On Mon, Sep 11, 2023 at 5:36 PM Andy Shevchenko <andy@kernel.org> wrote: > > On Mon, Sep 11, 2023 at 04:12:00PM +0200, Hans de Goede wrote: > > On 9/11/23 16:04, Bartosz Golaszewski wrote: > > ... > > > >>>>>>>> + 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; > > Actually you can use GPIO_LOOKUP() macro here as it provides a compound > literal. > > lookup->table[0] = GPIO_LOOKUP(...); > > > Therefor I don't want to / cannot change the hashes of the commits, > > so a follow-up patch it is. > Which makes me think, I should have used it in my SPI patch... Bart
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); - 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); } - *desc = gpiod; + if (desc) + *desc = gpiod; + return 0; } @@ -79,7 +83,8 @@ int x86_acpi_irq_helper_get(const struct x86_acpi_irq_data *data) return irq; case X86_ACPI_IRQ_TYPE_GPIOINT: /* Like acpi_dev_gpio_irq_get(), but without parsing ACPI resources */ - ret = x86_android_tablet_get_gpiod(data->chip, data->index, &gpiod); + ret = x86_android_tablet_get_gpiod(data->chip, data->index, data->con_id, + false, GPIOD_ASIS, &gpiod); if (ret) return ret; @@ -356,7 +361,9 @@ static __init int x86_android_tablet_probe(struct platform_device *pdev) for (i = 0; i < dev_info->gpio_button_count; i++) { ret = x86_android_tablet_get_gpiod(dev_info->gpio_button[i].chip, - dev_info->gpio_button[i].pin, &gpiod); + dev_info->gpio_button[i].pin, + dev_info->gpio_button[i].button.desc, + false, GPIOD_IN, &gpiod); if (ret < 0) { x86_android_tablet_remove(pdev); return ret; @@ -364,6 +371,8 @@ static __init int x86_android_tablet_probe(struct platform_device *pdev) buttons[i] = dev_info->gpio_button[i].button; buttons[i].gpio = desc_to_gpio(gpiod); + /* Release gpiod so that gpio-keys can request it */ + devm_gpiod_put(&x86_android_tablet_device->dev, gpiod); } pdata.buttons = buttons; diff --git a/drivers/platform/x86/x86-android-tablets/lenovo.c b/drivers/platform/x86/x86-android-tablets/lenovo.c index 26a4ef670ad7..35aa2968d726 100644 --- a/drivers/platform/x86/x86-android-tablets/lenovo.c +++ b/drivers/platform/x86/x86-android-tablets/lenovo.c @@ -95,6 +95,7 @@ static const struct x86_i2c_client_info lenovo_yb1_x90_i2c_clients[] __initconst .index = 56, .trigger = ACPI_EDGE_SENSITIVE, .polarity = ACPI_ACTIVE_LOW, + .con_id = "goodix_ts_irq", }, }, { /* Wacom Digitizer in keyboard half */ @@ -111,6 +112,7 @@ static const struct x86_i2c_client_info lenovo_yb1_x90_i2c_clients[] __initconst .index = 49, .trigger = ACPI_LEVEL_SENSITIVE, .polarity = ACPI_ACTIVE_LOW, + .con_id = "wacom_irq", }, }, { /* LP8557 Backlight controller */ @@ -136,6 +138,7 @@ static const struct x86_i2c_client_info lenovo_yb1_x90_i2c_clients[] __initconst .index = 77, .trigger = ACPI_LEVEL_SENSITIVE, .polarity = ACPI_ACTIVE_LOW, + .con_id = "hideep_ts_irq", }, }, }; @@ -321,6 +324,7 @@ static struct x86_i2c_client_info lenovo_yoga_tab2_830_1050_i2c_clients[] __init .index = 2, .trigger = ACPI_EDGE_SENSITIVE, .polarity = ACPI_ACTIVE_HIGH, + .con_id = "bq24292i_irq", }, }, { /* BQ27541 fuel-gauge */ @@ -431,7 +435,8 @@ static int __init lenovo_yoga_tab2_830_1050_init_touchscreen(void) int ret; /* Use PMIC GPIO 10 bootstrap pin to differentiate 830 vs 1050 */ - ret = x86_android_tablet_get_gpiod("gpio_crystalcove", 10, &gpiod); + ret = x86_android_tablet_get_gpiod("gpio_crystalcove", 10, "yoga_bootstrap", + false, GPIOD_IN, &gpiod); if (ret) return ret; @@ -615,6 +620,7 @@ static const struct x86_i2c_client_info lenovo_yt3_i2c_clients[] __initconst = { .index = 5, .trigger = ACPI_EDGE_SENSITIVE, .polarity = ACPI_ACTIVE_LOW, + .con_id = "bq25892_0_irq", }, }, { /* bq27500 fuel-gauge for the round li-ion cells in the hinge */ @@ -640,6 +646,7 @@ static const struct x86_i2c_client_info lenovo_yt3_i2c_clients[] __initconst = { .index = 77, .trigger = ACPI_LEVEL_SENSITIVE, .polarity = ACPI_ACTIVE_LOW, + .con_id = "hideep_ts_irq", }, }, { /* LP8557 Backlight controller */ @@ -655,7 +662,6 @@ static const struct x86_i2c_client_info lenovo_yt3_i2c_clients[] __initconst = { static int __init lenovo_yt3_init(void) { - struct gpio_desc *gpiod; int ret; /* @@ -665,31 +671,23 @@ static int __init lenovo_yt3_init(void) * * The bq25890_charger driver controls these through I2C, but this only * works if not overridden by the pins. Set these pins here: - * 1. Set /CE to 0 to allow charging. + * 1. Set /CE to 1 to allow charging. * 2. Set OTG to 0 disable V5 boost output since the 5V boost output of * the main "bq25892_1" charger is used when necessary. */ /* /CE pin */ - ret = x86_android_tablet_get_gpiod("INT33FF:02", 22, &gpiod); + ret = x86_android_tablet_get_gpiod("INT33FF:02", 22, "bq25892_0_ce", + true, GPIOD_OUT_HIGH, NULL); if (ret < 0) return ret; - /* - * The gpio_desc returned by x86_android_tablet_get_gpiod() is a "raw" - * gpio_desc, that is there is no way to pass lookup-flags like - * GPIO_ACTIVE_LOW. Set the GPIO to 0 here to enable charging since - * the /CE pin is active-low, but not marked as such in the gpio_desc. - */ - gpiod_set_value(gpiod, 0); - /* OTG pin */ - ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, &gpiod); + ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, "bq25892_0_otg", + false, GPIOD_OUT_LOW, NULL); if (ret < 0) return ret; - gpiod_set_value(gpiod, 0); - /* Enable the regulators used by the touchscreen */ intel_soc_pmic_exec_mipi_pmic_seq_element(0x6e, 0x9b, 0x02, 0xff); intel_soc_pmic_exec_mipi_pmic_seq_element(0x6e, 0xa0, 0x02, 0xff); diff --git a/drivers/platform/x86/x86-android-tablets/other.c b/drivers/platform/x86/x86-android-tablets/other.c index 621ca1e54d1f..bc6bbf7ec6ea 100644 --- a/drivers/platform/x86/x86-android-tablets/other.c +++ b/drivers/platform/x86/x86-android-tablets/other.c @@ -47,6 +47,7 @@ static const struct x86_i2c_client_info acer_b1_750_i2c_clients[] __initconst = .index = 3, .trigger = ACPI_EDGE_SENSITIVE, .polarity = ACPI_ACTIVE_LOW, + .con_id = "NVT-ts_irq", }, }, { /* BMA250E accelerometer */ @@ -62,6 +63,7 @@ static const struct x86_i2c_client_info acer_b1_750_i2c_clients[] __initconst = .index = 25, .trigger = ACPI_LEVEL_SENSITIVE, .polarity = ACPI_ACTIVE_HIGH, + .con_id = "bma250e_irq", }, }, }; @@ -174,6 +176,7 @@ static const struct x86_i2c_client_info chuwi_hi8_i2c_clients[] __initconst = { .index = 23, .trigger = ACPI_LEVEL_SENSITIVE, .polarity = ACPI_ACTIVE_HIGH, + .con_id = "bma250e_irq", }, }, }; @@ -312,6 +315,7 @@ static const struct x86_i2c_client_info medion_lifetab_s10346_i2c_clients[] __in .index = 23, .trigger = ACPI_EDGE_SENSITIVE, .polarity = ACPI_ACTIVE_HIGH, + .con_id = "kxtj21009_irq", }, }, { /* goodix touchscreen */ @@ -402,6 +406,7 @@ static const struct x86_i2c_client_info nextbook_ares8_i2c_clients[] __initconst .index = 3, .trigger = ACPI_EDGE_SENSITIVE, .polarity = ACPI_ACTIVE_LOW, + .con_id = "ft5416_irq", }, }, }; @@ -460,6 +465,7 @@ static const struct x86_i2c_client_info nextbook_ares8a_i2c_clients[] __initcons .index = 17, .trigger = ACPI_EDGE_SENSITIVE, .polarity = ACPI_ACTIVE_LOW, + .con_id = "ft5416_irq", }, }, }; diff --git a/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h b/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h index bf97fb84c0d4..9d2fb7fded6d 100644 --- a/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h +++ b/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h @@ -10,6 +10,7 @@ #ifndef __PDX86_X86_ANDROID_TABLETS_H #define __PDX86_X86_ANDROID_TABLETS_H +#include <linux/gpio/consumer.h> #include <linux/gpio_keys.h> #include <linux/i2c.h> #include <linux/irqdomain_defs.h> @@ -37,6 +38,7 @@ struct x86_acpi_irq_data { int index; int trigger; /* ACPI_EDGE_SENSITIVE / ACPI_LEVEL_SENSITIVE */ int polarity; /* ACPI_ACTIVE_HIGH / ACPI_ACTIVE_LOW / ACPI_ACTIVE_BOTH */ + const char *con_id; }; /* Structs to describe devices to instantiate */ @@ -81,7 +83,9 @@ struct x86_dev_info { void (*exit)(void); }; -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); int x86_acpi_irq_helper_get(const struct x86_acpi_irq_data *data); /*
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@bgdev.pl> Closes: https://lore.kernel.org/platform-driver-x86/20230905185309.131295-12-brgl@bgdev.pl/ Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../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(-)