[PATCH 0/2] Synaptics Clickpad support

[PATCH 0/2] Synaptics Clickpad support

[Takashi Iwai]

Hi,

here are small patches to add a support of Synaptics Clickpad devices;
more exactly, it adds the way for X11 synaptics driver detecting the
Clickpad device to handle its specific features.

Unlike my previous patch, this version gives little additions in the
kernel side.  It changes the bit-mask of supported buttons and
converts the button event to the left-button.  And, with the second
patch, it adds a capability of LED input event.

The corresponding patches required for xorg synaptics driver will be
posted to xorg ML soon later.  If anyone needs them beforehand, I can
mail them beforehand, of course.


thanks,

Takashi

[PATCH 1/2] input: Add support of Synaptics Clickpad device

[Takashi Iwai]

Add the detection of Synaptics Clickpad device.
The device can be detected a new query command 0x0c.  The clickpad
flags are in cap[0]:4 and cap[1]:0 bits.  But, the driver checks
first the product id bits in the ext capabilities to be sure, so
that it skips the new check on older devices.

When the device is detected, the driver now reports only the left
button as the supported buttons so that X11 driver can detect that
the device is Clickpad.  A Clickpad device gives the button events
only as the middle button.  The kernel driver morphs to the left
button.  The real handling of Clickpad is done rather in X driver
side.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/input/mouse/synaptics.c |   32 ++++++++++++++++++++++++++++++++
 drivers/input/mouse/synaptics.h |    4 ++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 026df60..6a51542 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -328,6 +328,24 @@ static void synaptics_pt_create(struct psmouse *psmouse)
  *	Functions to interpret the absolute mode packets
  ****************************************************************************/
 
+static void synaptics_check_clickpad(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+	unsigned char ncap[3];
+
+	/* check the new capability bits only on known working devices */
+	if (SYN_CAP_PRODUCT_ID(priv->ext_cap) != 0xe4)
+		return;
+	if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_CAPAB2, ncap))
+		return;
+	printk(KERN_INFO "Synaptics: newcap: %02x:%02x:%02x\n",
+	       ncap[0], ncap[1], ncap[2]);
+	priv->clickpad = ((ncap[0] & 0x10) >> 4) | ((ncap[1] & 0x01) << 1);
+	if (priv->clickpad)
+		printk(KERN_INFO "Synaptics: Clickpad device detected: %d\n",
+		       priv->clickpad);
+}
+
 static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data *priv, struct synaptics_hw_state *hw)
 {
 	memset(hw, 0, sizeof(struct synaptics_hw_state));
@@ -354,6 +372,13 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
 				hw->scroll = (signed char)(buf[1]);
 		}
 
+		if (priv->clickpad) {
+			/* clickpad reports only the middle button, report
+			 * it as the left button
+			 */
+			hw->left = ((buf[0] ^ buf[3]) & 0x01) ? 1 : 0;
+		}
+
 		if (SYN_CAP_FOUR_BUTTON(priv->capabilities)) {
 			hw->up   = ((buf[0] ^ buf[3]) & 0x01) ? 1 : 0;
 			hw->down = ((buf[0] ^ buf[3]) & 0x02) ? 1 : 0;
@@ -593,6 +618,11 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 
 	dev->absres[ABS_X] = priv->x_res;
 	dev->absres[ABS_Y] = priv->y_res;
+
+	if (priv->clickpad) {
+		__clear_bit(BTN_RIGHT, dev->keybit); /* only left-button */
+		__clear_bit(BTN_MIDDLE, dev->keybit);
+	}
 }
 
 static void synaptics_disconnect(struct psmouse *psmouse)
@@ -702,6 +732,8 @@ int synaptics_init(struct psmouse *psmouse)
 		SYN_ID_MAJOR(priv->identity), SYN_ID_MINOR(priv->identity),
 		priv->model_id, priv->capabilities, priv->ext_cap);
 
+	synaptics_check_clickpad(psmouse);
+
 	set_input_params(psmouse->dev, priv);
 
 	/*
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index f0f40a3..b824851 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -18,6 +18,7 @@
 #define SYN_QUE_SERIAL_NUMBER_SUFFIX	0x07
 #define SYN_QUE_RESOLUTION		0x08
 #define SYN_QUE_EXT_CAPAB		0x09
+#define SYN_QUE_EXT_CAPAB2		0x0c
 
 /* synatics modes */
 #define SYN_BIT_ABSOLUTE_MODE		(1 << 7)
@@ -48,6 +49,7 @@
 #define SYN_CAP_VALID(c)		((((c) & 0x00ff00) >> 8) == 0x47)
 #define SYN_EXT_CAP_REQUESTS(c)		(((c) & 0x700000) >> 20)
 #define SYN_CAP_MULTI_BUTTON_NO(ec)	(((ec) & 0x00f000) >> 12)
+#define SYN_CAP_PRODUCT_ID(ec)		(((ec) & 0xff0000) >> 16)
 
 /* synaptics modes query bits */
 #define SYN_MODE_ABSOLUTE(m)		((m) & (1 << 7))
@@ -103,6 +105,8 @@ struct synaptics_data {
 	unsigned char pkt_type;			/* packet type - old, new, etc */
 	unsigned char mode;			/* current mode byte */
 	int scroll;
+
+	unsigned char clickpad;
 };
 
 void synaptics_module_init(void);
-- 
1.7.0.4

Re: [PATCH 1/2] input: Add support of Synaptics Clickpad device

[Dmitry Torokhov]

Hi Takashi,

