[PATCH v2 1/2] platform/x86: Add x86-acpi-irq-helpers.h

[PATCH v2 1/2] platform/x86: Add x86-acpi-irq-helpers.h

[Hans de Goede]

Add helper code to get Linux IRQ numbers given a description of the IRQ
source (either IOAPIC index, or GPIO chip name + pin-number).

This is intended to be used to lookup Linux IRQ numbers in cases where the
ACPI description for a device somehow lacks this info. This is only meant
for use on x86 ACPI platforms.

This code is big/complex enough to warrant sharing, but too small to live
in its own module, therefor x86_acpi_irq_helper_get() is defined as
a static inline helper function.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- New patch in v2 of this patch-set
---
 drivers/platform/x86/x86-acpi-irq-helpers.h | 82 +++++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 drivers/platform/x86/x86-acpi-irq-helpers.h

diff --git a/drivers/platform/x86/x86-acpi-irq-helpers.h b/drivers/platform/x86/x86-acpi-irq-helpers.h
new file mode 100644
index 000000000000..2b3c02c47563
--- /dev/null
+++ b/drivers/platform/x86/x86-acpi-irq-helpers.h
@@ -0,0 +1,82 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Helper code to get Linux IRQ numbers given a description of the IRQ source
+ * (either IOAPIC index, or GPIO chip name + pin-number).
+ *
+ * This is intended to be used to lookup Linux IRQ numbers in cases where the
+ * ACPI description for a device somehow lacks this info. This is only meant
+ * for use on x86 ACPI platforms.
+ */
+
+#include <linux/acpi.h>
+#include <linux/irq.h>
+#include <linux/string.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+
+/* For gpio_get_desc which is EXPORT_SYMBOL_GPL() */
+#include "../../gpio/gpiolib.h"
+
+enum x86_acpi_irq_type {
+	X86_ACPI_IRQ_TYPE_NONE,
+	X86_ACPI_IRQ_TYPE_APIC,
+	X86_ACPI_IRQ_TYPE_GPIOINT,
+};
+
+struct x86_acpi_irq_data {
+	char *gpio_chip; /* GPIO chip label for X86_ACPI_IRQ_TYPE_GPIOINT */
+	enum x86_acpi_irq_type type;
+	int index;
+	int trigger;  /* ACPI_EDGE_SENSITIVE / ACPI_LEVEL_SENSITIVE */
+	int polarity; /* ACPI_ACTIVE_HIGH / ACPI_ACTIVE_LOW / ACPI_ACTIVE_BOTH */
+};
+
+static int x86_acpi_irq_helper_gpiochip_find(struct gpio_chip *gc, void *data)
+{
+	return gc->label && !strcmp(gc->label, data);
+}
+
+static inline int x86_acpi_irq_helper_get(const struct x86_acpi_irq_data *data)
+{
+	struct gpio_desc *gpiod;
+	struct gpio_chip *chip;
+	unsigned int irq_type;
+	int irq, ret;
+
+	switch (data->type) {
+	case X86_ACPI_IRQ_TYPE_APIC:
+		irq = acpi_register_gsi(NULL, data->index, data->trigger, data->polarity);
+		if (irq < 0)
+			pr_err("error %d getting APIC IRQ %d\n", irq, data->index);
+
+		return irq;
+	case X86_ACPI_IRQ_TYPE_GPIOINT:
+		/* Like acpi_dev_gpio_irq_get(), but without parsing ACPI resources */
+		chip = gpiochip_find(data->gpio_chip, x86_acpi_irq_helper_gpiochip_find);
+		if (!chip)
+			return -EPROBE_DEFER;
+
+		gpiod = gpiochip_get_desc(chip, data->index);
+		if (IS_ERR(gpiod)) {
+			ret = PTR_ERR(gpiod);
+			pr_err("error %d getting GPIO %s %d\n", ret,
+			       data->gpio_chip, data->index);
+			return ret;
+		}
+
+		irq = gpiod_to_irq(gpiod);
+		if (irq < 0) {
+			pr_err("error %d getting IRQ %s %d\n", irq,
+			       data->gpio_chip, data->index);
+			return irq;
+		}
+
+		irq_type = acpi_dev_get_irq_type(data->trigger, data->polarity);
+		if (irq_type != IRQ_TYPE_NONE && irq_type != irq_get_trigger_type(irq))
+			irq_set_irq_type(irq, irq_type);
+
+		return irq;
+	default:
+		return 0;
+	}
+}
-- 
2.33.1

[PATCH v2 2/2] platform/x86: x86-android-tablets: New driver for x86 Android tablets

[Hans de Goede]

x86 tablets which ship with Android as (part of) the factory image
typically have various problems with their DSDTs. The factory kernels
shipped on these devices typically have device addresses and GPIOs
hardcoded in the kernel, rather then specified in their DSDT.

With the DSDT containing a random collection of devices which may or
may not actually be present as well as missing devices which are
actually present.

This driver, which loads only on affected models based on DMI matching,
adds DMI based instantiating of kernel devices for devices which are
missing from the DSDT, fixing e.g. battery monitoring, touchpads and/or
accelerometers not working.

Note the Kconfig help text also refers to "various fixes" ATM there are
no such fixes, but there are also known cases where entries are present
in the DSDT but they contain bugs, such as missing/wrong GPIOs. The plan
is to also add fixes for things like this here in the future.

This is the least ugly option to get these devices to fully work and to
do so without adding any extra code to the main kernel image (vmlinuz)
when built as a module.

