On Thu, Nov 25, 2021 at 9:26 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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