On Wed, Apr 14, 2010 at 05:10:22PM +0200, Takashi Iwai wrote:
> Add the detection of Synaptics Clickpad device.
> The device can be detected a new query command 0x0c.  The clickpad
> flags are in cap[0]:4 and cap[1]:0 bits.  But, the driver checks
> first the product id bits in the ext capabilities to be sure, so
> that it skips the new check on older devices.
> 

Instead of looking at the product id, can we check the number of
supported extended capabilities queries and act accordingly, like the
patch below?

Thanks.

-- 
Dmitry


Input: Add support of Synaptics Clickpad device

From: Takashi Iwai <tiwai@suse.de>

The new type of touchpads can be detected via a new query command 0x0c.
The clickpad flags are in cap[0]:4 and cap[1]:0 bits.

When the device is detected, the driver now reports only the left
button as the supported buttons so that X11 driver can detect that
the device is Clickpad.  A Clickpad device gives the button events
only as the middle button.  The kernel driver morphs to the left
button.  The real handling of Clickpad is done rather in X driver
side.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/mouse/synaptics.c |   33 +++++++++++++++++++++++++++++----
 drivers/input/mouse/synaptics.h |    4 ++++
 2 files changed, 33 insertions(+), 4 deletions(-)


diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 026df60..c7b5285 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -137,7 +137,8 @@ static int synaptics_capability(struct psmouse *psmouse)
 	if (synaptics_send_cmd(psmouse, SYN_QUE_CAPABILITIES, cap))
 		return -1;
 	priv->capabilities = (cap[0] << 16) | (cap[1] << 8) | cap[2];
-	priv->ext_cap = 0;
+	priv->ext_cap = priv->ext_cap_0c = 0;
+
 	if (!SYN_CAP_VALID(priv->capabilities))
 		return -1;
 
@@ -162,6 +163,16 @@ static int synaptics_capability(struct psmouse *psmouse)
 				priv->ext_cap &= 0xff0fff;
 		}
 	}
+
+	if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 4) {
+		if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_CAPAB_0C, cap)) {
+			printk(KERN_ERR "Synaptics claims to have extended capability 0x0c,"
+			       " but I'm not able to read it.");
+		} else {
+			priv->ext_cap_0c = (cap[0] << 16) | (cap[1] << 8) | cap[2];
+		}
+	}
+
 	return 0;
 }
 
@@ -348,7 +359,15 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
 		hw->left  = (buf[0] & 0x01) ? 1 : 0;
 		hw->right = (buf[0] & 0x02) ? 1 : 0;
 
