Hi,
On 12/21/21 16:39, Andy Shevchenko wrote:
> On Tue, Dec 21, 2021 at 5:13 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.
>
> ...
>
>> +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; ?
>
