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

[Takashi Iwai <tiwai@suse.de>]

At Wed, 21 Apr 2010 10:23:22 -0700,
Dmitry Torokhov wrote:
> 
> On Wed, Apr 21, 2010 at 04:46:39PM +0200, Takashi Iwai wrote:
> > The new Synaptics devices have an LED on the top-left corner.
> > This patch adds a new LED class device to control it.  It's created
> > dynamically upon synaptics device probing.
> > 
> > The LED is controlled via the command 0x0a with parameters 0x88 or 0x10.
> > This seems only on/off control although other value might be accepted.
> > 
> > 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.
> >
> 
> Tkashi,
> 
> Does it build if you disable LED subsystem? Note that I don't want
> psmouse to depend on LEDs...

Oh, I missed that dependency, yes.
I'll fix to change to select LEDS on demand.

> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > 
> > Fixed a typo in v2 patch, sorry.
> > 
> >  drivers/input/mouse/synaptics.c |   88 +++++++++++++++++++++++++++++++++++++++
> >  drivers/input/mouse/synaptics.h |    3 +
> >  2 files changed, 91 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index c7b5285..fcc007c 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/leds.h>
> >  #include <linux/slab.h>
> >  #include "psmouse.h"
> >  #include "synaptics.h"
> > @@ -335,6 +336,79 @@ static void synaptics_pt_create(struct psmouse *psmouse)
> >  	serio_register_port(serio);
> >  }
> >  
> > +/*
> > + * LED handling:
> > + * Some Synaptics devices have an embeded LED at the top-left corner.
> > + */
> > +
> > +struct synaptics_led {
> > +	enum led_brightness status;
> > +	struct psmouse *psmouse;
> > +	struct work_struct work;
> > +	struct led_classdev cdev;
> > +};
> > +
> > +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_led *led;
> > +
> > +	led = container_of(work, struct synaptics_led, work);
> > +	synaptics_set_led(led->psmouse, led->status);
> 
> 2+ instances of work may run simultaneously on 2 CPUs; I think you need
> lcokign here. Also you need locking to prevent psmouse core access the
> device (send other commands) when user accesses sysfs attributes.

ps2_command() and co have already mutex in it.  I thought this should
suffice. Meanwhile, it'd be better to protect the whole command
sequence at once.  I'll change to call ps2_begin_command/end_command()
explicitly.

> Also,
> doesn't the device have to be in command mode before you start sending
> commands to it?

Looks not.


> > +}
> > +
> > +static void synaptics_led_cdev_brightness_set(struct led_classdev *cdev,
> > +					      enum led_brightness value)
> > +{
> > +	struct synaptics_led *led;
> > +
> > +	led = container_of(cdev, struct synaptics_led, cdev);
> > +	if (value != led->status) {
> > +		led->status = value;
> > +		schedule_work(&led->work);
> > +	}
> > +}
> > +
> > +static int synaptics_init_led(struct psmouse *psmouse)
> > +{
> > +	struct synaptics_data *priv = psmouse->private;
> > +	struct synaptics_led *led;
> > +	int err;
> > +
> > +	/* 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)
> > +		return 0;
> > +
> > +	printk(KERN_INFO "synaptics: support LED control\n");
> > +	led = kzalloc(sizeof(struct synaptics_led), GFP_KERNEL);
> > +	if (!led)
> > +		return -ENOMEM;
> 
> I do not think synaptics_led structure uses that much space to be
> allocated separately and conditionally; why don't you just put lde, work
> and value directly into synaptics_data?

Because I didn't want to pollute synaptics.h by dependency of led
stuff, wanted to make synaptics.c rather self-contained.  It'll be
ifdef'ed, and it's not beautiful to see much in the header that is
referred in other file.


thanks,

Takashi