-		if (SYN_CAP_MIDDLE_BUTTON(priv->capabilities)) {
+		if (SYN_CAP_CLICKPAD(priv->ext_cap_0c)) {
+			/*
+			 * Clickpad's button is transmitted as middle button,
+			 * however, since it is primary button, we will report
+			 * it as BTN_LEFT.
+			 */
+			hw->left = ((buf[0] ^ buf[3]) & 0x01) ? 1 : 0;
+
+		} else if (SYN_CAP_MIDDLE_BUTTON(priv->capabilities)) {
 			hw->middle = ((buf[0] ^ buf[3]) & 0x01) ? 1 : 0;
 			if (hw->w == 2)
 				hw->scroll = (signed char)(buf[1]);
@@ -593,6 +612,12 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 
 	dev->absres[ABS_X] = priv->x_res;
 	dev->absres[ABS_Y] = priv->y_res;
+
+	if (SYN_CAP_CLICKPAD(priv->ext_cap_0c)) {
+		/* Clickpads report only left button */
+		__clear_bit(BTN_RIGHT, dev->keybit);
+		__clear_bit(BTN_MIDDLE, dev->keybit);
+	}
 }
 
 static void synaptics_disconnect(struct psmouse *psmouse)
@@ -697,10 +722,10 @@ int synaptics_init(struct psmouse *psmouse)
 
 	priv->pkt_type = SYN_MODEL_NEWABS(priv->model_id) ? SYN_NEWABS : SYN_OLDABS;
 
-	printk(KERN_INFO "Synaptics Touchpad, model: %ld, fw: %ld.%ld, id: %#lx, caps: %#lx/%#lx\n",
+	printk(KERN_INFO "Synaptics Touchpad, model: %ld, fw: %ld.%ld, id: %#lx, caps: %#lx/%#lx/%#lx\n",
 		SYN_ID_MODEL(priv->identity),
 		SYN_ID_MAJOR(priv->identity), SYN_ID_MINOR(priv->identity),
-		priv->model_id, priv->capabilities, priv->ext_cap);
+		priv->model_id, priv->capabilities, priv->ext_cap, priv->ext_cap_0c);
 
 	set_input_params(psmouse->dev, priv);
 
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index f0f40a3..ae37c5d 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -18,6 +18,7 @@
 #define SYN_QUE_SERIAL_NUMBER_SUFFIX	0x07
 #define SYN_QUE_RESOLUTION		0x08
 #define SYN_QUE_EXT_CAPAB		0x09
+#define SYN_QUE_EXT_CAPAB_0C		0x0c
 
 /* synatics modes */
 #define SYN_BIT_ABSOLUTE_MODE		(1 << 7)
@@ -48,6 +49,8 @@
 #define SYN_CAP_VALID(c)		((((c) & 0x00ff00) >> 8) == 0x47)
 #define SYN_EXT_CAP_REQUESTS(c)		(((c) & 0x700000) >> 20)
 #define SYN_CAP_MULTI_BUTTON_NO(ec)	(((ec) & 0x00f000) >> 12)
+#define SYN_CAP_PRODUCT_ID(ec)		(((ec) & 0xff0000) >> 16)
+#define SYN_CAP_CLICKPAD(ex0c)		((ex0c) & 0x100100)
 
 /* synaptics modes query bits */
 #define SYN_MODE_ABSOLUTE(m)		((m) & (1 << 7))
@@ -96,6 +99,7 @@ struct synaptics_data {
 	unsigned long int model_id;		/* Model-ID */
 	unsigned long int capabilities;		/* Capabilities */
 	unsigned long int ext_cap;		/* Extended Capabilities */
+	unsigned long int ext_cap_0c;		/* Ext Caps from 0x0c query */
 	unsigned long int identity;		/* Identification */
 	int x_res;				/* X resolution in units/mm */
 	int y_res;				/* Y resolution in units/mm */

Re: [PATCH 1/2] input: Add support of Synaptics Clickpad device

[Takashi Iwai]

Hi Dmitry,

At Mon, 19 Apr 2010 01:32:22 -0700,
Dmitry Torokhov wrote:
> 
> Hi Takashi,
> 
> On Wed, Apr 14, 2010 at 05:10:22PM +0200, Takashi Iwai wrote:
> > Add the detection of Synaptics Clickpad device.
> > The device can be detected a new query command 0x0c.  The clickpad
> > flags are in cap[0]:4 and cap[1]:0 bits.  But, the driver checks
> > first the product id bits in the ext capabilities to be sure, so
> > that it skips the new check on older devices.
> > 
> 
> Instead of looking at the product id, can we check the number of
> supported extended capabilities queries and act accordingly, like the
> patch below?

Yes, it worked.  (Though, I've tested only new machines.)


> @@ -162,6 +163,16 @@ static int synaptics_capability(struct psmouse *psmouse)
>  				priv->ext_cap &= 0xff0fff;
>  		}
>  	}
> +
> +	if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 4) {
> +		if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_CAPAB_0C, cap)) {
> +			printk(KERN_ERR "Synaptics claims to have extended capability 0x0c,"
> +			       " but I'm not able to read it.");

Here missing a newline, BTW.


thanks,

Takashi

Re: [PATCH 1/2] input: Add support of Synaptics Clickpad device

[Dmitry Torokhov]

On Mon, Apr 19, 2010 at 12:29:49PM +0200, Takashi Iwai wrote:
> Hi Dmitry,
> 
> At Mon, 19 Apr 2010 01:32:22 -0700,
> Dmitry Torokhov wrote:
> > 
> > Hi Takashi,
> > 
> > On Wed, Apr 14, 2010 at 05:10:22PM +0200, Takashi Iwai wrote:
> > > Add the detection of Synaptics Clickpad device.
> > > The device can be detected a new query command 0x0c.  The clickpad
> > > flags are in cap[0]:4 and cap[1]:0 bits.  But, the driver checks
> > > first the product id bits in the ext capabilities to be sure, so
> > > that it skips the new check on older devices.
> > > 
> > 
> > Instead of looking at the product id, can we check the number of
> > supported extended capabilities queries and act accordingly, like the
> > patch below?
> 
> Yes, it worked.  (Though, I've tested only new machines.)
> 


Seems to be working on older (at least one ;) ) as well.

> 
> > @@ -162,6 +163,16 @@ static int synaptics_capability(struct psmouse *psmouse)
> >  				priv->ext_cap &= 0xff0fff;
> >  		}
> >  	}
> > +
> > +	if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 4) {
> > +		if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_CAPAB_0C, cap)) {
> > +			printk(KERN_ERR "Synaptics claims to have extended capability 0x0c,"
> > +			       " but I'm not able to read it.");
> 
> Here missing a newline, BTW.
> 

Fixed. Thank you for testing. I have that patch in 'for-linus' for .34.
Please push your synaptics X changes upstream as well.

-- 
Dmitry

Re: [PATCH 1/2] input: Add support of Synaptics Clickpad device

[Takashi Iwai]

At Tue, 20 Apr 2010 22:44:55 -0700,
Dmitry Torokhov wrote:
> 
> On Mon, Apr 19, 2010 at 12:29:49PM +0200, Takashi Iwai wrote:
> > Hi Dmitry,
> > 
> > At Mon, 19 Apr 2010 01:32:22 -0700,
> > Dmitry Torokhov wrote:
> > > 
> > > Hi Takashi,
> > > 
> > > On Wed, Apr 14, 2010 at 05:10:22PM +0200, Takashi Iwai wrote:
> > > > Add the detection of Synaptics Clickpad device.
> > > > The device can be detected a new query command 0x0c.  The clickpad
> > > > flags are in cap[0]:4 and cap[1]:0 bits.  But, the driver checks
> > > > first the product id bits in the ext capabilities to be sure, so
> > > > that it skips the new check on older devices.
> > > > 
> > > 
> > > Instead of looking at the product id, can we check the number of
> > > supported extended capabilities queries and act accordingly, like the
> > > patch below?
> > 
> > Yes, it worked.  (Though, I've tested only new machines.)
> > 
> 
> 
> Seems to be working on older (at least one ;) ) as well.
> 
> > 
> > > @@ -162,6 +163,16 @@ static int synaptics_capability(struct psmouse *psmouse)
> > >  				priv->ext_cap &= 0xff0fff;
> > >  		}
> > >  	}
> > > +
> > > +	if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 4) {
> > > +		if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_CAPAB_0C, cap)) {
> > > +			printk(KERN_ERR "Synaptics claims to have extended capability 0x0c,"
> > > +			       " but I'm not able to read it.");
> > 
> > Here missing a newline, BTW.
> > 
> 
> Fixed. Thank you for testing. I have that patch in 'for-linus' for .34.
> Please push your synaptics X changes upstream as well.

Thanks.  It was already submitted.
I'm going to repost the revised X patch later.


Takashi

[PATCH 2/2] input: Add LED support to Synaptics device

[Takashi Iwai]

The new Synaptics devices have an LED on the top-left corner.
This is controlled via the command 0x0a with parameters 0x88 or 0x10.

The detection of the LED isn't clear yet.  It should have been the new
capability bits that indicate the presence, but on real machines, it
doesn't fit.  So, for the time being, the driver checks the product id
in the ext capability bits and assumes that LED exists on the known
devices.

The support of LED is controlled via a normal input event with EV_LED
bit mask.  It supports LED_MUTE bit.  X driver can detect the LED
support by checking these bits.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/input/mouse/synaptics.c |   54 +++++++++++++++++++++++++++++++++++++++
 drivers/input/mouse/synaptics.h |    4 +++
 2 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 6a51542..ea874bd 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -28,6 +28,7 @@
 #include <linux/input.h>
 #include <linux/serio.h>
 #include <linux/libps2.h>
+#include <linux/workqueue.h>
 #include <linux/slab.h>
 #include "psmouse.h"
 #include "synaptics.h"
@@ -328,6 +329,37 @@ static void synaptics_pt_create(struct psmouse *psmouse)
  *	Functions to interpret the absolute mode packets
  ****************************************************************************/
 
+static void synaptics_set_led(struct psmouse *psmouse, int on)
+{
+	unsigned char param[1];
+
+	if (psmouse_sliced_command(psmouse, on ? 0x88 : 0x10))
+		return;
+	param[0] = 0x0a;
+	ps2_command(&psmouse->ps2dev, param, PSMOUSE_CMD_SETRATE);
+}
+
+static void synaptics_led_work(struct work_struct *work)
+{
+	struct synaptics_data *priv = container_of(work, struct synaptics_data, led_work);
+	synaptics_set_led(priv->psmouse, priv->led_status);
+}
+
+/* input event handler: changed by x11 synaptics driver */
+static int synaptics_led_event(struct input_dev *dev, unsigned int type,
+			       unsigned int code, int value)
+{
+	struct synaptics_data *priv = dev->dev.platform_data;
+
+	if (!priv)
+		return 0;
+	if (type == EV_LED && code == LED_MUTE) {
+		priv->led_status = !!value;
+		schedule_work(&priv->led_work);
+	}
+	return 0;
+}
+
 static void synaptics_check_clickpad(struct psmouse *psmouse)
 {
 	struct synaptics_data *priv = psmouse->private;
@@ -344,6 +376,12 @@ static void synaptics_check_clickpad(struct psmouse *psmouse)
 	if (priv->clickpad)
 		printk(KERN_INFO "Synaptics: Clickpad device detected: %d\n",
 		       priv->clickpad);
+	/* FIXME: this should be ncap[1] 0x20, but it's not really... */
+	priv->has_led = 1;
+	if (priv->has_led) {
+		printk(KERN_INFO "Synaptics: support LED control\n");
+		INIT_WORK(&priv->led_work, synaptics_led_work);
+	}
 }
 
 static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data *priv, struct synaptics_hw_state *hw)
