[PATCH 0/8] x86-android-tablets: Stop using gpiolib private APIs

[PATCH 0/8] x86-android-tablets: Stop using gpiolib private APIs

[Hans de Goede]

Hi Bart, et. al.,

As promised here is a patch-series to make the x86-android-tablets code
stop using gpiolib private APIs.

Patches 1-2 are (small) gpiolib-acpi.c patches to add a quirk
(using the existing quirk mechanisms) to allow removing
x86-android-tablets' acpi_gpiochip_free_interrupts() usage.

Since patches 3-4 depend on these I plan to add these to
the platform-drivers-x86-android-tablets immutable-branch / pull
with the entire set for you.

May I please have your (or Andy's or Mika's) Acked-by for
merging these 2 patches through this branch?

Patches 3-6 deal with actually removing the private gpiolib API usage.

Patches 7-8 are some small improvements on top.

My plan is to let these patches sit on the list for review for
a couple of days and then I'll prepare an immutable-branch
with 6.6-rc1 + this series and send you a pull-req for that
based on a signed tag.

Then you can base future versions of your
"gpio: convert users to gpio_device_find() and remove gpiochip_find()"
series on top and drop any workarounds for the x86-android-tablets
code from that patch-series.

Regards,

Hans


Hans de Goede (8):
  gpiolib: acpi: Check if a GPIO is listed in ignore_interrupt earlier
  gpiolib: acpi: Add a ignore interrupt quirk for Peaq C1010
  platform/x86: x86-android-tablets: Remove invalid_aei_gpiochip from
    Peaq C1010
  platform/x86: x86-android-tablets: Remove invalid_aei_gpiochip support
  platform/x86: x86-android-tablets: Create a platform_device from
    module_init()
  platform/x86: x86-android-tablets: Stop using gpiolib private APIs
  platform/x86: x86-android-tablets: Use platform-device as gpio-keys
    parent
  platform/x86: x86-android-tablets: Drop "linux,power-supply-name" from
    lenovo_yt3_bq25892_0_props[]

 drivers/gpio/gpiolib-acpi.c                   |  30 ++++-
 .../platform/x86/x86-android-tablets/asus.c   |   1 +
 .../platform/x86/x86-android-tablets/core.c   | 123 +++++++++++-------
 .../platform/x86/x86-android-tablets/lenovo.c |  29 ++---
 .../platform/x86/x86-android-tablets/other.c  |  11 +-
 .../x86-android-tablets/x86-android-tablets.h |   7 +-
 6 files changed, 124 insertions(+), 77 deletions(-)

-- 
2.41.0

[PATCH 1/8] gpiolib: acpi: Check if a GPIO is listed in ignore_interrupt earlier

[Hans de Goede]

In some cases where a broken AEI is present for a GPIO and the GPIO
is listed in the ignore_interrupt list to avoid the broken event
handler, the kernel may want to use the GPIO for another purpose.

Before this change trying to use such a GPIO for another purpose would
fail, because the ignore_interrupt list was only checked after
the acpi_request_own_gpiod() call, causing the GPIO to already be
claimed even though it is listed in the ignore_interrupt list.

Fix this by moving the ignore_interrupt list to above
the acpi_request_own_gpiod() call.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpiolib-acpi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 97496c0f9133..80b9650a2424 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -437,6 +437,11 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
 	if (!handler)
 		return AE_OK;
 
