* [PATCH] platform/x86: x86-android-tablets: New driver for x86 Android tablets
@ 2021-11-25 19:26 Hans de Goede
2021-11-25 19:43 ` Andy Shevchenko
0 siblings, 1 reply; 3+ messages in thread
From: Hans de Goede @ 2021-11-25 19:26 UTC (permalink / raw
To: Mark Gross, Andy Shevchenko
Cc: Hans de Goede, Tsuchiya Yuto, platform-driver-x86
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>
---
drivers/platform/x86/Kconfig | 17 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/x86-android-tablets.c | 315 +++++++++++++++++++++
3 files changed, 333 insertions(+)
create mode 100644 drivers/platform/x86/x86-android-tablets.c
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 27108c0e855f..a1dd5e0676d1 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1006,6 +1006,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 dfb7ca88f012..30047df8f701 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -113,6 +113,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..526c6a5ff7da
--- /dev/null
+++ b/drivers/platform/x86/x86-android-tablets.c
@@ -0,0 +1,315 @@
+// 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 then specified
+ * in their DSDT.
+ *
+ * Copyright (C) 2021 Hans de Goede <hdegoede@redhat.com>
+ */
+
+#define pr_fmt(fmt) "x86-android-tablet: " 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>
+
+#define IRQ_TYPE_NONE 0
+#define IRQ_TYPE_APIC 1
+#define IRQ_TYPE_GPIOINT 2
+
+struct x86_i2c_client_info {
+ struct i2c_board_info board_info;
+ char *adapter_path;
+ int irq_type;
+ int irq_index;
+ int irq_trigger;
+ int irq_polarity;
+};
+
+struct x86_dev_info {
+ const struct x86_i2c_client_info *i2c_client_info;
+ int i2c_client_count;
+ struct gpiod_lookup_table *gpiod_lookup_table;
+};
+
+int i2c_client_count;
+static struct i2c_client **i2c_clients;
+struct gpio_desc **irq_gpiods;
+
+/*
+ * 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_type = IRQ_TYPE_APIC,
+ .irq_index = 0x44,
+ .irq_trigger = ACPI_EDGE_SENSITIVE,
+ .irq_polarity = ACPI_ACTIVE_HIGH,
+ },
+ { /* BMA250E accelerometer */
+ .board_info = {
+ .type = "bma250e",
+ .addr = 0x18,
+ .swnode = &chuwi_hi8_bma250e_node,
+ },
+ .adapter_path = "\\_SB_.I2C3",
+ .irq_type = IRQ_TYPE_GPIOINT,
+ .irq_trigger = ACPI_LEVEL_SENSITIVE,
+ .irq_polarity = ACPI_ACTIVE_HIGH,
+ },
+};
+
+static struct gpiod_lookup_table chuwi_hi8_gpios = {
+ .table = {
+ GPIO_LOOKUP("INT33FC:02", 23, "bma250e_irq", GPIO_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),
+ .gpiod_lookup_table = &chuwi_hi8_gpios,
+};
+
+/*
+ * On the Xiaomi Mi Pad 2 X86 tablet a bunch of devices are hidden when
+ * the EFI if the tablet does thinks the OS it is booting is Windows
+ * (OSID in the DSDT is set to 1); and the EFI code thinks this as soon
+ * as the EFI bootloader is not Xiaomi's own signed Android loader :|
+ *
+ * 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;
+ unsigned int irq_type;
+ char gpio_name[16];
+ acpi_handle handle;
+ acpi_status status;
+ int ret;
+
+ 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;
+ }
+
+ switch (client_info->irq_type) {
+ case IRQ_TYPE_APIC:
+ ret = acpi_register_gsi(NULL, client_info->irq_index,
+ client_info->irq_trigger,
+ client_info->irq_polarity);
+ if (ret < 0) {
+ pr_err("Error getting APIC IRQ %d: %d\n",
+ client_info->irq_index, ret);
+ goto out;
+ }
+
+ board_info.irq = ret;
+ break;
+ case IRQ_TYPE_GPIOINT:
+ snprintf(gpio_name, sizeof(gpio_name), "%s_irq", board_info.type);
+ irq_gpiods[idx] = gpiod_get(NULL, gpio_name, GPIOD_IN);
+ if (IS_ERR(irq_gpiods[idx])) {
+ ret = PTR_ERR(irq_gpiods[idx]);
+ irq_gpiods[idx] = NULL;
+ pr_err("Error getting GPIO %s: %d\n", gpio_name, ret);
+ goto out;
+ }
+
+ ret = gpiod_to_irq(irq_gpiods[idx]);
+ if (ret < 0) {
+ pr_err("Error getting IRQ for %s: %d\n", gpio_name, ret);
+ goto out;
+ }
+
+ board_info.irq = ret;
+
+ irq_type = acpi_dev_get_irq_type(client_info->irq_trigger,
+ client_info->irq_polarity);
+ if (irq_type != IRQ_TYPE_NONE &&
+ irq_type != irq_get_trigger_type(board_info.irq))
+ irq_set_irq_type(board_info.irq, irq_type);
+
+ break;
+ }
+
+ ret = 0;
+ 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]);
+ gpiod_put(irq_gpiods[i]);
+ }
+
+ kfree(i2c_clients);
+ kfree(irq_gpiods);
+}
+
+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;
+
+ irq_gpiods = kcalloc(i2c_client_count, sizeof(*irq_gpiods), GFP_KERNEL);
+ if (!irq_gpiods) {
+ kfree(i2c_clients);
+ return -ENOMEM;
+ }
+
+ if (dev_info->gpiod_lookup_table)
+ gpiod_add_lookup_table(dev_info->gpiod_lookup_table);
+
+ 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;
+ }
+ }
+
+ if (dev_info->gpiod_lookup_table)
+ gpiod_remove_lookup_table(dev_info->gpiod_lookup_table);
+
+ 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] platform/x86: x86-android-tablets: New driver for x86 Android tablets
2021-11-25 19:26 [PATCH] platform/x86: x86-android-tablets: New driver for x86 Android tablets Hans de Goede
@ 2021-11-25 19:43 ` Andy Shevchenko
2021-12-06 22:03 ` Hans de Goede
0 siblings, 1 reply; 3+ messages in thread
From: Andy Shevchenko @ 2021-11-25 19:43 UTC (permalink / raw
To: Hans de Goede; +Cc: Mark Gross, Andy Shevchenko, Tsuchiya Yuto, Platform Driver
On Thu, Nov 25, 2021 at 9:26 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.
The idea is not bad.
...
> +config X86_ANDROID_TABLETS
> + tristate "X86 Android tablet support"
> + depends on I2C && ACPI
Why I²C dependency? Are you expecting all problematic DSDTs to be
related to I²C?
> + 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.
> +
> + If you have a x86 Android tablet say Y or M here, for a generic x86
> + distro config say M here.
...
> +#define pr_fmt(fmt) "x86-android-tablet: " fmt
Can we use a predefined module name instead?
...
> +#define IRQ_TYPE_NONE 0
> +#define IRQ_TYPE_APIC 1
> +#define IRQ_TYPE_GPIOINT 2
Is this same / similar to what we have in I²C multi-instantiate
driver? Perhaps something in include/linux/platform_data/x86?
...
> +struct x86_i2c_client_info {
> + struct i2c_board_info board_info;
> + char *adapter_path;
> + int irq_type;
> + int irq_index;
> + int irq_trigger;
> + int irq_polarity;
Wondering if these fields are already defined in some structure by IRQ
core or alike. In such case I would rather use more memory and
predefined structure (it it has proper naming, of course).
> +};
...
> +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),
> + .gpiod_lookup_table = &chuwi_hi8_gpios,
> +};
Okay, looking into the below, I think of better granularity of this
one, by splitting on per device module or so. Does it make sense? (I'm
expecting this one to grow too big to be suitable for everybody)
...
> +/*
> + * On the Xiaomi Mi Pad 2 X86 tablet a bunch of devices are hidden when
> + * the EFI if the tablet does thinks the OS it is booting is Windows
in Windows
> + * (OSID in the DSDT is set to 1); and the EFI code thinks this as soon
> + * as the EFI bootloader is not Xiaomi's own signed Android loader :|
> + *
> + * 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),
> + { }
> +};
...
> + if (ret < 0) {
> + pr_err("Error getting APIC IRQ %d: %d\n",
> + client_info->irq_index, ret);
One line?
> + goto out;
> + }
...
> + ret = 0;
> + 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);
> + }
Ah, can we actually use i2c-multi-instantiate as a driver here?
...
> + if (dev_info->gpiod_lookup_table)
Perhaps it makes sense to move this to the GPIO library? AFAIR I saw a
few more cases like this.
> + gpiod_add_lookup_table(dev_info->gpiod_lookup_table);
...
> + if (dev_info->gpiod_lookup_table)
This is not needed. (Yes, now I checked it carefully)
> + gpiod_remove_lookup_table(dev_info->gpiod_lookup_table);
...
> +
> +module_init(x86_android_tablet_init);
> +module_exit(x86_android_tablet_cleanup);
I would prefer to see them attached to the methods and without an
additional blank line (but it's minor thingy).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] platform/x86: x86-android-tablets: New driver for x86 Android tablets
2021-11-25 19:43 ` Andy Shevchenko
@ 2021-12-06 22:03 ` Hans de Goede
0 siblings, 0 replies; 3+ messages in thread
From: Hans de Goede @ 2021-12-06 22:03 UTC (permalink / raw
To: Andy Shevchenko
Cc: Mark Gross, Andy Shevchenko, Tsuchiya Yuto, Platform Driver
Hi,
On 11/25/21 20:43, Andy Shevchenko wrote:
> On Thu, Nov 25, 2021 at 9:26 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.
>
> The idea is not bad.
>
> ...
>
>> +config X86_ANDROID_TABLETS
>> + tristate "X86 Android tablet support"
>
>> + depends on I2C && ACPI
>
> Why I²C dependency? Are you expecting all problematic DSDTs to be
> related to I²C?
I2C at a minimum is a big part if it. So far all this "driver" dies
is instantiate I2C devices, so building it in a kernel which has I2C
disabled does not make any sense.
>
>> + 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
Ack, will fix.
>
>> + 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.
>
> ...
>
>> +#define pr_fmt(fmt) "x86-android-tablet: " fmt
>
> Can we use a predefined module name instead?
Ack, will fix.
>
> ...
>
>> +#define IRQ_TYPE_NONE 0
>> +#define IRQ_TYPE_APIC 1
>> +#define IRQ_TYPE_GPIOINT 2
>
> Is this same / similar to what we have in I²C multi-instantiate
> driver? Perhaps something in include/linux/platform_data/x86?
Yes and no, it looks similar, but there the "enum" is used to select
which ACPI resource to lookup. Where as here we are manually looking
up the interrupt number ourselves since there are no ACPI resources.
So the similarity is superficial only the actual code is quite
different.
>
> ...
>
>> +struct x86_i2c_client_info {
>> + struct i2c_board_info board_info;
>> + char *adapter_path;
>
>> + int irq_type;
>> + int irq_index;
>> + int irq_trigger;
>> + int irq_polarity;
>
> Wondering if these fields are already defined in some structure by IRQ
> core or alike. In such case I would rather use more memory and
> predefined structure (it it has proper naming, of course).
irq_type is set to our own unique defines from above, so I'm
afraid there is no pre-defined structure matching this.
>
>> +};
>
> ...
>
>> +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),
>> + .gpiod_lookup_table = &chuwi_hi8_gpios,
>> +};
>
> Okay, looking into the below, I think of better granularity of this
> one, by splitting on per device module or so. Does it make sense? (I'm
> expecting this one to grow too big to be suitable for everybody)
I actually started with a 1 special driver per model as concept,
but having a lot of tiny modules has a price too, figuring out Kconfig
options takes quite some time for distro kernel maintainers.
And each module adds overhead to, in disk use, but also in depmod info.
So I deliberately decided to go for one file. I'm actually not expecting
this to grow that much at all. Most tablets are dual-boot and the dual-boot
ones typically have a DSDT which is fine.
The only troublesome models which I'm aware of and which I plan to
eventually add are:
1. Asus MeMO Pad 7ME176C (7" BYT Android only tablet)
2. Asus Transformer TF103C (10" BYT Android only tablet)
3. No-name (sometimes branded as Glavey) TM800A550L (8" BYT Android only tablet)
Which puts the total at 5 devices, at least for now. So I believe that
keeping this in 1 file is fine. We can always split it later if it
becomes too big.
> ...
>
>> +/*
>> + * On the Xiaomi Mi Pad 2 X86 tablet a bunch of devices are hidden when
>> + * the EFI if the tablet does thinks the OS it is booting is Windows
>
> in Windows
Ack, will fix.
>
>> + * (OSID in the DSDT is set to 1); and the EFI code thinks this as soon
>> + * as the EFI bootloader is not Xiaomi's own signed Android loader :|
>> + *
>> + * 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),
>> + { }
>> +};
>
> ...
>
>> + if (ret < 0) {
>
>> + pr_err("Error getting APIC IRQ %d: %d\n",
>> + client_info->irq_index, ret);
>
> One line?
Ack, will fix.
>
>> + goto out;
>> + }
>
> ...
>
>> + ret = 0;
>> + 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);
>> + }
>
> Ah, can we actually use i2c-multi-instantiate as a driver here?
That expects ACPI resources, both for the I2C adapter / client address
pair (an I2cSerialBusV2 resource) as well as for the IRQs.
>
> ...
>
>> + if (dev_info->gpiod_lookup_table)
>
> Perhaps it makes sense to move this to the GPIO library? AFAIR I saw a
> few more cases like this.
Maybe something for a separate cleanup series, I don't know adding
NULL checks to register/add functions is not really standard practice
in the kernel.
>
>> + gpiod_add_lookup_table(dev_info->gpiod_lookup_table);
>
> ...
>
>> + if (dev_info->gpiod_lookup_table)
>
> This is not needed. (Yes, now I checked it carefully)
>
>> + gpiod_remove_lookup_table(dev_info->gpiod_lookup_table);
Ack, will fix.
>
> ...
>
>
>> +
>> +module_init(x86_android_tablet_init);
>> +module_exit(x86_android_tablet_cleanup);
>
> I would prefer to see them attached to the methods and without an
> additional blank line (but it's minor thingy).
>
Regards,
Hans
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-12-06 22:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-11-25 19:26 [PATCH] platform/x86: x86-android-tablets: New driver for x86 Android tablets Hans de Goede
2021-11-25 19:43 ` Andy Shevchenko
2021-12-06 22:03 ` Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).