Link: https://lore.kernel.org/platform-driver-x86/20211031162428.22368-1-hdegoede@redhat.com/
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Use the new x86_acpi_irq_helper_get() helper
- Fixup / improve a couple of comments
---
 MAINTAINERS                                |   8 +
 drivers/platform/x86/Kconfig               |  17 ++
 drivers/platform/x86/Makefile              |   1 +
 drivers/platform/x86/x86-android-tablets.c | 253 +++++++++++++++++++++
 4 files changed, 279 insertions(+)
 create mode 100644 drivers/platform/x86/x86-android-tablets.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 665c43d31ba3..12fea6d92b72 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20732,6 +20732,14 @@ S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/mm
 F:	arch/x86/mm/
 
+X86 PLATFORM ANDROID TABLETS DSDT FIXUP DRIVER
+M:	Hans de Goede <hdegoede@redhat.com>
+L:	platform-driver-x86@vger.kernel.org
+S:	Maintained
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git
+F:	drivers/platform/x86/x86-acpi-irq-helpers.h
+F:	drivers/platform/x86/x86-android-tablets.c
+
 X86 PLATFORM DRIVERS
 M:	Hans de Goede <hdegoede@redhat.com>
 M:	Mark Gross <markgross@kernel.org>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 140a18064550..a36ae015c0c6 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1024,6 +1024,23 @@ config TOUCHSCREEN_DMI
 	  the OS-image for the device. This option supplies the missing info.
 	  Enable this for x86 tablets with Silead or Chipone touchscreens.
 
+config X86_ANDROID_TABLETS
+	tristate "X86 Android tablet support"
+	depends on I2C && ACPI
+	help
+	  X86 tablets which ship with Android as (part of) the factory image
+	  typically have various problems with their DSDTs. The factory kernels
+	  shipped on these devices typically have device addresses and GPIOs
+	  hardcoded in the kernel, rather then specified in their DSDT.
+
+	  With the DSDT containing a random collection of devices which may or
+	  may not actually be present. This driver contains various fixes for
+	  such tablets, including instantiating kernel devices for devices which
+	  are missing from the DSDT.
+
+	  If you have a x86 Android tablet say Y or M here, for a generic x86
+	  distro config say M here.
+
 config FW_ATTR_CLASS
 	tristate
 
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 0f3ef40634b3..bd20b435c22b 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -114,6 +114,7 @@ obj-$(CONFIG_I2C_MULTI_INSTANTIATE)	+= i2c-multi-instantiate.o
 obj-$(CONFIG_MLX_PLATFORM)		+= mlx-platform.o
 obj-$(CONFIG_TOUCHSCREEN_DMI)		+= touchscreen_dmi.o
 obj-$(CONFIG_WIRELESS_HOTKEY)		+= wireless-hotkey.o
+obj-$(CONFIG_X86_ANDROID_TABLETS)	+= x86-android-tablets.o
 
 # Intel uncore drivers
 obj-$(CONFIG_INTEL_IPS)				+= intel_ips.o
