[PATCH 1/2] input: qt2160: Fix I2C write function.

[PATCH 1/2] input: qt2160: Fix I2C write function.

[Javier Martin]

The previous implementation of qt2160_write() didn't work
properly. Use a similar aproach as qt1070 instead.

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/input/keyboard/qt2160.c |   19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/input/keyboard/qt2160.c b/drivers/input/keyboard/qt2160.c
index e7a5e36..73ea4b0 100644
--- a/drivers/input/keyboard/qt2160.c
+++ b/drivers/input/keyboard/qt2160.c
@@ -207,23 +207,14 @@ static int __devinit qt2160_read(struct i2c_client *client, u8 reg)
 
 static int __devinit qt2160_write(struct i2c_client *client, u8 reg, u8 data)
 {
-	int error;
-
-	error = i2c_smbus_write_byte(client, reg);
-	if (error) {
-		dev_err(&client->dev,
-			"couldn't send request. Returned %d\n", error);
-		return error;
-	}
+	int ret;
 
-	error = i2c_smbus_write_byte(client, data);
-	if (error) {
+	ret = i2c_smbus_write_byte_data(client, reg, data);
+	if (ret < 0)
 		dev_err(&client->dev,
-			"couldn't write data. Returned %d\n", error);
-		return error;
-	}
+			"couldn't write data. Returned %d\n", ret);
 
-	return error;
+	return ret;
 }
 
 
-- 
1.7.9.5

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

[Javier Martin]

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);
+		qt2160_write(qt2160->client, QT2160_CMD_LEDS, val);
+		break;
+	default:
+		dev_err(&client->dev, "%s: Got type %d, code %d, value %d\n",
+		       __func__, type, code, value);
+		return -1;
+	}
+
+	return 0;
+}
 
 static bool __devinit qt2160_identify(struct i2c_client *client)
 {
@@ -290,8 +315,21 @@ static int __devinit qt2160_probe(struct i2c_client *client,
 	input->keycodesize = sizeof(qt2160->keycodes[0]);
 	input->keycodemax = ARRAY_SIZE(qt2160_key2code);
 
+	input->event = qt2160_event;
+
+	input_set_drvdata(input, qt2160);
+
 	__set_bit(EV_KEY, input->evbit);
 	__clear_bit(EV_REP, input->evbit);
+	__set_bit(EV_LED, input->evbit);
+	__set_bit(LED_NUML, input->ledbit);
+	__set_bit(LED_CAPSL, input->ledbit);
+	__set_bit(LED_SCROLLL, input->ledbit);
+	__set_bit(LED_COMPOSE, input->ledbit);
+	__set_bit(LED_KANA, input->ledbit);
+	__set_bit(LED_SLEEP, input->ledbit);
+	__set_bit(LED_SUSPEND, input->ledbit);
+	__set_bit(LED_MUTE, input->ledbit);
 	for (i = 0; i < ARRAY_SIZE(qt2160_key2code); i++) {
 		qt2160->keycodes[i] = qt2160_key2code[i];
 		__set_bit(qt2160_key2code[i], input->keybit);
-- 
1.7.9.5

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

[Dmitry Torokhov]

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?

> +		qt2160_write(qt2160->client, QT2160_CMD_LEDS, val);

I do not think this will work as qt2160_event() runs under a spinlock
with interrupts off, and qt2160_read() and qt2160_write() do I2C IO
and thus may sleep.

Also qt2160_write is marked __devinit and so may not be available to
qt2160_event.

How was this tested?

Thanks.

-- 
Dmitry

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

[javier Martin]

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.

>> +             qt2160_write(qt2160->client, QT2160_CMD_LEDS, val);
>
> I do not think this will work as qt2160_event() runs under a spinlock
> with interrupts off, and qt2160_read() and qt2160_write() do I2C IO
> and thus may sleep.

Sorry, I hadn't notice that the upper layers used a spinlock for this.
This have worked in our tests but it is obviously not safe. I should
use a tasklet or similar to issue the qt2160_write() call.

> Also qt2160_write is marked __devinit and so may not be available to
> qt2160_event.
>
> How was this tested?

Well, it was tested and works properly in our platform but it seems a
good idea to remove __devinit for those functions now that they will
be used outside the init functions.

Regards.
-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

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

[javier Martin]

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 ?

Regards.
-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

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

[javier Martin]

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?

Regards.
-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

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

[Dmitry Torokhov]

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

Re: [PATCH 1/2] input: qt2160: Fix I2C write function.

[Dmitry Torokhov]

On Tue, Oct 16, 2012 at 05:19:30PM +0200, Javier Martin wrote:
> The previous implementation of qt2160_write() didn't work
> properly. Use a similar aproach as qt1070 instead.

I think your editor lost the end of the sentence: "... qt2160_write()
didn't work properly because ..." :)

Thanks!

-- 
Dmitry

Re: [PATCH 1/2] input: qt2160: Fix I2C write function.

[javier Martin]

Hi Dmitry,
thank you for your answer.

On 16 October 2012 19:31, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Tue, Oct 16, 2012 at 05:19:30PM +0200, Javier Martin wrote:
>> The previous implementation of qt2160_write() didn't work
>> properly. Use a similar aproach as qt1070 instead.
>
> I think your editor lost the end of the sentence: "... qt2160_write()
> didn't work properly because ..." :)
>
> Thanks!
>
> --
> Dmitry

Because the requested value was actually not written to the device.
Probably nobody detected this because the only write that was issued
was the one related to auto calibration.

It was while I was trying to add support for the LEDs when I realized
that they were not being powered on.

Do you want me to send a v2 patch with these information added to the changelog?

Regards.
-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com