Hi, On 5/22/21 8:21 PM, Andy Shevchenko wrote: > On Fri, May 21, 2021 at 11:22 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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