diff --git a/drivers/platform/x86/x86-android-tablets.c b/drivers/platform/x86/x86-android-tablets.c
new file mode 100644
index 000000000000..400186d31c45
--- /dev/null
+++ b/drivers/platform/x86/x86-android-tablets.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * DMI based code to deal with broken DSDTs on X86 tablets which ship with
+ * Android as (part of) the factory image. The factory kernels shipped on these
+ * devices typically have a bunch of things hardcoded, rather than specified
+ * in their DSDT.
+ *
+ * Copyright (C) 2021 Hans de Goede <hdegoede@redhat.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/machine.h>
+#include <linux/i2c.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+
+#include "x86-acpi-irq-helpers.h"
+
+struct x86_i2c_client_info {
+	struct i2c_board_info board_info;
+	char *adapter_path;
+	struct x86_acpi_irq_data irq_data;
+};
+
+struct x86_dev_info {
+	const struct x86_i2c_client_info *i2c_client_info;
+	int i2c_client_count;
+};
+
+int i2c_client_count;
+static struct i2c_client **i2c_clients;
+
+/*
+ * When booted with the BIOS set to Android mode the Chuwi Hi8 (CWI509) DSDT
+ * contains a whole bunch of bogus ACPI I2C devices and is missing entries
+ * for the touchscreen and the accelerometer.
+ */
+static const struct property_entry chuwi_hi8_gsl1680_props[] = {
+	PROPERTY_ENTRY_U32("touchscreen-size-x", 1665),
+	PROPERTY_ENTRY_U32("touchscreen-size-y", 1140),
+	PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
+	PROPERTY_ENTRY_BOOL("silead,home-button"),
+	PROPERTY_ENTRY_STRING("firmware-name", "gsl1680-chuwi-hi8.fw"),
+	{ }
+};
+
+static const struct software_node chuwi_hi8_gsl1680_node = {
+	.properties = chuwi_hi8_gsl1680_props,
+};
+
+static const char * const chuwi_hi8_mount_matrix[] = {
+	"1", "0", "0",
+	"0", "-1", "0",
+	"0", "0", "1"
+};
+
+static const struct property_entry chuwi_hi8_bma250e_props[] = {
+	PROPERTY_ENTRY_STRING_ARRAY("mount-matrix", chuwi_hi8_mount_matrix),
+	{ }
+};
+
+static const struct software_node chuwi_hi8_bma250e_node = {
+	.properties = chuwi_hi8_bma250e_props,
+};
+
+static const struct x86_i2c_client_info chuwi_hi8_i2c_clients[] __initconst = {
+	{	/* Silead touchscreen */
+		.board_info = {
+			.type = "gsl1680",
+			.addr = 0x40,
+			.swnode = &chuwi_hi8_gsl1680_node,
+		},
+		.adapter_path = "\\_SB_.I2C4",
+		.irq_data = {
+			.type = X86_ACPI_IRQ_TYPE_APIC,
+			.index = 0x44,
+			.trigger = ACPI_EDGE_SENSITIVE,
+			.polarity = ACPI_ACTIVE_HIGH,
+		},
+	},
+	{	/* BMA250E accelerometer */
+		.board_info = {
+			.type = "bma250e",
+			.addr = 0x18,
+			.swnode = &chuwi_hi8_bma250e_node,
+		},
+		.adapter_path = "\\_SB_.I2C3",
+		.irq_data = {
+			.type = X86_ACPI_IRQ_TYPE_GPIOINT,
+			.gpio_chip = "INT33FC:02",
+			.index = 23,
+			.trigger = ACPI_LEVEL_SENSITIVE,
+			.polarity = ACPI_ACTIVE_HIGH,
+		},
+	},
+};
+
+static const struct x86_dev_info chuwi_hi8_info __initconst = {
+	.i2c_client_info = chuwi_hi8_i2c_clients,
+	.i2c_client_count = ARRAY_SIZE(chuwi_hi8_i2c_clients),
+};
+
+/*
+ * If the EFI bootloader is not Xiaomi's own signed Android loader, then the
+ * Xiaomi Mi Pad 2 X86 tablet sets OSID in the DSDT to 1 (Windows), causing
+ * a bunch of devices to be hidden.
+ *
+ * This takes care of instantiating the hidden devices manually.
+ */
+static const char * const bq27520_suppliers[] = { "bq25890-charger" };
+
+static const struct property_entry bq27520_props[] = {
+	PROPERTY_ENTRY_STRING_ARRAY("supplied-from", bq27520_suppliers),
+	{ }
+};
+
+static const struct software_node bq27520_node = {
+	.properties = bq27520_props,
+};
+
+static const struct x86_i2c_client_info xiaomi_mipad2_i2c_clients[] __initconst = {
+	{	/* BQ27520 fuel-gauge */
+		.board_info = {
+			.type = "bq27520",
+			.addr = 0x55,
+			.dev_name = "bq27520",
+			.swnode = &bq27520_node,
+		},
+		.adapter_path = "\\_SB_.PCI0.I2C1",
+	}, {	/* KTD2026 RGB notification LED controller */
+		.board_info = {
+			.type = "ktd2026",
+			.addr = 0x30,
+			.dev_name = "ktd2026",
+		},
+		.adapter_path = "\\_SB_.PCI0.I2C3",
+	}
+};
+
+static const struct x86_dev_info xiaomi_mipad2_info __initconst = {
+	.i2c_client_info = xiaomi_mipad2_i2c_clients,
+	.i2c_client_count = ARRAY_SIZE(xiaomi_mipad2_i2c_clients),
+};
+
+static const struct dmi_system_id x86_android_tablet_ids[] __initconst = {
+	{	/* Xiaomi Mi Pad 2 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Xiaomi Inc"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Mipad2"),
+		},
+		.driver_data = (void *)&xiaomi_mipad2_info,
+	}, {
+		/* Chuwi Hi8 (CWI509) */
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Hampoo"),
+			DMI_MATCH(DMI_BOARD_NAME, "BYT-PA03C"),
+			DMI_MATCH(DMI_SYS_VENDOR, "ilife"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "S806"),
+		},
+		.driver_data = (void *)&chuwi_hi8_info,
+	},
+	{} /* Terminating entry */
+};
+MODULE_DEVICE_TABLE(dmi, x86_android_tablet_ids);
+
+static __init int x86_instantiate_i2c_client(const struct x86_dev_info *dev_info,
+					     int idx)
+{
+	const struct x86_i2c_client_info *client_info = &dev_info->i2c_client_info[idx];
+	struct i2c_board_info board_info = client_info->board_info;
+	struct i2c_adapter *adap;
+	acpi_handle handle;
+	acpi_status status;
+	int ret = 0;
+
+	status = acpi_get_handle(NULL, client_info->adapter_path, &handle);
+	if (ACPI_FAILURE(status)) {
+		pr_err("Error could not get %s handle\n", client_info->adapter_path);
+		return -ENODEV;
+	}
+
+	adap = i2c_acpi_find_adapter_by_handle(handle);
+	if (!adap) {
+		pr_err("Error could not get %s adapter\n", client_info->adapter_path);
+		return -ENODEV;
+	}
+
+	board_info.irq = x86_acpi_irq_helper_get(&client_info->irq_data);
+	if (board_info.irq < 0) {
+		ret = board_info.irq;
+		goto out;
+	}
+
+	i2c_clients[idx] = i2c_new_client_device(adap, &board_info);
+	if (IS_ERR(i2c_clients[idx])) {
+		ret = PTR_ERR(i2c_clients[idx]);
+		pr_err("Error creating I2C-client %d: %d\n", idx, ret);
+	}
+out:
+	put_device(&adap->dev);
+	return ret;
+}
+
+static void x86_android_tablet_cleanup(void)
+{
+	int i;
+
+	for (i = 0; i < i2c_client_count; i++)
+		i2c_unregister_device(i2c_clients[i]);
+
+	kfree(i2c_clients);
+}
+
+static __init int x86_android_tablet_init(void)
+{
+	const struct x86_dev_info *dev_info;
+	const struct dmi_system_id *id;
+	int i, ret = 0;
+
+	id = dmi_first_match(x86_android_tablet_ids);
+	if (!id)
+		return -ENODEV;
+
+	dev_info = id->driver_data;
+
+	i2c_client_count = dev_info->i2c_client_count;
+
+	i2c_clients = kcalloc(i2c_client_count, sizeof(*i2c_clients), GFP_KERNEL);
+	if (!i2c_clients)
+		return -ENOMEM;
+
+	for (i = 0; i < dev_info->i2c_client_count; i++) {
+		ret = x86_instantiate_i2c_client(dev_info, i);
+		if (ret < 0) {
+			x86_android_tablet_cleanup();
+			break;
+		}
+	}
+
+	return ret;
+}
+
+module_init(x86_android_tablet_init);
+module_exit(x86_android_tablet_cleanup);
+
+MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com");
+MODULE_DESCRIPTION("X86 Android tablets DSDT fixups driver");
+MODULE_LICENSE("GPL");
-- 
2.33.1

Re: [PATCH v2 2/2] platform/x86: x86-android-tablets: New driver for x86 Android tablets

[Andy Shevchenko]

On Tue, Dec 21, 2021 at 5:13 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> x86 tablets which ship with Android as (part of) the factory image
> typically have various problems with their DSDTs. The factory kernels
> shipped on these devices typically have device addresses and GPIOs
> hardcoded in the kernel, rather then specified in their DSDT.
>
> With the DSDT containing a random collection of devices which may or
> may not actually be present as well as missing devices which are
> actually present.
>
> This driver, which loads only on affected models based on DMI matching,
> adds DMI based instantiating of kernel devices for devices which are
> missing from the DSDT, fixing e.g. battery monitoring, touchpads and/or
> accelerometers not working.
>
> Note the Kconfig help text also refers to "various fixes" ATM there are
> no such fixes, but there are also known cases where entries are present
> in the DSDT but they contain bugs, such as missing/wrong GPIOs. The plan
> is to also add fixes for things like this here in the future.
>
> This is the least ugly option to get these devices to fully work and to
> do so without adding any extra code to the main kernel image (vmlinuz)
> when built as a module.

...

> +config X86_ANDROID_TABLETS
> +       tristate "X86 Android tablet support"
> +       depends on I2C && ACPI
> +       help
> +         X86 tablets which ship with Android as (part of) the factory image
> +         typically have various problems with their DSDTs. The factory kernels
> +         shipped on these devices typically have device addresses and GPIOs
> +         hardcoded in the kernel, rather then specified in their DSDT.

than

> +
> +         With the DSDT containing a random collection of devices which may or
> +         may not actually be present. This driver contains various fixes for
> +         such tablets, including instantiating kernel devices for devices which
> +         are missing from the DSDT.

...

> +static const char * const chuwi_hi8_mount_matrix[] = {
> +       "1", "0", "0",
> +       "0", "-1", "0",
> +       "0", "0", "1"

+ comma?

> +};

...

> +       int ret = 0;

> +       board_info.irq = x86_acpi_irq_helper_get(&client_info->irq_data);
> +       if (board_info.irq < 0) {
> +               ret = board_info.irq;
> +               goto out;
> +       }

Can we rather use
ret = ...
if (ret < 0)
 goto
.irq = ret;

?

...

> +       i2c_clients[idx] = i2c_new_client_device(adap, &board_info);
> +       if (IS_ERR(i2c_clients[idx])) {

> +               ret = PTR_ERR(i2c_clients[idx]);
> +               pr_err("Error creating I2C-client %d: %d\n", idx, ret);

dev_err_probe()? (device of the adapter)

> +       }

...

> +out:

out_put_device: ?

> +       put_device(&adap->dev);
> +       return ret;

...

> +       int i, ret = 0;

Do you need this assignment? See below.

...

> +       for (i = 0; i < dev_info->i2c_client_count; i++) {
> +               ret = x86_instantiate_i2c_client(dev_info, i);
> +               if (ret < 0) {
> +                       x86_android_tablet_cleanup();

> +                       break;

return ret; ?

> +               }
> +       }

> +       return ret;

return 0; ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] platform/x86: x86-android-tablets: New driver for x86 Android tablets

[Hans de Goede]

Hi,

On 12/21/21 16:39, Andy Shevchenko wrote:
> On Tue, Dec 21, 2021 at 5:13 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> x86 tablets which ship with Android as (part of) the factory image
>> typically have various problems with their DSDTs. The factory kernels
>> shipped on these devices typically have device addresses and GPIOs
>> hardcoded in the kernel, rather then specified in their DSDT.
>>
>> With the DSDT containing a random collection of devices which may or
>> may not actually be present as well as missing devices which are
>> actually present.
>>
>> This driver, which loads only on affected models based on DMI matching,
>> adds DMI based instantiating of kernel devices for devices which are
>> missing from the DSDT, fixing e.g. battery monitoring, touchpads and/or
>> accelerometers not working.
>>
>> Note the Kconfig help text also refers to "various fixes" ATM there are
>> no such fixes, but there are also known cases where entries are present
>> in the DSDT but they contain bugs, such as missing/wrong GPIOs. The plan
>> is to also add fixes for things like this here in the future.
>>
>> This is the least ugly option to get these devices to fully work and to
>> do so without adding any extra code to the main kernel image (vmlinuz)
>> when built as a module.
> 
> ...
> 
>> +config X86_ANDROID_TABLETS
>> +       tristate "X86 Android tablet support"
>> +       depends on I2C && ACPI
>> +       help
>> +         X86 tablets which ship with Android as (part of) the factory image
>> +         typically have various problems with their DSDTs. The factory kernels
>> +         shipped on these devices typically have device addresses and GPIOs
>> +         hardcoded in the kernel, rather then specified in their DSDT.
> 
> than

