Hi,
On 7/29/21 10:50 AM, Andy Shevchenko wrote:
> On Thu, Jul 29, 2021 at 11:45 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> 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:
>
> With this promise taken
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
Thank you, I've added this to the review-hans and fixes branches now.
Regards,
Hans
