| Message ID | 20201125085444.11593-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: accel: kxcjk1013: Add tablet_mode sysfs file for exercising the KIOX010A ACPI DSM | expand |
On Wed, Nov 25, 2020 at 10:56 AM Hans de Goede <hdegoede@redhat.com> 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. ... > + 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); > + if (err) > + return err; ... > + ret = devm_device_add_group(&data->client->dev, &tablet_mode_attrs_group); > + if (ret < 0) > + dev_warn(&data->client->dev, "Error creating tablet_mode sysfs attribute\n"); devm is a beast (sometimes). Just to make sure that on removal you won't have situation when attribute is still there while data it's accessing to are already gone or garbage.
Hi, On 11/25/20 11:34 AM, Andy Shevchenko wrote: > On Wed, Nov 25, 2020 at 10:56 AM Hans de Goede <hdegoede@redhat.com> 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. > > ... > >> + 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). > >> + if (err) >> + return err; > > ... > >> + ret = devm_device_add_group(&data->client->dev, &tablet_mode_attrs_group); >> + if (ret < 0) >> + dev_warn(&data->client->dev, "Error creating tablet_mode sysfs attribute\n"); > > devm is a beast (sometimes). Just to make sure that on removal you > won't have situation when attribute is still there while data it's > accessing to are already gone or garbage. The main data struct is also devm managed and allocated earlier, so this is not an issue here. Regards, Hans
On Wed, 25 Nov 2020 11:55:45 +0100 Hans de Goede <hdegoede@redhat.com> 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@redhat.com> 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 Unless this is documented somewhere else? I've no idea if there is any precedence for this/ > > > > ... > > > >> + 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 :) Thanks, Jonathan
Hi, On 11/25/20 7:48 PM, Jonathan Cameron wrote: > On Wed, 25 Nov 2020 11:55:45 +0100 > Hans de Goede <hdegoede@redhat.com> 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@redhat.com> 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
On Thu, 26 Nov 2020 11:30:22 +0100 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 11/25/20 7:48 PM, Jonathan Cameron wrote: > > On Wed, 25 Nov 2020 11:55:45 +0100 > > Hans de Goede <hdegoede@redhat.com> 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@redhat.com> 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/ > oops. > 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... Definitely need docs, even if you need to create a new place to put it. Probably need to cc the i2c list as well given it's ABI directly under the I2C device. Possibly best to also cc linux-abi as this one is a bit unusual. ABI/test/sysfs-bus-i2c-devices-kxcjk1013 is probably the right place. There is some precedent in similar files + they deliberately avoid being too specific on the naming of the path. > > > 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). Absolutely. I suspect and hope things are getting stricter on that front than they have been in the past. Certainly Greg KH has been pointing out when people try to sneak in undocumented ABI. I guess long term, we should do a cleanup exercise to try and document as many of those non standard bits of ABI as possible. This is particularly true as the ABI docs are now part of the nice pretty printed HTML docs. (or will be very soon anyway given I'm not sure Mauro's work has landed yet!) Jonathan > > > > >>> > >>> ... > >>> > >>>> + 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 >
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c index 560a3373ff20..f15946d87c3c 100644 --- a/drivers/iio/accel/kxcjk-1013.c +++ b/drivers/iio/accel/kxcjk-1013.c @@ -150,6 +150,7 @@ struct kxcjk1013_data { int64_t timestamp; enum kx_chipset chipset; enum kx_acpi_type acpi_type; + bool tablet_mode; }; enum kxcjk1013_axis { @@ -300,6 +301,50 @@ static int kiox010a_dsm(struct device *dev, int fn_index) ACPI_FREE(obj); return 0; } + +static ssize_t tablet_mode_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_get_drvdata(dev); + struct kxcjk1013_data *data = iio_priv(indio_dev); + + return sprintf(buf, "%d\n", data->tablet_mode); +} + +static ssize_t tablet_mode_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct iio_dev *indio_dev = dev_get_drvdata(dev); + struct kxcjk1013_data *data = iio_priv(indio_dev); + unsigned long tablet_mode; + int err; + + err = kstrtoul(buf, 0, &tablet_mode); + if (err) + return err; + + err = kiox010a_dsm(&data->client->dev, + tablet_mode ? KIOX010A_SET_TABLET_MODE : + KIOX010A_SET_LAPTOP_MODE); + if (err) + return err; + + data->tablet_mode = tablet_mode; + return len; +} + +static DEVICE_ATTR_RW(tablet_mode); + +static struct attribute *tablet_mode_attrs[] = { + &dev_attr_tablet_mode.attr, + NULL +}; + +static const struct attribute_group tablet_mode_attrs_group = { + .attrs = tablet_mode_attrs, +}; #endif static int kxcjk1013_set_mode(struct kxcjk1013_data *data, @@ -383,6 +428,10 @@ static int kxcjk1013_chip_init(struct kxcjk1013_data *data) if (data->acpi_type == ACPI_KIOX010A) { /* Make sure the kbd and touchpad on 2-in-1s using 2 KXCJ91008-s work */ kiox010a_dsm(&data->client->dev, KIOX010A_SET_LAPTOP_MODE); + + ret = devm_device_add_group(&data->client->dev, &tablet_mode_attrs_group); + if (ret < 0) + dev_warn(&data->client->dev, "Error creating tablet_mode sysfs attribute\n"); } #endif
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. Reported-by: russianneuromancer <russianneuromancer@ya.ru> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/iio/accel/kxcjk-1013.c | 49 ++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)