Re: [PATCH] platform/x86: Add and use a dual_accel_detect() helper

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 7/29/21 10:37 AM, Andy Shevchenko wrote:
> On Thu, Jul 29, 2021 at 11:21 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> Various 360 degree hinges (yoga) style 2-in-1 devices use 2 accelerometers
>> to allow the OS to determine the angle between the display and the base of
>> the device.
>>
>> On Windows these are read by a special HingeAngleService process which
>> calls undocumented ACPI methods, to let the firmware know if the 2-in-1 is
>> in tablet- or laptop-mode. The firmware may use this to disable the kbd and
>> touchpad to avoid spurious input in tablet-mode as well as to report
>> SW_TABLET_MODE info to the OS.
>>
>> Since Linux does not call these undocumented methods, the SW_TABLET_MODE
>> info reported by various pdx86 drivers is incorrect on these devices.
>>
>> Before this commit the intel-hid and thinkpad_acpi code already had 2
>> hardcoded checks for ACPI hardware-ids of dual-accel sensors to avoid
>> reporting broken info.
>>
>> And now we also have a bug-report about the same problem in the intel-vbtn
>> code. Since there are at least 3 different ACPI hardware-ids in play, add
>> a new dual_accel_detect() helper which checks for all 3, rather then
>> adding different hardware-ids to the drivers as bug-reports trickle in.
>> Having shared code which checks all known hardware-ids is esp. important
>> for the intel-hid and intel-vbtn drivers as these are generic drivers
>> which are used on a lot of devices.
>>
>> The BOSC0200 hardware-id requires special handling, because often it is
>> used for a single-accelerometer setup. Only in a few cases it refers to
>> a dual-accel setup, in which case there will be 2 I2cSerialBus resources
>> in the device's resource-list, so the helper checks for this.
> 
> ...
> 
>> +static int dual_accel_i2c_resource_count(struct acpi_resource *ares, void *data)
>> +{
>> +       struct acpi_resource_i2c_serialbus *sb;
>> +       int *count = data;
>> +
>> +       if (i2c_acpi_get_i2c_resource(ares, &sb))
>> +               *count = *count + 1;
>> +
>> +       return 1;
>> +}
> 
> It will be a third copy of this in the kernel.
> Let's put it to i2c.h or somewhere available for all these users.
> 
>> +
>> +static int dual_accel_i2c_client_count(struct acpi_device *adev)
>> +{
>> +       int ret, count = 0;
>> +       LIST_HEAD(r);
>> +
>> +       ret = acpi_dev_get_resources(adev, &r, dual_accel_i2c_resource_count, &count);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       acpi_dev_free_resource_list(&r);
>> +       return count;
>> +}
> 
> So does this.
> 
> Taking into account that this is a bug fix, I'm okay if you do above
> as an additional patch (or patches) on top of this.

Right, I had a note about this behind the cut (---) line, but I dropped
the patch and git-am-ed it while reworking my tree for some other issue
dropping the note (sorry), the note was:

"""
---
Note the counting of the number of I2cSerialBus resources in an
ACPI-device's resource-list is becoming a common pattern. I plan
to add a new shared helper for this in a follow-up patch-set.
I've deliberately not made use of such a new helper in this patch
for easier backporting to the stable series.
"""

In other words, I fully agree. I've already added an item to my
TODO list about doing a followup series to replace the 3 copies in:

 drivers/platform/x86/dual_accel_detect.h
 drivers/platform/x86/i2c-multi-instantiate.c
 drivers/platform/x86/intel/int33fe/intel_cht_int33fe_common.c

With a new helper in drivers/i2c/i2c-core-acpi.c, like the
i2c_acpi_get_i2c_resource() helper which was recently added.

Regards,

Hans




[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux