[PATCH 03/18] Add the embedded LED support

[Takashi Iwai]

At Wed, 13 Oct 2010 15:56:38 +1000,
Peter Hutterer wrote:
> 
> On Wed, Oct 13, 2010 at 07:47:32AM +0200, Takashi Iwai wrote:
> > At Wed, 13 Oct 2010 13:29:21 +1000,
> > Peter Hutterer wrote:
> > > 
> > > On Fri, Oct 08, 2010 at 07:22:27PM +0200, Takashi Iwai wrote:
> > > > This patch adds the control of the embedded LED on the top-left corner
> > > > of new Synaptics devices.  For LED control, it requires the patch to
> > > > Linux synaptics input driver,
> > > > 	https://patchwork.kernel.org/patch/92434/
> > > > 
> > > > When a sysfs file /sys/class/leds/psmouse::synaptics exists, the driver
> > > > assumes it supports the embeded LED control.
> > > > 
> > > > The LED can be controlled via new properties, "Synaptics LED" and
> > > > "Synaptics LED Status".
> > > > 
> > > > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > > > ---
> > > >  include/synaptics-properties.h |    6 ++++++
> > > >  man/synaptics.man              |    9 +++++++++
> > > >  src/eventcomm.c                |   32 +++++++++++++++++++++++++++++++-
> > > >  src/properties.c               |   15 +++++++++++++++
> > > >  src/synapticsstr.h             |    2 ++
> > > >  src/synproto.h                 |    1 +
> > > >  tools/synclient.c              |    1 +
> > > >  7 files changed, 65 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/include/synaptics-properties.h b/include/synaptics-properties.h
> > > > index 9c6a2ee..dd7e259 100644
> > > > --- a/include/synaptics-properties.h
> > > > +++ b/include/synaptics-properties.h
> > > > @@ -155,4 +155,10 @@
> > > >  /* 32 bit, 4 values, left, right, top, bottom */
> > > >  #define SYNAPTICS_PROP_AREA "Synaptics Area"
> > > >  
> > > > +/* 8 bit (BOOL, read-only), has_led */
> > > > +#define SYNAPTICS_PROP_LED "Synaptics LED"
> > > > +
> > > 
> > > this should be added to the "Synaptics Capabilities" property, there's no
> > > need for a separate property.
> > 
> > OK, sounds sensible.
> > 
> > > I guess sooner or later we'll see non-clickpad
> > > devices come out with leds too, making this a generic capability like
> > > two-finger tapping and similar.
> > 
> > There are already lots of laptops with an LED but also separate
> > buttons, indeed.
> > 
> > > > @@ -278,6 +280,9 @@ InitDeviceProperties(InputInfoPtr pInfo)
> > > >      values[2] = para->area_top_edge;
> > > >      values[3] = para->area_bottom_edge;
> > > >      prop_area = InitAtom(pInfo->dev, SYNAPTICS_PROP_AREA, 32, 4, values);
> > > > +
> > > > +    prop_led = InitAtom(local->dev, SYNAPTICS_PROP_LED, 8, 1, &para->has_led);
> > > > +    prop_led_status = InitAtom(local->dev, SYNAPTICS_PROP_LED_STATUS, 8, 1, &para->led_status);
> > > 
> > > the prop_led_status atom should only be initialized if the touchpad actually
> > > has a led, please make this conditional on the has_led bit.
> > 
> > So, you mean that prop_led should be NULL when no LED exists, thus
> > the querying this property would lead to an error?
> 
> yeah, the property prop_led_status simply won't be available and return a
> BadAtom to a client that tries to modify it. this is more explanatory than
> having the property but not doing anything with it if the device doesn't
> have a led.
> 
> fwiw, note that all prop_* are of type Atom, which is just an int. And since
> 0 (None) isn't a property, we don't need any extra checks for it in the
> driver, the server filters for us.

Alright.

While we are at it.  Any reason all these prop_* aren't static?


Takashi