[PATCH 0/1] iio: accel: kxcjk1013: Add tablet_mode sysfs file for exercising the KIOX010A ACPI DSM

[PATCH 0/1] iio: accel: kxcjk1013: Add tablet_mode sysfs file for exercising the KIOX010A ACPI DSM

[Hans de Goede]

Hi All,

This builds on top of the recently merged commit e5b1032a656e ("iio: accel:
kxcjk1013: Add support for KIOX010A ACPI DSM for setting tablet-mode"):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e5b1032a656e9aa4c7a4df77cb9156a2a651a5f9

As explained in that commit: "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.

This commit adds a tablet_mode sysfs file to the i2c_client device
(not to the iio-device so outside of the iio namespace!) which allows
userspace to call the DSM to toggle between laptop/tablet-mode by
writing this file.

I guess this might be a bit controversial, butI would still like to
see this patch go upstream.

The user reporting the initial kbd / touchpad not working issue has
requested for the functionality offered by the DSM (silence kbd and
touchpad) to be made available to userspace; and I already wrote
this patch before then because I would like users to be able to
exercise the DSM to be able to observe what influence it has on
these systems.

Regards,

Hans

[PATCH] iio: accel: kxcjk1013: Add tablet_mode sysfs file for exercising the KIOX010A ACPI DSM

[Hans de Goede]

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(+)

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
 
-- 
2.28.0

Re: [PATCH] iio: accel: kxcjk1013: Add tablet_mode sysfs file for exercising the KIOX010A ACPI DSM

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: accel: kxcjk1013: Add tablet_mode sysfs file for exercising the KIOX010A ACPI DSM

[Hans de Goede]

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

Re: [PATCH] iio: accel: kxcjk1013: Add tablet_mode sysfs file for exercising the KIOX010A ACPI DSM

[Jonathan Cameron]

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

Re: [PATCH] iio: accel: kxcjk1013: Add tablet_mode sysfs file for exercising the KIOX010A ACPI DSM

[Hans de Goede]

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

Re: [PATCH] iio: accel: kxcjk1013: Add tablet_mode sysfs file for exercising the KIOX010A ACPI DSM

[Jonathan Cameron]

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
>