+	if (acpi_gpio_in_ignore_list(ignore_interrupt, dev_name(chip->parent), pin)) {
+		dev_info(chip->parent, "Ignoring interrupt on pin %u\n", pin);
+		return AE_OK;
+	}
+
 	desc = acpi_request_own_gpiod(chip, agpio, 0, "ACPI:Event");
 	if (IS_ERR(desc)) {
 		dev_err(chip->parent,
@@ -461,11 +466,6 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
 		goto fail_unlock_irq;
 	}
 
-	if (acpi_gpio_in_ignore_list(ignore_interrupt, dev_name(chip->parent), pin)) {
-		dev_info(chip->parent, "Ignoring interrupt on pin %u\n", pin);
-		return AE_OK;
-	}
-
 	event = kzalloc(sizeof(*event), GFP_KERNEL);
 	if (!event)
 		goto fail_unlock_irq;
-- 
2.41.0

Re: [PATCH 1/8] gpiolib: acpi: Check if a GPIO is listed in ignore_interrupt earlier

[Andy Shevchenko]

On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> In some cases where a broken AEI is present for a GPIO and the GPIO
> is listed in the ignore_interrupt list to avoid the broken event
> handler, the kernel may want to use the GPIO for another purpose.
>
> Before this change trying to use such a GPIO for another purpose would
> fail, because the ignore_interrupt list was only checked after
> the acpi_request_own_gpiod() call, causing the GPIO to already be
> claimed even though it is listed in the ignore_interrupt list.
>
> Fix this by moving the ignore_interrupt list to above
> the acpi_request_own_gpiod() call.

Not sure if we need a Fixes tag, either way this makes sense,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/8] gpiolib: acpi: Check if a GPIO is listed in ignore_interrupt earlier

[Mika Westerberg]

On Sat, Sep 09, 2023 at 04:18:09PM +0200, Hans de Goede wrote:
> In some cases where a broken AEI is present for a GPIO and the GPIO
> is listed in the ignore_interrupt list to avoid the broken event
> handler, the kernel may want to use the GPIO for another purpose.
> 
> Before this change trying to use such a GPIO for another purpose would
> fail, because the ignore_interrupt list was only checked after
> the acpi_request_own_gpiod() call, causing the GPIO to already be
> claimed even though it is listed in the ignore_interrupt list.
> 
> Fix this by moving the ignore_interrupt list to above
> the acpi_request_own_gpiod() call.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 1/8] gpiolib: acpi: Check if a GPIO is listed in ignore_interrupt earlier

[Bartosz Golaszewski]

On Sat, Sep 9, 2023 at 4:18 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> In some cases where a broken AEI is present for a GPIO and the GPIO
> is listed in the ignore_interrupt list to avoid the broken event
> handler, the kernel may want to use the GPIO for another purpose.
>
> Before this change trying to use such a GPIO for another purpose would
> fail, because the ignore_interrupt list was only checked after
> the acpi_request_own_gpiod() call, causing the GPIO to already be
> claimed even though it is listed in the ignore_interrupt list.
>
> Fix this by moving the ignore_interrupt list to above
> the acpi_request_own_gpiod() call.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH 2/8] gpiolib: acpi: Add a ignore interrupt quirk for Peaq C1010

[Hans de Goede]

On the Peaq C1010 2-in-1 INT33FC:00 pin 3 is connected to
a "dolby" button. At the ACPI level an _AEI event-handler
is connected which sets an ACPI variable to 1 on both
edges. This variable can be polled + cleared to 0 using WMI.

Since the variable is set on both edges the WMI interface is pretty
useless even when polling. So instead of writing a custom WMI
driver for this the x86-android-tablets code instantiates
a gpio-keys platform device for the "dolby" button.

Add an ignore_interrupt quirk for INT33FC:00 pin 3 on the Peaq C1010,
so that it is not seen as busy when the gpio-keys driver requests it.

Note this replaces a hack in x86-android-tablets where it would
call acpi_gpiochip_free_interrupts() on the INT33FC:00 GPIO
controller. acpi_gpiochip_free_interrupts() is considered private
(internal) gpiolib API so x86-android-tablets should stop using it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpiolib-acpi.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 80b9650a2424..583ac5da9d41 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1654,6 +1654,26 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] __initconst = {
 			.ignore_wake = "SYNA1202:00@16",
 		},
 	},