Fixed.

> 
>> +
>> +         With the DSDT containing a random collection of devices which may or
>> +         may not actually be present. This driver contains various fixes for
>> +         such tablets, including instantiating kernel devices for devices which
>> +         are missing from the DSDT.
> 
> ...
> 
>> +static const char * const chuwi_hi8_mount_matrix[] = {
>> +       "1", "0", "0",
>> +       "0", "-1", "0",
>> +       "0", "0", "1"
> 
> + comma?

This is a 3x3 matrix, there are never going to be extra entries
added, so no I don't think so.

> 
>> +};
> 
> ...
> 
>> +       int ret = 0;
> 
>> +       board_info.irq = x86_acpi_irq_helper_get(&client_info->irq_data);
>> +       if (board_info.irq < 0) {
>> +               ret = board_info.irq;
>> +               goto out;
>> +       }
> 
> Can we rather use
> ret = ...
> if (ret < 0)
>  goto
> .irq = ret;
> 
> ?

Ack, fixed.

> 
> ...
> 
>> +       i2c_clients[idx] = i2c_new_client_device(adap, &board_info);
>> +       if (IS_ERR(i2c_clients[idx])) {
> 
>> +               ret = PTR_ERR(i2c_clients[idx]);
>> +               pr_err("Error creating I2C-client %d: %d\n", idx, ret);
> 
> dev_err_probe()? (device of the adapter)

Ack, fixed.


> 
>> +       }
> 
> ...
> 
>> +out:
> 
> out_put_device: ?

I've gone with out_put_adap instead.

Regards,

Hans


> 
>> +       put_device(&adap->dev);
>> +       return ret;
> 
> ...
> 
>> +       int i, ret = 0;
> 
> Do you need this assignment? See below.
> 
> ...
> 
>> +       for (i = 0; i < dev_info->i2c_client_count; i++) {
>> +               ret = x86_instantiate_i2c_client(dev_info, i);
>> +               if (ret < 0) {
>> +                       x86_android_tablet_cleanup();
> 
>> +                       break;
> 
> return ret; ?
> 
>> +               }
>> +       }
> 
>> +       return ret;
> 
> return 0; ?
> 

Re: [PATCH v2 2/2] platform/x86: x86-android-tablets: New driver for x86 Android tablets

[Hans de Goede]

Hi,

On 12/21/21 20:05, Hans de Goede wrote:
> Hi,
> 
> On 12/21/21 16:39, Andy Shevchenko wrote:
>> On Tue, Dec 21, 2021 at 5:13 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> x86 tablets which ship with Android as (part of) the factory image
>>> typically have various problems with their DSDTs. The factory kernels
>>> shipped on these devices typically have device addresses and GPIOs
>>> hardcoded in the kernel, rather then specified in their DSDT.
>>>
>>> With the DSDT containing a random collection of devices which may or
>>> may not actually be present as well as missing devices which are
>>> actually present.
>>>
>>> This driver, which loads only on affected models based on DMI matching,
>>> adds DMI based instantiating of kernel devices for devices which are
>>> missing from the DSDT, fixing e.g. battery monitoring, touchpads and/or
>>> accelerometers not working.
>>>
>>> Note the Kconfig help text also refers to "various fixes" ATM there are
>>> no such fixes, but there are also known cases where entries are present
>>> in the DSDT but they contain bugs, such as missing/wrong GPIOs. The plan
>>> is to also add fixes for things like this here in the future.
>>>
>>> This is the least ugly option to get these devices to fully work and to
>>> do so without adding any extra code to the main kernel image (vmlinuz)
>>> when built as a module.
>>
>> ...
>>
>>> +config X86_ANDROID_TABLETS
>>> +       tristate "X86 Android tablet support"
>>> +       depends on I2C && ACPI
>>> +       help
>>> +         X86 tablets which ship with Android as (part of) the factory image
>>> +         typically have various problems with their DSDTs. The factory kernels
>>> +         shipped on these devices typically have device addresses and GPIOs
>>> +         hardcoded in the kernel, rather then specified in their DSDT.
>>
>> than
> 
> Fixed.
> 
>>
>>> +
>>> +         With the DSDT containing a random collection of devices which may or
>>> +         may not actually be present. This driver contains various fixes for
>>> +         such tablets, including instantiating kernel devices for devices which
>>> +         are missing from the DSDT.
>>
>> ...
>>
>>> +static const char * const chuwi_hi8_mount_matrix[] = {
>>> +       "1", "0", "0",
>>> +       "0", "-1", "0",
>>> +       "0", "0", "1"
>>
>> + comma?
> 
> This is a 3x3 matrix, there are never going to be extra entries
> added, so no I don't think so.
> 
>>
>>> +};
>>
>> ...
>>
>>> +       int ret = 0;
>>
>>> +       board_info.irq = x86_acpi_irq_helper_get(&client_info->irq_data);
>>> +       if (board_info.irq < 0) {
>>> +               ret = board_info.irq;
>>> +               goto out;
>>> +       }
>>
>> Can we rather use
>> ret = ...
>> if (ret < 0)
>>  goto
>> .irq = ret;
>>
>> ?
> 
> Ack, fixed.

Scrap that, this way the function will now return the Linux IRQ number on
success instead of 0.

Instead I've ended up refactoring things so that I don't need a ret variable;
nor a goto at all anymore (by getting the irq first).

Regards,

Hans