@@ -623,10 +661,22 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 		__clear_bit(BTN_RIGHT, dev->keybit); /* only left-button */
 		__clear_bit(BTN_MIDDLE, dev->keybit);
 	}
+	if (priv->has_led) {
+		__set_bit(EV_LED, dev->evbit);
+		__set_bit(LED_MUTE, dev->ledbit);
+		dev->event = synaptics_led_event;
+		dev->dev.platform_data = priv;
+	}
 }
 
 static void synaptics_disconnect(struct psmouse *psmouse)
 {
+	struct synaptics_data *priv = psmouse->private;
+
+	if (priv->has_led) {
+		cancel_work_sync(&priv->led_work);
+		synaptics_set_led(psmouse, 0);
+	}
 	synaptics_reset(psmouse);
 	kfree(psmouse->private);
 	psmouse->private = NULL;
@@ -658,6 +708,9 @@ static int synaptics_reconnect(struct psmouse *psmouse)
 		return -1;
 	}
 
+	if (priv->has_led)
+		synaptics_set_led(psmouse, priv->led_status);
+
 	return 0;
 }
 
@@ -713,6 +766,7 @@ int synaptics_init(struct psmouse *psmouse)
 	if (!priv)
 		return -1;
 
+	priv->psmouse = psmouse;
 	psmouse_reset(psmouse);
 
 	if (synaptics_query_hardware(psmouse)) {
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index b824851..aeca6d2 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -107,6 +107,10 @@ struct synaptics_data {
 	int scroll;
 
 	unsigned char clickpad;
+	unsigned char has_led;
+	unsigned char led_status;
+	struct psmouse *psmouse;
+	struct work_struct led_work;
 };
 
 void synaptics_module_init(void);
-- 
1.7.0.4

Re: [PATCH 2/2] input: Add LED support to Synaptics device

[Pavel Machek]

Hi!

> The new Synaptics devices have an LED on the top-left corner.
> This is controlled via the command 0x0a with parameters 0x88 or 0x10.
> 
> The detection of the LED isn't clear yet.  It should have been the new
> capability bits that indicate the presence, but on real machines, it
> doesn't fit.  So, for the time being, the driver checks the product id
> in the ext capability bits and assumes that LED exists on the known
> devices.
> 
> The support of LED is controlled via a normal input event with EV_LED
> bit mask.  It supports LED_MUTE bit.  X driver can detect the LED
> support by checking these bits.

Could we use generic LED API for this? It is not really 'mute' led
after all...
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/2] input: Add LED support to Synaptics device