+	{
+		/*
+		 * On the Peaq C1010 2-in-1 INT33FC:00 pin 3 is connected to
+		 * a "dolby" button. At the ACPI level an _AEI event-handler
+		 * is connected which sets an ACPI variable to 1 on both
+		 * edges. This variable can be polled + cleared to 0 using
+		 * WMI. But since the variable is set on both edges the WMI
+		 * interface is pretty useless even when polling.
+		 * So instead the x86-android-tablets code instantiates
+		 * a gpio-keys platform device for it.
+		 * Ignore the _AEI handler for the pin, so that it is not busy.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PEAQ"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "PEAQ PMM C1010 MD99187"),
+		},
+		.driver_data = &(struct acpi_gpiolib_dmi_quirk) {
+			.ignore_interrupt = "INT33FC:00@3",
+		},
+	},
 	{} /* Terminating entry */
 };
 
-- 
2.41.0

Re: [PATCH 2/8] gpiolib: acpi: Add a ignore interrupt quirk for Peaq C1010

[Andy Shevchenko]

On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> On the Peaq C1010 2-in-1 INT33FC:00 pin 3 is connected to
> a "dolby" button. At the ACPI level an _AEI event-handler
> is connected which sets an ACPI variable to 1 on both
> edges. This variable can be polled + cleared to 0 using WMI.
>
> Since the variable is set on both edges the WMI interface is pretty
> useless even when polling. So instead of writing a custom WMI
> driver for this the x86-android-tablets code instantiates
> a gpio-keys platform device for the "dolby" button.
>
> Add an ignore_interrupt quirk for INT33FC:00 pin 3 on the Peaq C1010,
> so that it is not seen as busy when the gpio-keys driver requests it.
>
> Note this replaces a hack in x86-android-tablets where it would
> call acpi_gpiochip_free_interrupts() on the INT33FC:00 GPIO
> controller. acpi_gpiochip_free_interrupts() is considered private
> (internal) gpiolib API so x86-android-tablets should stop using it.

Yeah, OEMs often don't know what they are doing in firmwares...
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/8] gpiolib: acpi: Add a ignore interrupt quirk for Peaq C1010

[Mika Westerberg]

On Sat, Sep 09, 2023 at 04:18:10PM +0200, Hans de Goede wrote:
> On the Peaq C1010 2-in-1 INT33FC:00 pin 3 is connected to
> a "dolby" button. At the ACPI level an _AEI event-handler
> is connected which sets an ACPI variable to 1 on both
> edges. This variable can be polled + cleared to 0 using WMI.
> 
> Since the variable is set on both edges the WMI interface is pretty
> useless even when polling. So instead of writing a custom WMI
> driver for this the x86-android-tablets code instantiates
> a gpio-keys platform device for the "dolby" button.
> 
> Add an ignore_interrupt quirk for INT33FC:00 pin 3 on the Peaq C1010,
> so that it is not seen as busy when the gpio-keys driver requests it.
> 
> Note this replaces a hack in x86-android-tablets where it would
> call acpi_gpiochip_free_interrupts() on the INT33FC:00 GPIO
> controller. acpi_gpiochip_free_interrupts() is considered private
> (internal) gpiolib API so x86-android-tablets should stop using it.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 2/8] gpiolib: acpi: Add a ignore interrupt quirk for Peaq C1010

[Bartosz Golaszewski]

On Sat, Sep 9, 2023 at 4:18 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> On the Peaq C1010 2-in-1 INT33FC:00 pin 3 is connected to
> a "dolby" button. At the ACPI level an _AEI event-handler
> is connected which sets an ACPI variable to 1 on both
> edges. This variable can be polled + cleared to 0 using WMI.
>
> Since the variable is set on both edges the WMI interface is pretty
> useless even when polling. So instead of writing a custom WMI
> driver for this the x86-android-tablets code instantiates
> a gpio-keys platform device for the "dolby" button.
>
> Add an ignore_interrupt quirk for INT33FC:00 pin 3 on the Peaq C1010,
> so that it is not seen as busy when the gpio-keys driver requests it.
>
> Note this replaces a hack in x86-android-tablets where it would
> call acpi_gpiochip_free_interrupts() on the INT33FC:00 GPIO
> controller. acpi_gpiochip_free_interrupts() is considered private
> (internal) gpiolib API so x86-android-tablets should stop using it.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH 3/8] platform/x86: x86-android-tablets: Remove invalid_aei_gpiochip from Peaq C1010

[Hans de Goede]

Remove the invalid_aei_gpiochip setting from the x86_dev_info
for the Peaq C1010.

This is no longer necessary since there now is a quirk to ignore
the "dolby" button GPIO in gpiolib_acpi_quirks[] in
drivers/gpio/gpiolib-acpi.c .

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/x86-android-tablets/other.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/platform/x86/x86-android-tablets/other.c b/drivers/platform/x86/x86-android-tablets/other.c
index e79549c6aae1..621ca1e54d1f 100644
--- a/drivers/platform/x86/x86-android-tablets/other.c
+++ b/drivers/platform/x86/x86-android-tablets/other.c
@@ -505,11 +505,6 @@ static const struct x86_gpio_button peaq_c1010_button __initconst = {
 const struct x86_dev_info peaq_c1010_info __initconst = {
 	.gpio_button = &peaq_c1010_button,
 	.gpio_button_count = 1,
-	/*
-	 * Move the ACPI event handler used by the broken WMI interface out of
-	 * the way. This is the only event handler on INT33FC:00.
-	 */
-	.invalid_aei_gpiochip = "INT33FC:00",
 };
 
 /*
-- 
2.41.0

Re: [PATCH 3/8] platform/x86: x86-android-tablets: Remove invalid_aei_gpiochip from Peaq C1010

[Bartosz Golaszewski]

On Sat, Sep 9, 2023 at 4:18 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Remove the invalid_aei_gpiochip setting from the x86_dev_info
> for the Peaq C1010.
>
> This is no longer necessary since there now is a quirk to ignore
> the "dolby" button GPIO in gpiolib_acpi_quirks[] in
> drivers/gpio/gpiolib-acpi.c .
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH 4/8] platform/x86: x86-android-tablets: Remove invalid_aei_gpiochip support

[Hans de Goede]

x86_dev_info.invalid_aei_gpiochip is no longer used by any boards
and the x86-android-tablets code should not use the gpiolib private
acpi_gpiochip_free_interrupts() function.

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>
---
 drivers/platform/x86/x86-android-tablets/core.c   | 15 ---------------
 .../x86/x86-android-tablets/x86-android-tablets.h |  1 -
 2 files changed, 16 deletions(-)

diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
index 2fd6060a31bb..ab8cf22ac5da 100644
--- a/drivers/platform/x86/x86-android-tablets/core.c
+++ b/drivers/platform/x86/x86-android-tablets/core.c
@@ -259,7 +259,6 @@ static __init int x86_android_tablet_init(void)
 {
 	const struct x86_dev_info *dev_info;
 	const struct dmi_system_id *id;
-	struct gpio_chip *chip;
 	int i, ret = 0;
 
 	id = dmi_first_match(x86_android_tablet_ids);
@@ -268,20 +267,6 @@ static __init int x86_android_tablet_init(void)
 
 	dev_info = id->driver_data;
 
-	/*
-	 * The broken DSDTs on these devices often also include broken
-	 * _AEI (ACPI Event Interrupt) handlers, disable these.
-	 */
-	if (dev_info->invalid_aei_gpiochip) {
-		chip = gpiochip_find(dev_info->invalid_aei_gpiochip,
-				     gpiochip_find_match_label);
-		if (!chip) {
-			pr_err("error cannot find GPIO chip %s\n", dev_info->invalid_aei_gpiochip);
-			return -ENODEV;
-		}
-		acpi_gpiochip_free_interrupts(chip);
-	}
-
 	/*
 	 * Since this runs from module_init() it cannot use -EPROBE_DEFER,
 	 * instead pre-load any modules which are listed as requirements.
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 e46e1128acc8..bf97fb84c0d4 100644
--- a/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
+++ b/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
@@ -66,7 +66,6 @@ struct x86_gpio_button {
 };
 
 struct x86_dev_info {
-	char *invalid_aei_gpiochip;
 	const char * const *modules;
 	const struct software_node *bat_swnode;
 	struct gpiod_lookup_table * const *gpiod_lookup_tables;
-- 
2.41.0

Re: [PATCH 4/8] platform/x86: x86-android-tablets: Remove invalid_aei_gpiochip support

[Andy Shevchenko]

On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> x86_dev_info.invalid_aei_gpiochip is no longer used by any boards
> and the x86-android-tablets code should not use the gpiolib private
> acpi_gpiochip_free_interrupts() function.

Shouldn't this patch remove the private header inclusion as well?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/8] platform/x86: x86-android-tablets: Remove invalid_aei_gpiochip support

[Andy Shevchenko]

On Sun, Sep 10, 2023 at 10:56 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > x86_dev_info.invalid_aei_gpiochip is no longer used by any boards
> > and the x86-android-tablets code should not use the gpiolib private
> > acpi_gpiochip_free_interrupts() function.
>
> Shouldn't this patch remove the private header inclusion as well?

Ah, it seems it uses more than that...

-- 
With Best Regards,
Andy Shevchenko

[PATCH 5/8] platform/x86: x86-android-tablets: Create a platform_device from module_init()

[Hans de Goede]

Create a platform_device from module_init() and change
x86_android_tablet_init() / cleanup() into platform_device
probe() and remove() functions.

This is a preparation patch for refactoring x86_android_tablet_get_gpiod()
to no longer use gpiolib private functions like gpiochip_find().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../platform/x86/x86-android-tablets/core.c   | 57 ++++++++++++++-----
 1 file changed, 44 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
index ab8cf22ac5da..3d3101b2848f 100644
--- a/drivers/platform/x86/x86-android-tablets/core.c
+++ b/drivers/platform/x86/x86-android-tablets/core.c
@@ -25,6 +25,8 @@
 #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);
@@ -224,7 +226,7 @@ static __init int x86_instantiate_serdev(const struct x86_serdev_info *info, int
 	return ret;
 }
 
-static void x86_android_tablet_cleanup(void)
+static void x86_android_tablet_remove(struct platform_device *pdev)
 {
 	int i;
 
@@ -255,7 +257,7 @@ static void x86_android_tablet_cleanup(void)
 	software_node_unregister(bat_swnode);
 }
 
-static __init int x86_android_tablet_init(void)
+static __init int x86_android_tablet_probe(struct platform_device *pdev)
 {
 	const struct x86_dev_info *dev_info;
 	const struct dmi_system_id *id;
@@ -266,6 +268,8 @@ static __init int x86_android_tablet_init(void)
 		return -ENODEV;
 
 	dev_info = id->driver_data;
+	/* Allow x86_android_tablet_device use before probe() exits */
+	x86_android_tablet_device = pdev;
 
 	/*
 	 * Since this runs from module_init() it cannot use -EPROBE_DEFER,
@@ -288,7 +292,7 @@ static __init int x86_android_tablet_init(void)
 	if (dev_info->init) {
 		ret = dev_info->init();
 		if (ret < 0) {
-			x86_android_tablet_cleanup();
+			x86_android_tablet_remove(pdev);
 			return ret;
 		}
 		exit_handler = dev_info->exit;
@@ -296,7 +300,7 @@ static __init int x86_android_tablet_init(void)
 
 	i2c_clients = kcalloc(dev_info->i2c_client_count, sizeof(*i2c_clients), GFP_KERNEL);
 	if (!i2c_clients) {
-		x86_android_tablet_cleanup();
+		x86_android_tablet_remove(pdev);
 		return -ENOMEM;
 	}
 
@@ -304,7 +308,7 @@ static __init int x86_android_tablet_init(void)
 	for (i = 0; i < i2c_client_count; i++) {
 		ret = x86_instantiate_i2c_client(dev_info, i);
 		if (ret < 0) {
-			x86_android_tablet_cleanup();
+			x86_android_tablet_remove(pdev);
 			return ret;
 		}
 	}
@@ -312,7 +316,7 @@ static __init int x86_android_tablet_init(void)
 	/* + 1 to make space for (optional) gpio_keys_button pdev */
 	pdevs = kcalloc(dev_info->pdev_count + 1, sizeof(*pdevs), GFP_KERNEL);
 	if (!pdevs) {
-		x86_android_tablet_cleanup();
+		x86_android_tablet_remove(pdev);
 		return -ENOMEM;
 	}
 
@@ -320,14 +324,14 @@ static __init int x86_android_tablet_init(void)
 	for (i = 0; i < pdev_count; i++) {
 		pdevs[i] = platform_device_register_full(&dev_info->pdev_info[i]);
 		if (IS_ERR(pdevs[i])) {
-			x86_android_tablet_cleanup();
+			x86_android_tablet_remove(pdev);
 			return PTR_ERR(pdevs[i]);
 		}
 	}
 
 	serdevs = kcalloc(dev_info->serdev_count, sizeof(*serdevs), GFP_KERNEL);
 	if (!serdevs) {
-		x86_android_tablet_cleanup();
+		x86_android_tablet_remove(pdev);
 		return -ENOMEM;
 	}
 
@@ -335,7 +339,7 @@ static __init int x86_android_tablet_init(void)
 	for (i = 0; i < serdev_count; i++) {
 		ret = x86_instantiate_serdev(&dev_info->serdev_info[i], i);
 		if (ret < 0) {
-			x86_android_tablet_cleanup();
+			x86_android_tablet_remove(pdev);
 			return ret;
 		}
 	}
@@ -346,7 +350,7 @@ static __init int x86_android_tablet_init(void)
 
 		buttons = kcalloc(dev_info->gpio_button_count, sizeof(*buttons), GFP_KERNEL);
 		if (!buttons) {
-			x86_android_tablet_cleanup();
+			x86_android_tablet_remove(pdev);
 			return -ENOMEM;
 		}
 
@@ -354,7 +358,7 @@ static __init int x86_android_tablet_init(void)
 			ret = x86_android_tablet_get_gpiod(dev_info->gpio_button[i].chip,
 							   dev_info->gpio_button[i].pin, &gpiod);
 			if (ret < 0) {
-				x86_android_tablet_cleanup();
+				x86_android_tablet_remove(pdev);
 				return ret;
 			}
 
@@ -369,7 +373,7 @@ static __init int x86_android_tablet_init(void)
 								  PLATFORM_DEVID_AUTO,
 								  &pdata, sizeof(pdata));
 		if (IS_ERR(pdevs[pdev_count])) {
-			x86_android_tablet_cleanup();
+			x86_android_tablet_remove(pdev);
 			return PTR_ERR(pdevs[pdev_count]);
 		}
 		pdev_count++;
