[PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[Ritesh Raj Sarraf]

Lenovo Yoga (many variants: Yoga, Yoga2 Pro, Yoga2 13, Yoga3 Pro, Yoga 3
14 etc) has multiple modles that are a hybrid laptop, working in laptop
mode as well as tablet mode.

Currently, there is no easy interface to determine the touchpad status,
which in case of the Yoga family of machines, can also be useful to
assume tablet mode status.
Note: The ideapad-laptop driver does not provide a SW_TABLET_MODE either

For a detailed discussion  on why we want either of the interfaces,
please see:
https://bugs.launchpad.net/onboard/+bug/1366421/comments/43

This patch adds a sysfs interface for read/write access under:
/sys/bus/platform/devices/VPC2004\:00/touchpad_mode

v4:
Use kstrtobool because supported values are boolean

v3:
Include Darren Hart's comments
Changed sysfs inteface from "touchpad_mode" to "touchpad"

v2:
Include Andy Shevchenko's comments

Signed-off-by: Ritesh Raj Sarraf <rrs@debian.org>
---
 .../ABI/testing/sysfs-platform-ideapad-laptop      |  8 +++++
 drivers/platform/x86/ideapad-laptop.c              | 34 ++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
index b31e782bd985..5d24f1e8e6ef 100644
--- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
+++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
@@ -17,3 +17,11 @@ Description:
 			* 2 -> Dust Cleaning
 			* 4 -> Efficient Thermal Dissipation Mode
 
+What:		/sys/devices/platform/ideapad/touchpad
+Date:		Feb 2017
+KernelVersion:	4.11
+Contact:	"Ritesh Raj Sarraf <rrs@debian.org>"
+Description:
+		Control touchpad mode.
+			* 1 -> Switched On
+			* 0 -> Switched Off
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index f46ece2ce3c4..b35954707e11 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -423,9 +423,43 @@ static ssize_t store_ideapad_fan(struct device *dev,
 
 static DEVICE_ATTR(fan_mode, 0644, show_ideapad_fan, store_ideapad_fan);
 
+
+static ssize_t touchpad_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct ideapad_private *priv = dev_get_drvdata(dev);
+	unsigned long result;
+
+	if (read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result))
+		return sprintf(buf, "-1\n");
+	return sprintf(buf, "%lu\n", result);
+}
+
+static ssize_t touchpad_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct ideapad_private *priv = dev_get_drvdata(dev);
+	int ret;
+	bool state;
+
+	ret = kstrtobool(buf, &state);
+	if (ret)
+		return ret;
+
+	ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, state);
+	if (ret < 0)
+		return -EIO;
+	return count;
+}
+
+static DEVICE_ATTR_RW(touchpad);
+
 static struct attribute *ideapad_attributes[] = {
 	&dev_attr_camera_power.attr,
 	&dev_attr_fan_mode.attr,
+	&dev_attr_touchpad.attr,
 	NULL
 };
 
-- 
2.11.0