[Dmitry Torokhov]

On Thu, Apr 15, 2010 at 09:12:18PM +0200, Pavel Machek wrote:
> Hi!
> 
> > The new Synaptics devices have an LED on the top-left corner.
> > This is controlled via the command 0x0a with parameters 0x88 or 0x10.
> > 
> > The detection of the LED isn't clear yet.  It should have been the new
> > capability bits that indicate the presence, but on real machines, it
> > doesn't fit.  So, for the time being, the driver checks the product id
> > in the ext capability bits and assumes that LED exists on the known
> > devices.
> > 
> > The support of LED is controlled via a normal input event with EV_LED
> > bit mask.  It supports LED_MUTE bit.  X driver can detect the LED
> > support by checking these bits.
> 
> Could we use generic LED API for this? It is not really 'mute' led
> after all...

Yep, LED_MUTE is not for "muting" touchpads.

-- 
Dmitry

Re: [PATCH 2/2] input: Add LED support to Synaptics device

[Takashi Iwai]

At Thu, 15 Apr 2010 21:12:18 +0200,
Pavel Machek wrote:
> 
> Hi!
> 
> > The new Synaptics devices have an LED on the top-left corner.
> > This is controlled via the command 0x0a with parameters 0x88 or 0x10.
> > 
> > The detection of the LED isn't clear yet.  It should have been the new
> > capability bits that indicate the presence, but on real machines, it
> > doesn't fit.  So, for the time being, the driver checks the product id
> > in the ext capability bits and assumes that LED exists on the known
> > devices.
> > 
> > The support of LED is controlled via a normal input event with EV_LED
> > bit mask.  It supports LED_MUTE bit.  X driver can detect the LED
> > support by checking these bits.
> 
> Could we use generic LED API for this?

Yeah, actually I started implementing with LED ADI at first.

But, then it turned out to be that it's much easier to use the
existing LED input bits since this LED is really tightly coupled with
the synaptics input device.  An individual LED device makes hard to
find out the corresponding input device.

If we assume there is only one synaptics and only one synaptics-LED
device, then yes, the situation can be a bit easier, though.

> It is not really 'mute' led after all...

If the problem is the misuse of LED_MUTE bit, how about adding a new
LED bit, e.g. LED_TOUCHPAD?


thanks,

Takashi

Re: [PATCH 2/2] input: Add LED support to Synaptics device

[Takashi Iwai]

At Fri, 16 Apr 2010 10:00:20 +0200,
I wrote:
> 
> At Thu, 15 Apr 2010 21:12:18 +0200,
> Pavel Machek wrote:
> > 
> > Hi!
> > 
> > > The new Synaptics devices have an LED on the top-left corner.
> > > This is controlled via the command 0x0a with parameters 0x88 or 0x10.
> > > 
> > > The detection of the LED isn't clear yet.  It should have been the new
> > > capability bits that indicate the presence, but on real machines, it
> > > doesn't fit.  So, for the time being, the driver checks the product id
> > > in the ext capability bits and assumes that LED exists on the known
> > > devices.
> > > 
> > > The support of LED is controlled via a normal input event with EV_LED
> > > bit mask.  It supports LED_MUTE bit.  X driver can detect the LED
> > > support by checking these bits.
> > 
> > Could we use generic LED API for this?
> 
> Yeah, actually I started implementing with LED ADI at first.
> 
> But, then it turned out to be that it's much easier to use the
> existing LED input bits since this LED is really tightly coupled with
> the synaptics input device.  An individual LED device makes hard to
> find out the corresponding input device.
> 
> If we assume there is only one synaptics and only one synaptics-LED
> device, then yes, the situation can be a bit easier, though.
> 
> > It is not really 'mute' led after all...
> 
> If the problem is the misuse of LED_MUTE bit, how about adding a new
> LED bit, e.g. LED_TOUCHPAD?

The revised patch with an addition of LED_TOUCHPAD is below.


thanks,

Takashi

===

From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] input: Add LED support to Synaptics device

The new Synaptics devices have an LED on the top-left corner.
This is controlled via the command 0x0a with parameters 0x88 or 0x10.

The detection of the LED isn't clear yet.  It should have been the new
capability bits that indicate the presence, but on real machines, it
doesn't fit.  So, for the time being, the driver checks the product id
in the ext capability bits and assumes that LED exists on the known
devices.

The support of LED is controlled via a normal input event with EV_LED
bit mask.  A new LED bitmask, LED_TOUCHPAD, is added.  X driver can
detect the LED support by checking these bits.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/input/mouse/synaptics.c |   64 +++++++++++++++++++++++++++++++++++++++
 drivers/input/mouse/synaptics.h |    5 +++
 include/linux/input.h           |    1 +
 3 files changed, 70 insertions(+), 0 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index c7b5285..bffa474 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -28,6 +28,7 @@
 #include <linux/input.h>
 #include <linux/serio.h>
 #include <linux/libps2.h>
+#include <linux/workqueue.h>
 #include <linux/slab.h>
 #include "psmouse.h"
 #include "synaptics.h"