@@ -378,8 +382,35 @@ static __init int x86_android_tablet_init(void)
 	return 0;
 }
 
+static struct platform_driver x86_android_tablet_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+	},
+	.remove_new = x86_android_tablet_remove,
+};
+
+static int __init x86_android_tablet_init(void)
+{
+	if (!dmi_first_match(x86_android_tablet_ids)) {
+		pr_err("error loaded on unknown tablet model\n");
+		return -ENODEV;
+	}
+
+	x86_android_tablet_device = platform_create_bundle(&x86_android_tablet_driver,
+						   x86_android_tablet_probe,
+						   NULL, 0, NULL, 0);
+
+	return PTR_ERR_OR_ZERO(x86_android_tablet_device);
+}
+
+static void __exit x86_android_tablet_exit(void)
+{
+	platform_device_unregister(x86_android_tablet_device);
+	platform_driver_unregister(&x86_android_tablet_driver);
+}
+
 module_init(x86_android_tablet_init);
-module_exit(x86_android_tablet_cleanup);
+module_exit(x86_android_tablet_exit);
 
 MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
 MODULE_DESCRIPTION("X86 Android tablets DSDT fixups driver");
-- 
2.41.0

Re: [PATCH 5/8] platform/x86: x86-android-tablets: Create a platform_device from module_init()

[Andy Shevchenko]

On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Create a platform_device from module_init() and change
> x86_android_tablet_init() / cleanup() into platform_device
> probe() and remove() functions.
>
> This is a preparation patch for refactoring x86_android_tablet_get_gpiod()
> to no longer use gpiolib private functions like gpiochip_find().

...

> +static int __init x86_android_tablet_init(void)
> +{

> +       if (!dmi_first_match(x86_android_tablet_ids)) {

Why do we need this? Module alias is based on DMI matching, is it
against manual loading?

> +               pr_err("error loaded on unknown tablet model\n");
> +               return -ENODEV;
> +       }
> +
> +       x86_android_tablet_device = platform_create_bundle(&x86_android_tablet_driver,
> +                                                  x86_android_tablet_probe,
> +                                                  NULL, 0, NULL, 0);

So, in case of manual loading, would it be harmful for non-listed platforms?

> +       return PTR_ERR_OR_ZERO(x86_android_tablet_device);
> +}
> +
> +static void __exit x86_android_tablet_exit(void)
> +{
> +       platform_device_unregister(x86_android_tablet_device);
> +       platform_driver_unregister(&x86_android_tablet_driver);
> +}

> +

Instead of adding this blank line you can move
module_init()/module_exit() closer to the respective callbacks.

>  module_init(x86_android_tablet_init);
> -module_exit(x86_android_tablet_cleanup);
> +module_exit(x86_android_tablet_exit);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/8] platform/x86: x86-android-tablets: Create a platform_device from module_init()

[Hans de Goede]

Hi,

On 9/10/23 10:07, Andy Shevchenko wrote:
> On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Create a platform_device from module_init() and change
>> x86_android_tablet_init() / cleanup() into platform_device
>> probe() and remove() functions.
>>
>> This is a preparation patch for refactoring x86_android_tablet_get_gpiod()
>> to no longer use gpiolib private functions like gpiochip_find().
> 
> ...
> 
>> +static int __init x86_android_tablet_init(void)
>> +{
> 
>> +       if (!dmi_first_match(x86_android_tablet_ids)) {
> 
> Why do we need this? Module alias is based on DMI matching, is it
> against manual loading?

Yes I added this to protect against manual loading.

> 
>> +               pr_err("error loaded on unknown tablet model\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       x86_android_tablet_device = platform_create_bundle(&x86_android_tablet_driver,
>> +                                                  x86_android_tablet_probe,
>> +                                                  NULL, 0, NULL, 0);
> 
> So, in case of manual loading, would it be harmful for non-listed platforms?

No this will not be harmful, x86_android_tablet_probe() also
checks the DMI table and it will return -ENODEV when there is
no match.

So we just end up with an unused x86_android_tablets platform-device
and otherwise no harm is done.

I guess my main reason here is to not change manual loading
behavior, before the entire insmod would fail since
module_init() would return -ENODEV, this preserves this
behavior.

But you are right that this check can be dropped without
any bad side-effects.

I'll drop the check before merging this.

> 
>> +       return PTR_ERR_OR_ZERO(x86_android_tablet_device);
>> +}
>> +
>> +static void __exit x86_android_tablet_exit(void)
>> +{
>> +       platform_device_unregister(x86_android_tablet_device);
>> +       platform_driver_unregister(&x86_android_tablet_driver);
>> +}
> 
>> +
> 
> Instead of adding this blank line you can move
> module_init()/module_exit() closer to the respective callbacks.

Ack, I'll fix this before merging this.

Regards,

Hans



> 
>>  module_init(x86_android_tablet_init);
>> -module_exit(x86_android_tablet_cleanup);
>> +module_exit(x86_android_tablet_exit);
> 

[PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Hans de Goede]

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);
 
