Hi, On 11/25/20 7:48 PM, Jonathan Cameron wrote: > On Wed, 25 Nov 2020 11:55:45 +0100 > Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > >> Hi, >> >> On 11/25/20 11:34 AM, Andy Shevchenko wrote: >>> On Wed, Nov 25, 2020 at 10:56 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >>>> >>>> Some 360 degree hinges (yoga) style 2-in-1 devices use 2 KXCJ91008-s >>>> 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 by a special HingeAngleService >>>> process; and this process calls a DSM (Device Specific Method) on the >>>> ACPI KIOX010A device node for the sensor in the display, to let the >>>> embedded-controller (EC) know about the mode so that it can disable the >>>> kbd and touchpad to avoid spurious input while folded into tablet-mode. >>>> >>>> Currently the kxcjk1013 driver calls the DSM for this once at probe time >>>> to ensure that the builtin kbd and touchpad work. >>>> >>>> But some users have expressed interest in using this functionality to >>>> disable the kbd and touchpad when folded into tablet-mode as done under >>>> Windows. >>>> >>>> Add a tablet_mode sysfs file so that users can control the kbd/touchpad >>>> enable/disable functionality from user-space. > > Biggest thing missing here is documentation. > > Documentation/ABI/testing/sysfs-bus-iio-kxcjk1013 Erm, as I explained in the cover letter this new sysfs attribute sits below /sys/bus/i2c/devices/KIOX010A:00/ So thew new file is: /sys/bus/i2c/devices/KIOX010A:00/tablet_mode IOW it does NOT sit below: /sys/bus/iio/devices/iio-device1 I did this deliberately so that it is in no way part of the IIO userspace-API (where it does not belong). I did have it as part of the IIO userspace API before, that is one of the reasons why I wrote the: "iio: core: Copy iio_info.attrs->is_visible into iio_dev_opaque.chan_attr_group.is_visible" Patch so that I could only make it show up in the KIOX010A case without needing to duplicate the structures describing the IIO channels. But just not putting it in the IIO "namespace" at all seemed cleaner, so I eventually went with that as solution. And I still believe that having this under the i2c-device rather then under the iio-device is the right thing to do, but I'm open to changing that. I guess I could still try to document it somewhere, but with the scheme which I chose there really isn't a vert good place to document this... > Unless this is documented somewhere else? I've no idea if there is any > precedence for this/ There is plenty of precedence for adhoc driver specific sysfs attributes being added to the (parent) device, but these tend to not be documented (which I must admit is kinda bad). > >>> >>> ... >>> >>>> + err = kiox010a_dsm(&data->client->dev, >>>> + tablet_mode ? KIOX010A_SET_TABLET_MODE : >>>> + KIOX010A_SET_LAPTOP_MODE); >>> >>> A nit. With temporary variable it may be slightly better to read, like: >>> >>> int value; >>> ... >>> value = tablet_mode ? KIOX010A_SET_TABLET_MODE : KIOX010A_SET_LAPTOP_MODE); >>> err = kiox010a_dsm(&data->client->dev, value); >> >> I'm fine with either solution, Jonathan let me know if you want a v2 with >> Andy's suggestion implemented (assuming you are willing to take this at all). > > Prefer Andy's suggestion slightly seeing as you are going around again to > include some docs :) Ok, I'll change this for v2 once we have the other bits figured out. Regards, Hans