@@ -339,6 +340,52 @@ static void synaptics_pt_create(struct psmouse *psmouse)
  *	Functions to interpret the absolute mode packets
  ****************************************************************************/
 
+static void synaptics_set_led(struct psmouse *psmouse, int on)
+{
+	unsigned char param[1];
+
+	if (psmouse_sliced_command(psmouse, on ? 0x88 : 0x10))
+		return;
+	param[0] = 0x0a;
+	ps2_command(&psmouse->ps2dev, param, PSMOUSE_CMD_SETRATE);
+}
+
+static void synaptics_led_work(struct work_struct *work)
+{
+	struct synaptics_data *priv = container_of(work, struct synaptics_data, led_work);
+	synaptics_set_led(priv->psmouse, priv->led_status);
+}
+
+/* input event handler: changed by x11 synaptics driver */
+static int synaptics_led_event(struct input_dev *dev, unsigned int type,
+			       unsigned int code, int value)
+{
+	struct synaptics_data *priv = dev->dev.platform_data;
+
+	if (!priv)
+		return 0;
+	if (type == EV_LED && code == LED_TOUCHPAD) {
+		priv->led_status = !!value;
+		schedule_work(&priv->led_work);
+	}
+	return 0;
+}
+
+static void synaptics_query_led(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+
+	/* FIXME: LED is supposedly detectable in cap0c[1] 0x20, but it seems
+	 * not working on real machines.  So we check the product id to be sure
+	 */
+	if (priv->ext_cap_0c && SYN_CAP_PRODUCT_ID(priv->ext_cap) == 0xe4)
+		printk(KERN_INFO "synaptics: support LED control\n");
+		priv->has_led = 1;
+		priv->psmouse = psmouse;
+		INIT_WORK(&priv->led_work, synaptics_led_work);
+	}
+}
+
 static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data *priv, struct synaptics_hw_state *hw)
 {
 	memset(hw, 0, sizeof(struct synaptics_hw_state));
@@ -618,10 +665,22 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 		__clear_bit(BTN_RIGHT, dev->keybit);
 		__clear_bit(BTN_MIDDLE, dev->keybit);
 	}
+	if (priv->has_led) {
+		__set_bit(EV_LED, dev->evbit);
+		__set_bit(LED_TOUCHPAD, dev->ledbit);
+		dev->event = synaptics_led_event;
+		dev->dev.platform_data = priv;
+	}
 }
 
 static void synaptics_disconnect(struct psmouse *psmouse)
 {
+	struct synaptics_data *priv = psmouse->private;
+
+	if (priv->has_led) {
+		cancel_work_sync(&priv->led_work);
+		synaptics_set_led(psmouse, 0);
+	}
 	synaptics_reset(psmouse);
 	kfree(psmouse->private);
 	psmouse->private = NULL;
@@ -653,6 +712,9 @@ static int synaptics_reconnect(struct psmouse *psmouse)
 		return -1;
 	}
 
+	if (priv->has_led)
+		synaptics_set_led(psmouse, priv->led_status);
+
 	return 0;
 }
 
@@ -727,6 +789,8 @@ int synaptics_init(struct psmouse *psmouse)
 		SYN_ID_MAJOR(priv->identity), SYN_ID_MINOR(priv->identity),
 		priv->model_id, priv->capabilities, priv->ext_cap, priv->ext_cap_0c);
 
+	synaptics_query_led(psmouse);
+
 	set_input_params(psmouse->dev, priv);
 
 	/*
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index ae37c5d..0b989f4 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -107,6 +107,11 @@ struct synaptics_data {
 	unsigned char pkt_type;			/* packet type - old, new, etc */
 	unsigned char mode;			/* current mode byte */
 	int scroll;
+
+	unsigned char has_led;
+	unsigned char led_status;
+	struct psmouse *psmouse;
+	struct work_struct led_work;
 };
 
 void synaptics_module_init(void);
diff --git a/include/linux/input.h b/include/linux/input.h
index 7ed2251..0aea50d 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -757,6 +757,7 @@ struct input_absinfo {
 #define LED_MISC		0x08
 #define LED_MAIL		0x09
 #define LED_CHARGING		0x0a
+#define LED_TOUCHPAD		0x0b
 #define LED_MAX			0x0f
 #define LED_CNT			(LED_MAX+1)
 
-- 
1.7.0.4

Re: [PATCH 2/2] input: Add LED support to Synaptics device

[Dmitry Torokhov]

On Mon, Apr 19, 2010 at 12:44:08PM +0200, Takashi Iwai wrote:
> At Fri, 16 Apr 2010 10:00:20 +0200,
> I wrote:
> > 
> > At Thu, 15 Apr 2010 21:12:18 +0200,
> > Pavel Machek wrote:
> > > 
> > > Hi!
> > > 
> > > > The new Synaptics devices have an LED on the top-left corner.
> > > > This is controlled via the command 0x0a with parameters 0x88 or 0x10.
> > > > 
> > > > The detection of the LED isn't clear yet.  It should have been the new
> > > > capability bits that indicate the presence, but on real machines, it
> > > > doesn't fit.  So, for the time being, the driver checks the product id
> > > > in the ext capability bits and assumes that LED exists on the known
> > > > devices.
> > > > 
> > > > The support of LED is controlled via a normal input event with EV_LED
> > > > bit mask.  It supports LED_MUTE bit.  X driver can detect the LED
> > > > support by checking these bits.
> > > 
> > > Could we use generic LED API for this?
> > 
> > Yeah, actually I started implementing with LED ADI at first.
> > 
> > But, then it turned out to be that it's much easier to use the
> > existing LED input bits since this LED is really tightly coupled with
> > the synaptics input device.  An individual LED device makes hard to
> > find out the corresponding input device.
> > 
> > If we assume there is only one synaptics and only one synaptics-LED
> > device, then yes, the situation can be a bit easier, though.
> > 
> > > It is not really 'mute' led after all...
> > 
> > If the problem is the misuse of LED_MUTE bit, how about adding a new
> > LED bit, e.g. LED_TOUCHPAD?
> 
> The revised patch with an addition of LED_TOUCHPAD is below.
> 

Sorry Takashi, but I will not add any new LED types to input. Even
current input LEDs are going to be accessible thought standard LED
framework (even though I have not apploed Samuel's patch yet I do think
it would move kernel in the right direction).

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] input: Add LED support to Synaptics device

[Takashi Iwai]

At Tue, 20 Apr 2010 22:43:03 -0700,
Dmitry Torokhov wrote:
> 
> On Mon, Apr 19, 2010 at 12:44:08PM +0200, Takashi Iwai wrote:
> > At Fri, 16 Apr 2010 10:00:20 +0200,
> > I wrote:
> > > 
> > > At Thu, 15 Apr 2010 21:12:18 +0200,
> > > Pavel Machek wrote:
> > > > 
> > > > Hi!
> > > > 
> > > > > The new Synaptics devices have an LED on the top-left corner.
> > > > > This is controlled via the command 0x0a with parameters 0x88 or 0x10.
> > > > > 
> > > > > The detection of the LED isn't clear yet.  It should have been the new
> > > > > capability bits that indicate the presence, but on real machines, it
> > > > > doesn't fit.  So, for the time being, the driver checks the product id
> > > > > in the ext capability bits and assumes that LED exists on the known
> > > > > devices.
> > > > > 
> > > > > The support of LED is controlled via a normal input event with EV_LED
> > > > > bit mask.  It supports LED_MUTE bit.  X driver can detect the LED
> > > > > support by checking these bits.
> > > > 
> > > > Could we use generic LED API for this?
> > > 
> > > Yeah, actually I started implementing with LED ADI at first.
> > > 
> > > But, then it turned out to be that it's much easier to use the
> > > existing LED input bits since this LED is really tightly coupled with
> > > the synaptics input device.  An individual LED device makes hard to
> > > find out the corresponding input device.
> > > 
> > > If we assume there is only one synaptics and only one synaptics-LED
> > > device, then yes, the situation can be a bit easier, though.
> > > 
> > > > It is not really 'mute' led after all...
> > > 
> > > If the problem is the misuse of LED_MUTE bit, how about adding a new
> > > LED bit, e.g. LED_TOUCHPAD?
> > 
> > The revised patch with an addition of LED_TOUCHPAD is below.
> > 
> 
> Sorry Takashi, but I will not add any new LED types to input. Even
> current input LEDs are going to be accessible thought standard LED
> framework (even though I have not apploed Samuel's patch yet I do think
> it would move kernel in the right direction).

Hrm, OK, we can live in other way, too.  For touchpad, it wouldn't be
a big problem because it's likely a single device.

But, how can we link a led class device and another device, in
general?

Also, another remaining question is the lifetime of led device.
The mouse device tends to be re-assigned often, e.g. at each time you
suspend/hibernate.  Should led device also be removed and revived at
each time, or should we keep it and just ignore event?  If we remove
it, how can we avoid race?


thanks,

Takashi

Re: [PATCH 2/2] input: Add LED support to Synaptics device

[Dmitry Torokhov]

On Wed, Apr 21, 2010 at 08:31:15AM +0200, Takashi Iwai wrote:
> At Tue, 20 Apr 2010 22:43:03 -0700,
> Dmitry Torokhov wrote:
> > 
> > On Mon, Apr 19, 2010 at 12:44:08PM +0200, Takashi Iwai wrote:
> > > At Fri, 16 Apr 2010 10:00:20 +0200,
> > > I wrote:
> > > > 
> > > > At Thu, 15 Apr 2010 21:12:18 +0200,
> > > > Pavel Machek wrote:
> > > > > 
> > > > > Hi!
> > > > > 
> > > > > > The new Synaptics devices have an LED on the top-left corner.
> > > > > > This is controlled via the command 0x0a with parameters 0x88 or 0x10.
> > > > > > 
> > > > > > The detection of the LED isn't clear yet.  It should have been the new
> > > > > > capability bits that indicate the presence, but on real machines, it
> > > > > > doesn't fit.  So, for the time being, the driver checks the product id
> > > > > > in the ext capability bits and assumes that LED exists on the known
> > > > > > devices.
> > > > > > 
> > > > > > The support of LED is controlled via a normal input event with EV_LED
> > > > > > bit mask.  It supports LED_MUTE bit.  X driver can detect the LED
> > > > > > support by checking these bits.
> > > > > 
> > > > > Could we use generic LED API for this?
> > > > 
> > > > Yeah, actually I started implementing with LED ADI at first.
> > > > 
> > > > But, then it turned out to be that it's much easier to use the
> > > > existing LED input bits since this LED is really tightly coupled with
> > > > the synaptics input device.  An individual LED device makes hard to
> > > > find out the corresponding input device.
> > > > 
> > > > If we assume there is only one synaptics and only one synaptics-LED
> > > > device, then yes, the situation can be a bit easier, though.
> > > > 
> > > > > It is not really 'mute' led after all...
> > > > 
> > > > If the problem is the misuse of LED_MUTE bit, how about adding a new
> > > > LED bit, e.g. LED_TOUCHPAD?
> > > 
> > > The revised patch with an addition of LED_TOUCHPAD is below.
> > > 
> > 
> > Sorry Takashi, but I will not add any new LED types to input. Even
> > current input LEDs are going to be accessible thought standard LED
> > framework (even though I have not apploed Samuel's patch yet I do think
> > it would move kernel in the right direction).
> 
> Hrm, OK, we can live in other way, too.  For touchpad, it wouldn't be
> a big problem because it's likely a single device.
> 
> But, how can we link a led class device and another device, in
> general?