-	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);
 
 /*
-- 
2.41.0

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Andy Shevchenko]

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;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Hans de Goede]

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

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Bartosz Golaszewski]

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

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Hans de Goede]

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

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Bartosz Golaszewski]

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

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Hans de Goede]

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

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Bartosz Golaszewski]

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);
>         }
>

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Hans de Goede]

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);
>>         }
>>
> 

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Bartosz Golaszewski]

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);
> >>         }
> >>
> >
>

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Hans de Goede]

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

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Bartosz Golaszewski]

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

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Hans de Goede]

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

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Bartosz Golaszewski]

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

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Hans de Goede]

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

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/8] platform/x86: x86-android-tablets: Stop using gpiolib private APIs

[Bartosz Golaszewski]

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

[PATCH 7/8] platform/x86: x86-android-tablets: Use platform-device as gpio-keys parent

[Hans de Goede]

Use the new x86-android-tablets platform-device as gpio-keys parent
to make it clear that this gpio-keys device was instantiated by
the x86-android-tablets driver.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/x86-android-tablets/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
index 673f3a14941b..b72bb0376fe5 100644
--- a/drivers/platform/x86/x86-android-tablets/core.c
+++ b/drivers/platform/x86/x86-android-tablets/core.c
@@ -378,7 +378,7 @@ static __init int x86_android_tablet_probe(struct platform_device *pdev)
 		pdata.buttons = buttons;
 		pdata.nbuttons = dev_info->gpio_button_count;
 
-		pdevs[pdev_count] = platform_device_register_data(NULL, "gpio-keys",
+		pdevs[pdev_count] = platform_device_register_data(&pdev->dev, "gpio-keys",
 								  PLATFORM_DEVID_AUTO,
 								  &pdata, sizeof(pdata));
 		if (IS_ERR(pdevs[pdev_count])) {
-- 
2.41.0

[PATCH 8/8] platform/x86: x86-android-tablets: Drop "linux,power-supply-name" from lenovo_yt3_bq25892_0_props[]

[Hans de Goede]

The "linux,power-supply-name" property is a left-over from an earlier
attempt to allow properties to specify the power_supply class-device name.

The patch to read this property never made it upstream (and is no longer
necessary). Drop the unused property.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/x86-android-tablets/lenovo.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/platform/x86/x86-android-tablets/lenovo.c b/drivers/platform/x86/x86-android-tablets/lenovo.c
index 35aa2968d726..5c803cdb5586 100644
--- a/drivers/platform/x86/x86-android-tablets/lenovo.c
+++ b/drivers/platform/x86/x86-android-tablets/lenovo.c
@@ -565,7 +565,6 @@ static const struct software_node fg_bq25890_1_supply_node = {
 /* bq25892 charger settings for the flat lipo battery behind the screen */
 static const struct property_entry lenovo_yt3_bq25892_0_props[] = {
 	PROPERTY_ENTRY_STRING_ARRAY("supplied-from", lenovo_yt3_bq25892_0_suppliers),
-	PROPERTY_ENTRY_STRING("linux,power-supply-name", "bq25892-second-chrg"),
 	PROPERTY_ENTRY_U32("linux,iinlim-percentage", 40),
 	PROPERTY_ENTRY_BOOL("linux,skip-reset"),
 	/* Values taken from Android Factory Image */
-- 
2.41.0

Re: [PATCH 0/8] x86-android-tablets: Stop using gpiolib private APIs

[Linus Walleij]

On Sat, Sep 9, 2023 at 4:18 PM Hans de Goede <hdegoede@redhat.com> wrote:

> As promised here is a patch-series to make the x86-android-tablets code
> stop using gpiolib private APIs.

This looks very helpful to me!
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij