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