[PATCH 7/8] iio: accel: bmc150: Add support for DUAL250E ACPI DSM for setting the hinge angle

[Hans de Goede]

Hi,

On 5/22/21 8:21 PM, Andy Shevchenko wrote:
> On Fri, May 21, 2021 at 11:22 PM Hans de Goede <hdegoede at redhat.com> wrote:
>>
>> Some 360 degree hinges (yoga) style 2-in-1 devices use 2 bmc150 accels
>> to allow the OS to determine the angle between the display and the base
>> of the device, so that the OS can determine if the 2-in-1 is in laptop
>> or in tablet-mode.
>>
>> On Windows both accelerometers are read (polled) by a special service
>> and this service calls the DSM (Device Specific Method), which in turn
>> translates the angles to one of laptop/tablet/tent/stand mode and then
>> notifies the EC about the new mode and the EC then enables or disables
>> the builtin keyboard and touchpad based in the mode.
>>
>> When the 2-in-1 is powered-on or resumed folded in tablet mode the
>> EC senses this independent of the DSM by using a HALL effect sensor
>> which senses that the keyboard has been folded away behind the display.
>>
>> At power-on or resume the EC disables the keyboard based on this and
>> the only way to get the keyboard to work after this is to call the
>> DSM to re-enable it.
>>
>> Call the DSM on probe() and resume() to fix the keyboard not working
>> when powered-on / resumed in tablet-mode.
>>
>> This patch was developed and tested on a Lenovo Yoga 300-IBR.
> 
> ...
> 
>> +       if (strcmp(acpi_device_hid(adev), "DUAL250E"))
> 
> Can we use like in the other case in this series the acpi_dev_hid_uid_match() ?

I agree that that would be more consistent, I'll fix this for 2.


> Because it's actually what you are doing here and it may be better to
> see the same approach for this HID done in different places in the
> code to recognize what it is about.
> 
>> +               return false;
> 
> ...
> 
>> +       /*
>> +        * The EC must see a change for it to re-enable the kbd, so first set the
>> +        * angle to 270° (tent/stand mode) and then change it to 90° (laptop mode).
>> +        */
>> +       if (!bmc150_acpi_set_angle_dsm(client, 0, 270))
>> +               return false;
> 
>> +       /* The EC needs some time to notice the angle being changed */
>> +       msleep(100);
> 
> I feel that you conducted a research and answer to the following will
> be no, but...
> 
> Do we have any means of polling something (embedded controller / ASL /
> etc) to actually see the ACK for the action?

Nope, there is nothing to poll and I tried shorter time-outs and that
lead to the EC sometimes not seeing the change.

> 
>> +       return bmc150_acpi_set_angle_dsm(client, 0, 90);
> 
> ...
> 
>> +       schedule_delayed_work(&data->resume_work, msecs_to_jiffies(1000));
> 
> Isn't it the same as 1 * HZ ?

Yes, but this makes it clearer that the delay is 1 second, IMHO using n * HZ
is harder to read / reason about then having the delay right there in msecs.
This also means less churn if it needs to change to a different amount of msecs
later.

Regards,

Hans