On Wed, Oct 24, 2012 at 11:24:12AM +0200, javier Martin wrote:
> Hi Dmitry,
> thank you for your review.
>
> On 22 October 2012 18:17, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
> > Hi Javier,
> >
> > On Mon, Oct 22, 2012 at 10:04:12AM +0200, Javier Martin wrote:
> >> Outputs x8..x0 of the qt2160 can have leds attached to it.
> >> This patch handles those outputs using the generic LED
> >> framework.
> >>
> >> The PWM controls available in the chip are used to achieve
> >> different levels of brightness.
> >>
> >> Signed-off-by: Javier Martin <javier.martin@xxxxxxxxxxxxxxxxx>
> >> ---
> >> Changes since v3:
> >> - Protect led related code with #ifdefs so that users can decide
> >> wether to enable LEDS_CLASS support in the kernel or not.
> >>
> >> ---
> >> drivers/input/keyboard/qt2160.c | 113 ++++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 111 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/input/keyboard/qt2160.c b/drivers/input/keyboard/qt2160.c
> >> index 73ea4b0..482a7c5 100644
> >> --- a/drivers/input/keyboard/qt2160.c
> >> +++ b/drivers/input/keyboard/qt2160.c
> >> @@ -20,6 +20,7 @@
> >>
> >> #include <linux/kernel.h>
> >> #include <linux/init.h>
> >> +#include <linux/leds.h>
> >> #include <linux/module.h>
> >> #include <linux/slab.h>
> >> #include <linux/jiffies.h>
> >> @@ -39,6 +40,11 @@
> >> #define QT2160_CMD_GPIOS 6
> >> #define QT2160_CMD_SUBVER 7
> >> #define QT2160_CMD_CALIBRATE 10
> >> +#define QT2160_CMD_DRIVE_X 70
> >> +#define QT2160_CMD_PWMEN_X 74
> >> +#define QT2160_CMD_PWM_DUTY 76
> >> +
> >> +#define QT2160_NUM_LEDS_X 8
> >>
> >> #define QT2160_CYCLE_INTERVAL (2*HZ)
> >>
> >> @@ -49,6 +55,17 @@ static unsigned char qt2160_key2code[] = {
> >> KEY_C, KEY_D, KEY_E, KEY_F,
> >> };
> >>
> >> +#ifdef CONFIG_LEDS_CLASS
> >> +struct qt2160_led {
> >> + struct qt2160_data *qt2160;
> >> + struct led_classdev cdev;
> >> + struct work_struct work;
> >> + char name[32];
> >> + int id;
> >> + enum led_brightness new_brightness;
> >> +};
> >> +#endif
> >> +
> >> struct qt2160_data {
> >> struct i2c_client *client;
> >> struct input_dev *input;
> >> @@ -56,8 +73,61 @@ struct qt2160_data {
> >> spinlock_t lock; /* Protects canceling/rescheduling of dwork */
> >> unsigned short keycodes[ARRAY_SIZE(qt2160_key2code)];
> >> u16 key_matrix;
> >> +#ifdef CONFIG_LEDS_CLASS
> >> + struct qt2160_led leds[QT2160_NUM_LEDS_X];
> >> + struct mutex led_lock;
> >> +#endif
> >> };
> >>
> >> +static int qt2160_read(struct i2c_client *client, u8 reg);
> >> +static int qt2160_write(struct i2c_client *client, u8 reg, u8 data);
> >> +
> >> +#ifdef CONFIG_LEDS_CLASS
> >> +
> >> +static void qt2160_led_work(struct work_struct *work)
> >> +{
> >> + struct qt2160_led *led = container_of(work, struct qt2160_led, work);
> >> + struct qt2160_data *qt2160 = led->qt2160;
> >> + struct i2c_client *client = qt2160->client;
> >> + int value = led->new_brightness;
> >> + u32 drive, pwmen;
> >> +
> >> + mutex_lock(&qt2160->led_lock);
> >
> > What about accessing I2C from other contexts? Don't we need to take this
> > lock there too?
>
> The purpose of this mutex is to avoid races between multiple calls to
> this function which is the only one that access (read/modify/write)
> CMD_DRIVE_X, CMD_PWMEN_X, CMD_PWM_DUTY registers.
> Please, correct me if I am wrong but I don't think we have to take
> this mutex anywhere else.
Ah, OK then.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