> 
>>
>> ...
>>
>>> +       i2c_clients[idx] = i2c_new_client_device(adap, &board_info);
>>> +       if (IS_ERR(i2c_clients[idx])) {
>>
>>> +               ret = PTR_ERR(i2c_clients[idx]);
>>> +               pr_err("Error creating I2C-client %d: %d\n", idx, ret);
>>
>> dev_err_probe()? (device of the adapter)
> 
> Ack, fixed.
> 
> 
>>
>>> +       }
>>
>> ...
>>
>>> +out:
>>
>> out_put_device: ?
> 
> I've gone with out_put_adap instead.
> 
> Regards,
> 
> Hans
> 
> 
>>
>>> +       put_device(&adap->dev);
>>> +       return ret;
>>
>> ...
>>
>>> +       int i, ret = 0;
>>
>> Do you need this assignment? See below.
>>
>> ...
>>
>>> +       for (i = 0; i < dev_info->i2c_client_count; i++) {
>>> +               ret = x86_instantiate_i2c_client(dev_info, i);
>>> +               if (ret < 0) {
>>> +                       x86_android_tablet_cleanup();
>>
>>> +                       break;
>>
>> return ret; ?
>>
>>> +               }
>>> +       }
>>
>>> +       return ret;
>>
>> return 0; ?
>>

Re: [PATCH v2 1/2] platform/x86: Add x86-acpi-irq-helpers.h

[Andy Shevchenko]

On Tue, Dec 21, 2021 at 5:13 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Add helper code to get Linux IRQ numbers given a description of the IRQ
> source (either IOAPIC index, or GPIO chip name + pin-number).
>
> This is intended to be used to lookup Linux IRQ numbers in cases where the
> ACPI description for a device somehow lacks this info. This is only meant
> for use on x86 ACPI platforms.
>
> This code is big/complex enough to warrant sharing, but too small to live
> in its own module, therefor x86_acpi_irq_helper_get() is defined as
> a static inline helper function.

...

> +/* For gpio_get_desc which is EXPORT_SYMBOL_GPL() */

gpio_get_desc() and honestly I don't like this kind of includes (yes,
I know sometimes it's the best compromise).

> +#include "../../gpio/gpiolib.h"

...

> +               /* Like acpi_dev_gpio_irq_get(), but without parsing ACPI resources */
> +               chip = gpiochip_find(data->gpio_chip, x86_acpi_irq_helper_gpiochip_find);
> +               if (!chip)
> +                       return -EPROBE_DEFER;
> +
> +               gpiod = gpiochip_get_desc(chip, data->index);
> +               if (IS_ERR(gpiod)) {
> +                       ret = PTR_ERR(gpiod);
> +                       pr_err("error %d getting GPIO %s %d\n", ret,
> +                              data->gpio_chip, data->index);
> +                       return ret;
> +               }
> +
> +               irq = gpiod_to_irq(gpiod);
> +               if (irq < 0) {
> +                       pr_err("error %d getting IRQ %s %d\n", irq,
> +                              data->gpio_chip, data->index);
> +                       return irq;
> +               }
> +
> +               irq_type = acpi_dev_get_irq_type(data->trigger, data->polarity);
> +               if (irq_type != IRQ_TYPE_NONE && irq_type != irq_get_trigger_type(irq))
> +                       irq_set_irq_type(irq, irq_type);
> +
> +               return irq;

I'm wondering why it can't be a part of the GPIO ACPI library?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] platform/x86: Add x86-acpi-irq-helpers.h

[Hans de Goede]

Hi,

On 12/21/21 16:27, Andy Shevchenko wrote:
> On Tue, Dec 21, 2021 at 5:13 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Add helper code to get Linux IRQ numbers given a description of the IRQ
>> source (either IOAPIC index, or GPIO chip name + pin-number).
>>
>> This is intended to be used to lookup Linux IRQ numbers in cases where the
>> ACPI description for a device somehow lacks this info. This is only meant
>> for use on x86 ACPI platforms.
>>
>> This code is big/complex enough to warrant sharing, but too small to live
>> in its own module, therefor x86_acpi_irq_helper_get() is defined as
>> a static inline helper function.
> 
> ...
> 
>> +/* For gpio_get_desc which is EXPORT_SYMBOL_GPL() */
> 
> gpio_get_desc()

Fixed in my local version.

> and honestly I don't like this kind of includes (yes,
> I know sometimes it's the best compromise).
> 
>> +#include "../../gpio/gpiolib.h"
> 
> ...
> 
>> +               /* Like acpi_dev_gpio_irq_get(), but without parsing ACPI resources */
>> +               chip = gpiochip_find(data->gpio_chip, x86_acpi_irq_helper_gpiochip_find);
>> +               if (!chip)
>> +                       return -EPROBE_DEFER;
>> +
>> +               gpiod = gpiochip_get_desc(chip, data->index);
>> +               if (IS_ERR(gpiod)) {
>> +                       ret = PTR_ERR(gpiod);
>> +                       pr_err("error %d getting GPIO %s %d\n", ret,
>> +                              data->gpio_chip, data->index);
>> +                       return ret;
>> +               }
>> +
>> +               irq = gpiod_to_irq(gpiod);
>> +               if (irq < 0) {
>> +                       pr_err("error %d getting IRQ %s %d\n", irq,
>> +                              data->gpio_chip, data->index);
>> +                       return irq;
>> +               }
>> +
>> +               irq_type = acpi_dev_get_irq_type(data->trigger, data->polarity);
>> +               if (irq_type != IRQ_TYPE_NONE && irq_type != irq_get_trigger_type(irq))
>> +                       irq_set_irq_type(irq, irq_type);
>> +
>> +               return irq;
> 
> I'm wondering why it can't be a part of the GPIO ACPI library?

Interesting suggestion, but this really is only intended for the
special case when the DSDT is missing this info. I'm a bit worried
that having this available as a generic helper may lead to it
getting used too much. But I guess we can just put a comment on it
explaining that normally its use should be avoided.

I've added Mika to the Cc, Mika, what do you think about adding this
as a new helper to the GPIO ACPI library ?

Regards,

Hans

Re: [PATCH v2 1/2] platform/x86: Add x86-acpi-irq-helpers.h

[Mika Westerberg]

Hi,

On Tue, Dec 21, 2021 at 07:58:26PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 12/21/21 16:27, Andy Shevchenko wrote:
> > On Tue, Dec 21, 2021 at 5:13 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Add helper code to get Linux IRQ numbers given a description of the IRQ
> >> source (either IOAPIC index, or GPIO chip name + pin-number).
> >>
> >> This is intended to be used to lookup Linux IRQ numbers in cases where the
> >> ACPI description for a device somehow lacks this info. This is only meant
> >> for use on x86 ACPI platforms.
> >>
> >> This code is big/complex enough to warrant sharing, but too small to live
> >> in its own module, therefor x86_acpi_irq_helper_get() is defined as
> >> a static inline helper function.
> > 
> > ...
> > 
> >> +/* For gpio_get_desc which is EXPORT_SYMBOL_GPL() */
> > 
> > gpio_get_desc()
> 
> Fixed in my local version.
> 
> > and honestly I don't like this kind of includes (yes,
> > I know sometimes it's the best compromise).
> > 
> >> +#include "../../gpio/gpiolib.h"
> > 
> > ...
> > 
> >> +               /* Like acpi_dev_gpio_irq_get(), but without parsing ACPI resources */
> >> +               chip = gpiochip_find(data->gpio_chip, x86_acpi_irq_helper_gpiochip_find);
> >> +               if (!chip)
> >> +                       return -EPROBE_DEFER;
> >> +
> >> +               gpiod = gpiochip_get_desc(chip, data->index);
> >> +               if (IS_ERR(gpiod)) {
> >> +                       ret = PTR_ERR(gpiod);
> >> +                       pr_err("error %d getting GPIO %s %d\n", ret,
> >> +                              data->gpio_chip, data->index);
> >> +                       return ret;
> >> +               }
> >> +
> >> +               irq = gpiod_to_irq(gpiod);
> >> +               if (irq < 0) {
> >> +                       pr_err("error %d getting IRQ %s %d\n", irq,
> >> +                              data->gpio_chip, data->index);
> >> +                       return irq;
> >> +               }
> >> +
> >> +               irq_type = acpi_dev_get_irq_type(data->trigger, data->polarity);
> >> +               if (irq_type != IRQ_TYPE_NONE && irq_type != irq_get_trigger_type(irq))
> >> +                       irq_set_irq_type(irq, irq_type);
> >> +
> >> +               return irq;
> > 
> > I'm wondering why it can't be a part of the GPIO ACPI library?
> 
> Interesting suggestion, but this really is only intended for the
> special case when the DSDT is missing this info. I'm a bit worried
> that having this available as a generic helper may lead to it
> getting used too much. But I guess we can just put a comment on it
> explaining that normally its use should be avoided.
> 
> I've added Mika to the Cc, Mika, what do you think about adding this
> as a new helper to the GPIO ACPI library ?

Preferably no :-) Reason is that even if we add comment to the function
you don't remember it after two weeks so the next patch adding another
user will not be noticed by reviewers (unless tha name of the function
clearly says it is a quirk/workaround).

Re: [PATCH v2 1/2] platform/x86: Add x86-acpi-irq-helpers.h

[Andy Shevchenko]

On Wed, Dec 22, 2021 at 2:47 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Dec 21, 2021 at 07:58:26PM +0100, Hans de Goede wrote:
> > On 12/21/21 16:27, Andy Shevchenko wrote:
> > > On Tue, Dec 21, 2021 at 5:13 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > >>
> > >> Add helper code to get Linux IRQ numbers given a description of the IRQ
> > >> source (either IOAPIC index, or GPIO chip name + pin-number).
> > >>
> > >> This is intended to be used to lookup Linux IRQ numbers in cases where the
> > >> ACPI description for a device somehow lacks this info. This is only meant
> > >> for use on x86 ACPI platforms.
> > >>
> > >> This code is big/complex enough to warrant sharing, but too small to live
> > >> in its own module, therefor x86_acpi_irq_helper_get() is defined as
> > >> a static inline helper function.
> > >
> > > ...
> > >
> > >> +/* For gpio_get_desc which is EXPORT_SYMBOL_GPL() */
> > >
> > > gpio_get_desc()
> >
> > Fixed in my local version.
> >
> > > and honestly I don't like this kind of includes (yes,
> > > I know sometimes it's the best compromise).
> > >
> > >> +#include "../../gpio/gpiolib.h"
> > >
> > > ...
> > >
> > >> +               /* Like acpi_dev_gpio_irq_get(), but without parsing ACPI resources */
> > >> +               chip = gpiochip_find(data->gpio_chip, x86_acpi_irq_helper_gpiochip_find);
> > >> +               if (!chip)
> > >> +                       return -EPROBE_DEFER;
> > >> +
> > >> +               gpiod = gpiochip_get_desc(chip, data->index);
> > >> +               if (IS_ERR(gpiod)) {
> > >> +                       ret = PTR_ERR(gpiod);
> > >> +                       pr_err("error %d getting GPIO %s %d\n", ret,
> > >> +                              data->gpio_chip, data->index);
> > >> +                       return ret;
> > >> +               }
> > >> +
> > >> +               irq = gpiod_to_irq(gpiod);
> > >> +               if (irq < 0) {
> > >> +                       pr_err("error %d getting IRQ %s %d\n", irq,
> > >> +                              data->gpio_chip, data->index);
> > >> +                       return irq;
> > >> +               }
> > >> +
> > >> +               irq_type = acpi_dev_get_irq_type(data->trigger, data->polarity);
> > >> +               if (irq_type != IRQ_TYPE_NONE && irq_type != irq_get_trigger_type(irq))
> > >> +                       irq_set_irq_type(irq, irq_type);
> > >> +
> > >> +               return irq;
> > >
> > > I'm wondering why it can't be a part of the GPIO ACPI library?
> >
> > Interesting suggestion, but this really is only intended for the
> > special case when the DSDT is missing this info. I'm a bit worried
> > that having this available as a generic helper may lead to it
> > getting used too much. But I guess we can just put a comment on it
> > explaining that normally its use should be avoided.
> >
> > I've added Mika to the Cc, Mika, what do you think about adding this
> > as a new helper to the GPIO ACPI library ?
>
> Preferably no :-) Reason is that even if we add comment to the function
> you don't remember it after two weeks so the next patch adding another
> user will not be noticed by reviewers (unless tha name of the function
> clearly says it is a quirk/workaround).

Oh, we have a solution for this already, it's called an export
namespace. We may export symbols in its own namespace and any user
must to import it. It will show immediately who is trying to do "bad
things". Code duplication makes kernel bigger and harder to maintain.
Imagine if there is an issue or refactoring happening in one copy of
the code and missed in the other. How long would it take to discover
that and fix it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] platform/x86: Add x86-acpi-irq-helpers.h

[Hans de Goede]

Hi,

On 12/22/21 13:45, Mika Westerberg wrote:
> Hi,
> 
> On Tue, Dec 21, 2021 at 07:58:26PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 12/21/21 16:27, Andy Shevchenko wrote:
>>> On Tue, Dec 21, 2021 at 5:13 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Add helper code to get Linux IRQ numbers given a description of the IRQ
>>>> source (either IOAPIC index, or GPIO chip name + pin-number).
>>>>
>>>> This is intended to be used to lookup Linux IRQ numbers in cases where the
>>>> ACPI description for a device somehow lacks this info. This is only meant
>>>> for use on x86 ACPI platforms.
>>>>
>>>> This code is big/complex enough to warrant sharing, but too small to live
>>>> in its own module, therefor x86_acpi_irq_helper_get() is defined as
>>>> a static inline helper function.
>>>
>>> ...
>>>
>>>> +/* For gpio_get_desc which is EXPORT_SYMBOL_GPL() */
>>>
>>> gpio_get_desc()
>>
>> Fixed in my local version.
>>
>>> and honestly I don't like this kind of includes (yes,
>>> I know sometimes it's the best compromise).
>>>
>>>> +#include "../../gpio/gpiolib.h"
>>>
>>> ...
>>>
>>>> +               /* Like acpi_dev_gpio_irq_get(), but without parsing ACPI resources */
>>>> +               chip = gpiochip_find(data->gpio_chip, x86_acpi_irq_helper_gpiochip_find);
>>>> +               if (!chip)
>>>> +                       return -EPROBE_DEFER;
>>>> +
>>>> +               gpiod = gpiochip_get_desc(chip, data->index);
>>>> +               if (IS_ERR(gpiod)) {
>>>> +                       ret = PTR_ERR(gpiod);
>>>> +                       pr_err("error %d getting GPIO %s %d\n", ret,
>>>> +                              data->gpio_chip, data->index);
>>>> +                       return ret;
>>>> +               }
>>>> +
>>>> +               irq = gpiod_to_irq(gpiod);
>>>> +               if (irq < 0) {
>>>> +                       pr_err("error %d getting IRQ %s %d\n", irq,
>>>> +                              data->gpio_chip, data->index);
>>>> +                       return irq;
>>>> +               }
>>>> +
>>>> +               irq_type = acpi_dev_get_irq_type(data->trigger, data->polarity);
>>>> +               if (irq_type != IRQ_TYPE_NONE && irq_type != irq_get_trigger_type(irq))
>>>> +                       irq_set_irq_type(irq, irq_type);
>>>> +
>>>> +               return irq;
>>>
>>> I'm wondering why it can't be a part of the GPIO ACPI library?
>>
>> Interesting suggestion, but this really is only intended for the
>> special case when the DSDT is missing this info. I'm a bit worried
>> that having this available as a generic helper may lead to it
>> getting used too much. But I guess we can just put a comment on it
>> explaining that normally its use should be avoided.
>>
>> I've added Mika to the Cc, Mika, what do you think about adding this
>> as a new helper to the GPIO ACPI library ?
> 
> Preferably no :-) Reason is that even if we add comment to the function
> you don't remember it after two weeks so the next patch adding another
> user will not be noticed by reviewers (unless tha name of the function
> clearly says it is a quirk/workaround).

Right, I also just found another, better way of dealing with the second
use-case for these helpers, which leaves only the upcoming
x86-android-tablets kernel module as user. So for version 3 of that module
I'm just going to fold these back into that module, since now that will
be the only user of this kludge.

Turning this into a separate helper did result in a nice cleanup, so I'm
going to keep things as is but just put them back into x86-android-tablets.c
keeping them as is will also make it easier to factor them out again
if that ever becomes necessary.

Regards,

Hans