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

[Hans de Goede <hdegoede@redhat.com>]

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