Re: [PATCH 2/2] input: qt2160: Add support for LEDs.

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Javier,

On Wed, Oct 17, 2012 at 03:09:52PM +0200, javier Martin wrote:
> On 17 October 2012 10:32, javier Martin <javier.martin@vista-silicon.com> wrote:
> > Hi Dmitry,
> > let me elaborate more on the LED issue.
> >
> > On 17 October 2012 09:23, javier Martin <javier.martin@vista-silicon.com> wrote:
> >> Hi Dmitry,
> >>
> >> On 16 October 2012 17:53, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >>> Hi Javier,
> >>>
> >>> On Tuesday, October 16, 2012 05:19:31 PM Javier Martin wrote:
> >>>> Outputs x8..x0 of the qt2160 can have leds attached to it.
> >>>> This patch handles those outputs using EV_LED events.
> >>>>
> >>>> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> >>>> ---
> >>>>  drivers/input/keyboard/qt2160.c |   38
> >>>> ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
> >>>>
> >>>> diff --git a/drivers/input/keyboard/qt2160.c
> >>>> b/drivers/input/keyboard/qt2160.c index 73ea4b0..7070372 100644
> >>>> --- a/drivers/input/keyboard/qt2160.c
> >>>> +++ b/drivers/input/keyboard/qt2160.c
> >>>> @@ -39,6 +39,7 @@
> >>>>  #define QT2160_CMD_GPIOS      6
> >>>>  #define QT2160_CMD_SUBVER     7
> >>>>  #define QT2160_CMD_CALIBRATE  10
> >>>> +#define QT2160_CMD_LEDS       70
> >>>>
> >>>>  #define QT2160_CYCLE_INTERVAL        (2*HZ)
> >>>>
> >>>> @@ -217,6 +218,30 @@ static int __devinit qt2160_write(struct i2c_client
> >>>> *client, u8 reg, u8 data) return ret;
> >>>>  }
> >>>>
> >>>> +static int qt2160_event(struct input_dev *dev,
> >>>> +                     unsigned int type, unsigned int code, int value)
> >>>> +{
> >>>> +     struct qt2160_data *qt2160 = input_get_drvdata(dev);
> >>>> +     struct i2c_client *client = qt2160->client;
> >>>> +     u32 val;
> >>>> +
> >>>> +     switch (type) {
> >>>> +     case EV_LED:
> >>>> +             val = qt2160_read(qt2160->client, QT2160_CMD_LEDS);
> >>>> +             if (value)
> >>>> +                     val |= (1 << code);
> >>>> +             else
> >>>> +                     val &= ~(1 << code);
> >>>
> >>> So qt2160 happens to use the same encoding as Linux and the leds
> >>> have the same purpose? Or maybe LED subsystem should be used to
> >>> register general-purpose leds?
> >>
> >> Yes, it uses the same encoding as Linux.
> >> The second question is more tricky and I expect you kindly give some
> >> guidelines about it.
> >> It is true that x8...x0 are just output pins in the qt2160, and
> >> anything could be attached to them. They could be handled as
> >> output-only GPIOs. Is it right to mix GPIO framework with input
> >> framework in this driver?
> >> On the other hand, with the current approach x8...x0 are fully functional too.
> >
> > There are three possible frameworks that could be used to implement
> > this functionality:
> > 1. Using EV_LED events in the input framework.
> > 2. Using LED subsystem and registering general-purpose leds.
> > 3. Using GPIO subsystem to register generic, output only, GPIOs.
> >
> > IMHO, option 3 could be discarded since it doesn't make much sense to
> > have output only GPIOs, the flexibility this framework provides
> > wouldn't be used at all.
> > So only options 1 and 2 would be left.
> >
> > I chose option 1 because this driver already uses the input framework
> > and I thought this functionality would be more easily integrated this
> > way.
> > As I understand, your concern with option 1 is related with the fact
> > that LED_NUML, LED_CAPSL, etc... don't have that meaning in this chip,
> > because we don't know what the user will be attaching to x7...x0
> > outputs.
> > But this argument could be used for the EV_KEY feature too. The
> > following suggested mapping in the same driver is just arbitrary,
> > isn't it?:
> >
> > static unsigned char qt2160_key2code[] = {
> >         KEY_0, KEY_1, KEY_2, KEY_3,
> >         KEY_4, KEY_5, KEY_6, KEY_7,
> >         KEY_8, KEY_9, KEY_A, KEY_B,
> >         KEY_C, KEY_D, KEY_E, KEY_F,
> > };
> >
> > Anyway I'm totally open to reimplement the feature using option 2, as
> > long as we agree that it's the right way to proceed. If this approach
> > were to be taken, should I add it to the same file or should I create
> > a separate driver inside /drivers/leds ?
> 
> Please, forgive my multiple e-mails.
> Another argument in favour of using the LED framework (2) is that
> qt2160 has the possibility to enable PWM in x7...x0. This way
> brightness can be controlled. If we use EV_LED, that feature cannot be
> controlled.
> 
> But, in order to provide LED framework support there are, AFAIK, two
> ways to proceed:
> a) Implement the functionality directly in /drivers/input/keyboard/qt2160.c
> b) Create a new multifunction device in /drivers/mfd/ and two
> subdevices: the existing one for keyboard and
> /drivers/leds/qt2160-leds.c for leds.
> 
> Option b) seems cleaner to me but LED control is such a simple
> functionality that I don't know if it's worth a separate file.
> 
> What do you think?

We have quite a few input drivers that either also use LED framework or
export unused gpio pins using gpiolib, so going full MFD route is not
necessary.

Thanks.

-- 
Dmitry