Re: [PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[Ritesh Raj Sarraf]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Hello Darren / Andy,

Any further comment on this patch ? Will this be accepted ?
Please give a N/ACK.

Thanks,
Ritesh

On Sat, 2017-02-18 at 00:17 +0530, Ritesh Raj Sarraf wrote:
> Lenovo Yoga (many variants: Yoga, Yoga2 Pro, Yoga2 13, Yoga3 Pro, Yoga 3
> 14 etc) has multiple modles that are a hybrid laptop, working in laptop
> mode as well as tablet mode.
> 
> Currently, there is no easy interface to determine the touchpad status,
> which in case of the Yoga family of machines, can also be useful to
> assume tablet mode status.
> Note: The ideapad-laptop driver does not provide a SW_TABLET_MODE either
> 
> For a detailed discussion  on why we want either of the interfaces,
> please see:
> https://bugs.launchpad.net/onboard/+bug/1366421/comments/43
> 
> This patch adds a sysfs interface for read/write access under:
> /sys/bus/platform/devices/VPC2004\:00/touchpad_mode
> 
> v4:
> Use kstrtobool because supported values are boolean
> 
> v3:
> Include Darren Hart's comments
> Changed sysfs inteface from "touchpad_mode" to "touchpad"
> 
> v2:
> Include Andy Shevchenko's comments
> 
> Signed-off-by: Ritesh Raj Sarraf <rrs@debian.org>
> ---
>  .../ABI/testing/sysfs-platform-ideapad-laptop      |  8 +++++
>  drivers/platform/x86/ideapad-laptop.c              | 34
> ++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> index b31e782bd985..5d24f1e8e6ef 100644
> --- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> +++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> @@ -17,3 +17,11 @@ Description:
>  			* 2 -> Dust Cleaning
>  			* 4 -> Efficient Thermal Dissipation Mode
>  
> +What:		/sys/devices/platform/ideapad/touchpad
> +Date:		Feb 2017
> +KernelVersion:	4.11
> +Contact:	"Ritesh Raj Sarraf <rrs@debian.org>"
> +Description:
> +		Control touchpad mode.
> +			* 1 -> Switched On
> +			* 0 -> Switched Off
> diff --git a/drivers/platform/x86/ideapad-laptop.c
> b/drivers/platform/x86/ideapad-laptop.c
> index f46ece2ce3c4..b35954707e11 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -423,9 +423,43 @@ static ssize_t store_ideapad_fan(struct device *dev,
>  
>  static DEVICE_ATTR(fan_mode, 0644, show_ideapad_fan, store_ideapad_fan);
>  
> +
> +static ssize_t touchpad_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct ideapad_private *priv = dev_get_drvdata(dev);
> +	unsigned long result;
> +
> +	if (read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result))
> +		return sprintf(buf, "-1\n");
> +	return sprintf(buf, "%lu\n", result);
> +}
> +
> +static ssize_t touchpad_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	struct ideapad_private *priv = dev_get_drvdata(dev);
> +	int ret;
> +	bool state;
> +
> +	ret = kstrtobool(buf, &state);
> +	if (ret)
> +		return ret;
> +
> +	ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, state);
> +	if (ret < 0)
> +		return -EIO;
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(touchpad);
> +
>  static struct attribute *ideapad_attributes[] = {
>  	&dev_attr_camera_power.attr,
>  	&dev_attr_fan_mode.attr,
> +	&dev_attr_touchpad.attr,
>  	NULL
>  };
>  
- -- 
Ritesh Raj Sarraf | http://people.debian.org/~rrs
Debian - The Universal Operating System
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEQCVDstmIVAB/Yn02pjpYo/LhdWkFAlitZtIACgkQpjpYo/Lh
dWnCsw/9Eo0+id6jjJZaVRBhtQTcI510T2IY/wDFfrMOH8Ei2+Iw1d/rCCA5iCWa
HDP9R+AZpFxM71nJsm/JZVuYOZDbKh17TkeM9eIG1dpQH48I576hz0IdwvRNeT+t
kba0NGyQTw0PdMdP8mcOhrFbDkFiLJ0rg0fu1ESxilTnshTUPch+1aUexqeBTCYG
ICvidlD0gr+Q8x9pbqCEdFNwRzqekl7a9ezR4bvyqXpJSx2DWVq2WFHbnrX9BADF
gextZaRWQ7sUR27seXO3JL0DgfYspDkP2UQno7lr4Vx3UxyeUUDVYmF2OmpupHJu
F8BifivXBPy0ZDLn8UbKgEMBqLIx5E3BWWdVyJyobkB9PHoDbe0vvpNXyAQW2xzY
X+CtozZ04su9rk5L+ivsozbAyNFXirHrXOOGZzTa/svKHSsR7xiFA9JHK5mwtnsl
KhJNR0FkXH+QIBm8dbUDteJ9/fTodnUjXN+C7dya9+Sb0Bi1AgSpVe1poZ+74XWR
jz5NtJ+yKxo7SOQ6SxRSPN7zLfhwpORoi5pid2Qn0dKpW8GoB8b6bi0xHGWuraIJ
LIVTJE6TgMgFkjPTJR4vlR+eoxyDM2HTsyx59fWwECEJUANXit7pnbixnZ4McYLh
he6dsgStv8kuHNNCJoeDPLiNYvruUKqHdnf/Vvbk0A+sCgY8JAI=
=BuNe
-----END PGP SIGNATURE-----

Re: [PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[Andy Shevchenko]

On Wed, Feb 22, 2017 at 12:24 PM, Ritesh Raj Sarraf <rrs@debian.org> wrote:

> Any further comment on this patch ? Will this be accepted ?
> Please give a N/ACK.

First of all, please do not top post.

> On Sat, 2017-02-18 at 00:17 +0530, Ritesh Raj Sarraf wrote:
>> Lenovo Yoga (many variants: Yoga, Yoga2 Pro, Yoga2 13, Yoga3 Pro, Yoga 3
>> 14 etc) has multiple modles that are a hybrid laptop, working in laptop
>> mode as well as tablet mode.
>>
>> Currently, there is no easy interface to determine the touchpad status,
>> which in case of the Yoga family of machines, can also be useful to
>> assume tablet mode status.
>> Note: The ideapad-laptop driver does not provide a SW_TABLET_MODE either
>>
>> For a detailed discussion  on why we want either of the interfaces,
>> please see:
>> https://bugs.launchpad.net/onboard/+bug/1366421/comments/43
>>
>> This patch adds a sysfs interface for read/write access under:
>> /sys/bus/platform/devices/VPC2004\:00/touchpad_mode

While patch is okay to me per se, I would like to have a formal
acknowledgement from maintainer.

Ike, can you give one?

>> @@ -17,3 +17,11 @@ Description:
>>                       * 2 -> Dust Cleaning
>>                       * 4 -> Efficient Thermal Dissipation Mode
>>
>> +What:                /sys/devices/platform/ideapad/touchpad
>> +Date:                Feb 2017
>> +KernelVersion:       4.11
>> +Contact:     "Ritesh Raj Sarraf <rrs@debian.org>"
>> +Description:
>> +             Control touchpad mode.
>> +                     * 1 -> Switched On
>> +                     * 0 -> Switched Off
>> diff --git a/drivers/platform/x86/ideapad-laptop.c
>> b/drivers/platform/x86/ideapad-laptop.c
>> index f46ece2ce3c4..b35954707e11 100644
>> --- a/drivers/platform/x86/ideapad-laptop.c
>> +++ b/drivers/platform/x86/ideapad-laptop.c
>> @@ -423,9 +423,43 @@ static ssize_t store_ideapad_fan(struct device *dev,
>>
>>  static DEVICE_ATTR(fan_mode, 0644, show_ideapad_fan, store_ideapad_fan);
>>

>> +

Extra empty line.

>> +static ssize_t touchpad_show(struct device *dev,
>> +                             struct device_attribute *attr,
>> +                             char *buf)
>> +{
>> +     struct ideapad_private *priv = dev_get_drvdata(dev);
>> +     unsigned long result;
>> +
>> +     if (read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result))
>> +             return sprintf(buf, "-1\n");
>> +     return sprintf(buf, "%lu\n", result);
>> +}
>> +
>> +static ssize_t touchpad_store(struct device *dev,
>> +                              struct device_attribute *attr,
>> +                              const char *buf, size_t count)
>> +{
>> +     struct ideapad_private *priv = dev_get_drvdata(dev);

>> +     int ret;
>> +     bool state;

Reverse tree order.

>> +
>> +     ret = kstrtobool(buf, &state);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, state);
>> +     if (ret < 0)
>> +             return -EIO;
>> +     return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(touchpad);
>> +
>>  static struct attribute *ideapad_attributes[] = {
>>       &dev_attr_camera_power.attr,
>>       &dev_attr_fan_mode.attr,
>> +     &dev_attr_touchpad.attr,
>>       NULL
>>  };


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[Andy Shevchenko]

On Wed, Feb 22, 2017 at 12:24 PM, Ritesh Raj Sarraf <rrs@debian.org> wrote:

> Any further comment on this patch ? Will this be accepted ?
> Please give a N/ACK.

Sorry for a long delay, but I can't go with this without clear
understanding that there is indeed no other ways available.
>From what I read in the above link the main issue that driver doesn't
send SW_TABLET_MODE event through input device.
I think this is the best approach how it should go.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[Ritesh Raj Sarraf]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

Hello,

On Fri, 2017-04-28 at 22:17 +0300, Andy Shevchenko wrote:
> On Wed, Feb 22, 2017 at 12:24 PM, Ritesh Raj Sarraf <rrs@debian.org> wrote:
> 
> > Any further comment on this patch ? Will this be accepted ?
> > Please give a N/ACK.
> 
> Sorry for a long delay, but I can't go with this without clear
> understanding that there is indeed no other ways available.
> From what I read in the above link 

i am assuming you are referring to the launchpad bug report here.

> the main issue that driver doesn't
> send SW_TABLET_MODE event through input device.

Well. Yes. That is one part. If SW_TABLET_MODE is done in the driver, much
better. My patch was simply in line with some of the other drivers (hp-wmi and
thinkpad_acpi) to get it working for Lenovo Yoga series.

> I think this is the best approach how it should go.

-- 
Ritesh Raj Sarraf | http://people.debian.org/~rrs
Debian - The Universal Operating System

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[Andy Shevchenko]

On Sat, Apr 29, 2017 at 9:52 AM, Ritesh Raj Sarraf <rrs@debian.org> wrote:
> Hello,
>
> On Fri, 2017-04-28 at 22:17 +0300, Andy Shevchenko wrote:
>> On Wed, Feb 22, 2017 at 12:24 PM, Ritesh Raj Sarraf <rrs@debian.org> wrote:
>>
>> > Any further comment on this patch ? Will this be accepted ?
>> > Please give a N/ACK.
>>
>> Sorry for a long delay, but I can't go with this without clear
>> understanding that there is indeed no other ways available.
>> From what I read in the above link
>
> i am assuming you are referring to the launchpad bug report here.

Correct.

>> the main issue that driver doesn't
>> send SW_TABLET_MODE event through input device.
>
> Well. Yes. That is one part. If SW_TABLET_MODE is done in the driver, much
> better. My patch was simply in line with some of the other drivers (hp-wmi and
> thinkpad_acpi) to get it working for Lenovo Yoga series.

sysfs ABI for drivers that provide input interface is quite strong for
my opinion. It means I'm not totally objecting, but I would accept it
if and only if there is nothing else could be done.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[Darren Hart]

On Sun, Apr 30, 2017 at 03:54:43PM +0300, Andy Shevchenko wrote:
> On Sat, Apr 29, 2017 at 9:52 AM, Ritesh Raj Sarraf <rrs@debian.org> wrote:
> > Hello,
> >
> > On Fri, 2017-04-28 at 22:17 +0300, Andy Shevchenko wrote:
> >> On Wed, Feb 22, 2017 at 12:24 PM, Ritesh Raj Sarraf <rrs@debian.org> wrote:
> >>
> >> > Any further comment on this patch ? Will this be accepted ?
> >> > Please give a N/ACK.
> >>
> >> Sorry for a long delay,

Eeek, likewise. This one fell off my stack. We've improved our patch management
process since this was originally submitted, so hopefully this is more and more
rare.

> >> but I can't go with this without clear
> >> understanding that there is indeed no other ways available.
> >> From what I read in the above link
> >
> > i am assuming you are referring to the launchpad bug report here.
> 
> Correct.
> 
> >> the main issue that driver doesn't
> >> send SW_TABLET_MODE event through input device.
> >
> > Well. Yes. That is one part. If SW_TABLET_MODE is done in the driver, much
> > better. My patch was simply in line with some of the other drivers (hp-wmi and
> > thinkpad_acpi) to get it working for Lenovo Yoga series.
> 
> sysfs ABI for drivers that provide input interface is quite strong for
> my opinion. It means I'm not totally objecting, but I would accept it
> if and only if there is nothing else could be done.

Agreed, we've recently wanted to remove certain sysfs attributes from another
driver as they were obsolete and better implemented in other ways, but once they
are there, are hands are tied.

That said, we will support getting these systems functional. From what I see in
the patch you are implementing a polling sysfs interface. Have you verified that
there is no event we can capture and send the SW_TABLET_MODE along to the input
system?

Finally, I just want to point out that while it may seem capricious for us to
resist accepting a new sysfs attribute that exists in other drivers, please note
that the thinkpad driver (for example) is ancient and implements multiple
mechanisms for several things and should not be considered as a reference for
new code. hp_wmi is another story.

Andy, we may also need to consult on a set of common laptop attributes that we
will accept in general. This would ease review and also lead to some more
uniformity in the sysfs attributes of the most common features (like tablet
mode, touchpad, etc.). That said, the value are likely to be platform specific
and it could prove difficult to map all that variety to a common set of values.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[Ritesh Raj Sarraf]

[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]

On Mon, 2017-05-01 at 09:05 -0700, Darren Hart wrote:
> > sysfs ABI for drivers that provide input interface is quite strong for
> > my opinion. It means I'm not totally objecting, but I would accept it
> > if and only if there is nothing else could be done.
> 
> Agreed, we've recently wanted to remove certain sysfs attributes from another
> driver as they were obsolete and better implemented in other ways, but once
> they
> are there, are hands are tied.
> 
> That said, we will support getting these systems functional. From what I see
> in
> the patch you are implementing a polling sysfs interface. Have you verified
> that
> there is no event we can capture and send the SW_TABLET_MODE along to the
> input
> system?

I believe it does generate an event (at least on my Yoga 2 13 variant). IIRC,
when I talked to Andy initially about the driver's limitation, we did do an
exercise [1] to determine if an event was generated.

Cooking this patch (sysfs interface) was much quicker and easy. I looked at the
other example drivers, that do SW_TABLET_MODE, but left it there. I don't think
I have all the necessary information to get that done for the ideapad driver.


[1] https://patchwork.kernel.org/patch/9546987/

-- 
Ritesh Raj Sarraf | http://people.debian.org/~rrs
Debian - The Universal Operating System

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[Darren Hart]

On Mon, May 01, 2017 at 11:57:27PM +0530, Ritesh Raj Sarraf wrote:
> On Mon, 2017-05-01 at 09:05 -0700, Darren Hart wrote:
> > > sysfs ABI for drivers that provide input interface is quite strong for
> > > my opinion. It means I'm not totally objecting, but I would accept it
> > > if and only if there is nothing else could be done.
> > 
> > Agreed, we've recently wanted to remove certain sysfs attributes from another
> > driver as they were obsolete and better implemented in other ways, but once
> > they
> > are there, are hands are tied.
> > 
> > That said, we will support getting these systems functional. From what I see
> > in
> > the patch you are implementing a polling sysfs interface. Have you verified
> > that
> > there is no event we can capture and send the SW_TABLET_MODE along to the
> > input
> > system?
> 
> I believe it does generate an event (at least on my Yoga 2 13 variant). IIRC,
> when I talked to Andy initially about the driver's limitation, we did do an
> exercise [1] to determine if an event was generated.
> 
> Cooking this patch (sysfs interface) was much quicker and easy. I looked at the
> other example drivers, that do SW_TABLET_MODE, but left it there. I don't think
> I have all the necessary information to get that done for the ideapad driver.
> 
> 
> [1] https://patchwork.kernel.org/patch/9546987/

Thanks. We really want to avoid user-space having to write platform specific
sysfs attribute access to determine system state for common things like "should
I show the on screen keyboard?". That doesn't scale and will result in a very
sparse support matrix in general.

Something I am not particularly well versed in is the issue with hotkeys,
xinput, and only working in Xorg rather than wayland. Where in the stack is this
limitation created? I assume it is above the kernel - no reason Wayland can't
read SW_TABLET_MODE from the input subsystem correct?

I believe I have a couple systems which will replicate this issue so I'll try to
spend some time with them to see what else might be done.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[Ritesh Raj Sarraf]

[-- Attachment #1.1: Type: text/plain, Size: 1351 bytes --]

Resending again, as Google servers are behaving weird lately.

On Sun, 2017-04-30 at 15:54 +0300, Andy Shevchenko wrote:
> > > the main issue that driver doesn't
> > > send SW_TABLET_MODE event through input device.
> > 
> > Well. Yes. That is one part. If SW_TABLET_MODE is done in the driver, much
> > better. My patch was simply in line with some of the other drivers (hp-wmi
> > and
> > thinkpad_acpi) to get it working for Lenovo Yoga series.
> 
> sysfs ABI for drivers that provide input interface is quite strong for
> my opinion. It means I'm not totally objecting, but I would accept it
> if and only if there is nothing else could be done.

The need we have in the user application is for read-only access. 

I have attached the same old patch with just the minor file attribute change,
which would make the sysfs interface to read-only. If in future, there are good
reasons to expose that interface as read/write, updating that attribute can be
revisited.

Can this now be included in your tree ? This minor patch may be helpful to many
Ideapad users with a Hybrid machine.


PS: Given how minimal the change is, I just hand updated the patch. If you want
it in the proper git send-email flow, please let me know.

-- 
Ritesh Raj Sarraf | http://people.debian.org/~rrs
Debian - The Universal Operating System

[-- Attachment #1.2: 0001-Add-sysfs-interface-for-touchpad-state.patch --]
[-- Type: text/x-patch, Size: 3269 bytes --]

From f18911c1ee0714943a023db8ae43bf84a3873d98 Mon Sep 17 00:00:00 2001
From: Ritesh Raj Sarraf <rrs@debian.org>
Date: Mon, 30 Jan 2017 15:05:48 +0530
Subject: [PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

Lenovo Yoga (many variants: Yoga, Yoga2 Pro, Yoga2 13, Yoga3 Pro, Yoga 3
14 etc) has multiple modles that are a hybrid laptop, working in laptop
mode as well as tablet mode.

Currently, there is no easy interface to determine the touchpad status,
which in case of the Yoga family of machines, can also be useful to
assume tablet mode status.
Note: The ideapad-laptop driver does not provide a SW_TABLET_MODE either

For a detailed discussion  on why we want either of the interfaces,
please see:
https://bugs.launchpad.net/onboard/+bug/1366421/comments/43

This patch adds a sysfs interface for read access under:
/sys/bus/platform/devices/VPC2004\:00/touchpad

v5:
Set file attribute to read-only

v4:
Use kstrtobool because supported values are boolean

v3:
Include Darren Hart's comments
Changed sysfs inteface from "touchpad_mode" to "touchpad"

v2:
Include Andy Shevchenko's comments

Signed-off-by: Ritesh Raj Sarraf <rrs@debian.org>
---
 .../ABI/testing/sysfs-platform-ideapad-laptop      |  8 +++++
 drivers/platform/x86/ideapad-laptop.c              | 34 ++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
index b31e782bd985..5d24f1e8e6ef 100644
--- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
+++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
@@ -17,3 +17,11 @@ Description:
 			* 2 -> Dust Cleaning
 			* 4 -> Efficient Thermal Dissipation Mode
 
+What:		/sys/devices/platform/ideapad/touchpad
+Date:		Feb 2017
+KernelVersion:	4.12
+Contact:	"Ritesh Raj Sarraf <rrs@debian.org>"
+Description:
+		Read touchpad status.
+			* 1 -> Switched On
+			* 0 -> Switched Off
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index f46ece2ce3c4..b35954707e11 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -423,9 +423,43 @@ static ssize_t store_ideapad_fan(struct device *dev,
 
 static DEVICE_ATTR(fan_mode, 0644, show_ideapad_fan, store_ideapad_fan);
 
+
+static ssize_t touchpad_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct ideapad_private *priv = dev_get_drvdata(dev);
+	unsigned long result;
+
+	if (read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result))
+		return sprintf(buf, "-1\n");
+	return sprintf(buf, "%lu\n", result);
+}
+
+static ssize_t touchpad_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct ideapad_private *priv = dev_get_drvdata(dev);
+	int ret;
+	bool state;
+
+	ret = kstrtobool(buf, &state);
+	if (ret)
+		return ret;
+
+	ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, state);
+	if (ret < 0)
+		return -EIO;
+	return count;
+}
+
+static DEVICE_ATTR_RO(touchpad);
+
 static struct attribute *ideapad_attributes[] = {
 	&dev_attr_camera_power.attr,
 	&dev_attr_fan_mode.attr,
+	&dev_attr_touchpad.attr,
 	NULL
 };
 
-- 
2.11.0


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[Andy Shevchenko]

On Wed, May 3, 2017 at 5:36 PM, Ritesh Raj Sarraf <rrs@debian.org> wrote:
> Resending again, as Google servers are behaving weird lately.
>
> On Sun, 2017-04-30 at 15:54 +0300, Andy Shevchenko wrote:
>> > > the main issue that driver doesn't
>> > > send SW_TABLET_MODE event through input device.
>> >
>> > Well. Yes. That is one part. If SW_TABLET_MODE is done in the driver, much
>> > better. My patch was simply in line with some of the other drivers (hp-wmi
>> > and
>> > thinkpad_acpi) to get it working for Lenovo Yoga series.
>>
>> sysfs ABI for drivers that provide input interface is quite strong for
>> my opinion. It means I'm not totally objecting, but I would accept it
>> if and only if there is nothing else could be done.
>
> The need we have in the user application is for read-only access.

I does not matter at all! You are proposing a part of ABI which will
be closer to what is "carved in stone". Here is the problem.

So, I'm really trying hard to get avoid such "small things" which
would make our lives quite hard in long term prospective while fixing
short-term issues in a good will.

Please, consider to do it better. For now I didn't hear any proof that
there is no other way to achieve your goal.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[Darren Hart]

On Wed, May 03, 2017 at 05:42:02PM +0300, Andy Shevchenko wrote:
> On Wed, May 3, 2017 at 5:36 PM, Ritesh Raj Sarraf <rrs@debian.org> wrote:
> > Resending again, as Google servers are behaving weird lately.
> >
> > On Sun, 2017-04-30 at 15:54 +0300, Andy Shevchenko wrote:
> >> > > the main issue that driver doesn't
> >> > > send SW_TABLET_MODE event through input device.
> >> >
> >> > Well. Yes. That is one part. If SW_TABLET_MODE is done in the driver, much
> >> > better. My patch was simply in line with some of the other drivers (hp-wmi
> >> > and
> >> > thinkpad_acpi) to get it working for Lenovo Yoga series.
> >>
> >> sysfs ABI for drivers that provide input interface is quite strong for
> >> my opinion. It means I'm not totally objecting, but I would accept it
> >> if and only if there is nothing else could be done.
> >
> > The need we have in the user application is for read-only access.
> 
> I does not matter at all! You are proposing a part of ABI which will
> be closer to what is "carved in stone". Here is the problem.
> 
> So, I'm really trying hard to get avoid such "small things" which
> would make our lives quite hard in long term prospective while fixing
> short-term issues in a good will.
> 
> Please, consider to do it better. For now I didn't hear any proof that
> there is no other way to achieve your goal.

I spent a day with the Lenovo Yoga 2 11 I have available, and have not yet found
a reliable means of detecting the tablet mode (reverse engineering these things
isn't something I'm expert at though). I've reached out to the original author
for some context, but that didn't lead to any revelations as of yet.

Ultimately, this sysfs attribute is no better or worse than others that exist in
our tree. So until we can provide a better solution, we do need to be careful
to not inadvertently favor one laptop driver over another while seeking to avoid
sysfs stable API issues.

I do think we need to start distinguishing between core kernel userspace
interfaces and leaf node driver sysfs attributes.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v4] platform/x86: ideapad-laptop: Add sysfs interface for touchpad state

[Andy Shevchenko]

On Wed, May 3, 2017 at 7:11 PM, Darren Hart <dvhart@infradead.org> wrote:
> On Wed, May 03, 2017 at 05:42:02PM +0300, Andy Shevchenko wrote:
>> On Wed, May 3, 2017 at 5:36 PM, Ritesh Raj Sarraf <rrs@debian.org> wrote:
>> > Resending again, as Google servers are behaving weird lately.
>> >
>> > On Sun, 2017-04-30 at 15:54 +0300, Andy Shevchenko wrote:
>> >> > > the main issue that driver doesn't
>> >> > > send SW_TABLET_MODE event through input device.
>> >> >
>> >> > Well. Yes. That is one part. If SW_TABLET_MODE is done in the driver, much
>> >> > better. My patch was simply in line with some of the other drivers (hp-wmi
>> >> > and
>> >> > thinkpad_acpi) to get it working for Lenovo Yoga series.
>> >>
>> >> sysfs ABI for drivers that provide input interface is quite strong for
>> >> my opinion. It means I'm not totally objecting, but I would accept it
>> >> if and only if there is nothing else could be done.
>> >
>> > The need we have in the user application is for read-only access.
>>
>> I does not matter at all! You are proposing a part of ABI which will
>> be closer to what is "carved in stone". Here is the problem.
>>
>> So, I'm really trying hard to get avoid such "small things" which
>> would make our lives quite hard in long term prospective while fixing
>> short-term issues in a good will.
>>
>> Please, consider to do it better. For now I didn't hear any proof that
>> there is no other way to achieve your goal.
>
> I spent a day with the Lenovo Yoga 2 11 I have available, and have not yet found
> a reliable means of detecting the tablet mode (reverse engineering these things
> isn't something I'm expert at though). I've reached out to the original author
> for some context, but that didn't lead to any revelations as of yet.

Okay, I have pushed to testing current version and put on top the
separate patch for switching attribute to be RO.
(I just realized it might produce a warning since we have defined and
not used function.)

> Ultimately, this sysfs attribute is no better or worse than others that exist in
> our tree. So until we can provide a better solution, we do need to be careful
> to not inadvertently favor one laptop driver over another while seeking to avoid
> sysfs stable API issues.
>
> I do think we need to start distinguishing between core kernel userspace
> interfaces and leaf node driver sysfs attributes.

More flexibility -- more ways to abuse it OTOH :-)

-- 
With Best Regards,
Andy Shevchenko