I think this question is to Richard, I am a bit fuzzy on LED subsystem.
But in general I'd expect LED have touchpad input device as parent.

> 
> Also, another remaining question is the lifetime of led device.
> The mouse device tends to be re-assigned often, e.g. at each time you
> suspend/hibernate.

We go to a great lengths to properly resume the device keeping the same
input_dev structure and the same event node (for historical reasons
really, nowadays X is hotplug aware so we could do without), do you see
it being recreated often?

>  Should led device also be removed and revived at
> each time, or should we keep it and just ignore event?  If we remove
> it, how can we avoid race?

I am not sure what race you see - psmouse protocol switch should not
race so if you remove LED there you shoudl be race-free. If you see
something racing I am definitely interested in hearing about it.

Thanks!

-- 
Dmitry

Re: [PATCH 2/2] input: Add LED support to Synaptics device

[Takashi Iwai]

At Tue, 20 Apr 2010 23:39:54 -0700,
Dmitry Torokhov wrote:
> 
> On Wed, Apr 21, 2010 at 08:31:15AM +0200, Takashi Iwai wrote:
> > At Tue, 20 Apr 2010 22:43:03 -0700,
> > Dmitry Torokhov wrote:
> > > 
> > > On Mon, Apr 19, 2010 at 12:44:08PM +0200, Takashi Iwai wrote:
> > > > At Fri, 16 Apr 2010 10:00:20 +0200,
> > > > I wrote:
> > > > > 
> > > > > At Thu, 15 Apr 2010 21:12:18 +0200,
> > > > > Pavel Machek wrote:
> > > > > > 
> > > > > > Hi!
> > > > > > 
> > > > > > > The new Synaptics devices have an LED on the top-left corner.
> > > > > > > This is controlled via the command 0x0a with parameters 0x88 or 0x10.
> > > > > > > 
> > > > > > > The detection of the LED isn't clear yet.  It should have been the new
> > > > > > > capability bits that indicate the presence, but on real machines, it
> > > > > > > doesn't fit.  So, for the time being, the driver checks the product id
> > > > > > > in the ext capability bits and assumes that LED exists on the known
> > > > > > > devices.
> > > > > > > 
> > > > > > > The support of LED is controlled via a normal input event with EV_LED
> > > > > > > bit mask.  It supports LED_MUTE bit.  X driver can detect the LED
> > > > > > > support by checking these bits.
> > > > > > 
> > > > > > Could we use generic LED API for this?
> > > > > 
> > > > > Yeah, actually I started implementing with LED ADI at first.
> > > > > 
> > > > > But, then it turned out to be that it's much easier to use the
> > > > > existing LED input bits since this LED is really tightly coupled with
> > > > > the synaptics input device.  An individual LED device makes hard to
> > > > > find out the corresponding input device.
> > > > > 
> > > > > If we assume there is only one synaptics and only one synaptics-LED
> > > > > device, then yes, the situation can be a bit easier, though.
> > > > > 
> > > > > > It is not really 'mute' led after all...
> > > > > 
> > > > > If the problem is the misuse of LED_MUTE bit, how about adding a new
> > > > > LED bit, e.g. LED_TOUCHPAD?
> > > > 
> > > > The revised patch with an addition of LED_TOUCHPAD is below.
> > > > 
> > > 
> > > Sorry Takashi, but I will not add any new LED types to input. Even
> > > current input LEDs are going to be accessible thought standard LED
> > > framework (even though I have not apploed Samuel's patch yet I do think
> > > it would move kernel in the right direction).
> > 
> > Hrm, OK, we can live in other way, too.  For touchpad, it wouldn't be
> > a big problem because it's likely a single device.
> > 
> > But, how can we link a led class device and another device, in
> > general?
> 
> I think this question is to Richard, I am a bit fuzzy on LED subsystem.
> But in general I'd expect LED have touchpad input device as parent.

This might work, yes.

> > Also, another remaining question is the lifetime of led device.
> > The mouse device tends to be re-assigned often, e.g. at each time you
> > suspend/hibernate.
> 
> We go to a great lengths to properly resume the device keeping the same
> input_dev structure and the same event node (for historical reasons
> really, nowadays X is hotplug aware so we could do without), do you see
> it being recreated often?

Maybe my test system is old; it's not Xorg 1.8 and not using dynamic X
config.  At least, X driver loses sometimes the device upon resume,
and it required reopening.

> >  Should led device also be removed and revived at
> > each time, or should we keep it and just ignore event?  If we remove
> > it, how can we avoid race?
> 
> I am not sure what race you see - psmouse protocol switch should not
> race so if you remove LED there you shoudl be race-free. If you see
> something racing I am definitely interested in hearing about it.

Well, what I thought of is when the led sysfs doesn't exist even
though the input device is created.  Or, vice versa, the led sysfs
remains while the input device is deactivated.

But this might not be a problem when I create the led device in
probe.  Let's see in the real world.


thanks,

Takashi