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