Hi, On Sat, Mar 07, 2009 at 06:38:37PM -0800, Dmitry Torokhov wrote: > Hi felipe, > > On Thu, Feb 19, 2009 at 02:31:17PM +0200, Felipe Balbi wrote: > > From: Felipe Balbi <[email protected]> > > > > lm8323 is the keypad driver used in n810 device. This > > driver has been sitting in linux-omap for quite a long > > time, it's about time to get comments from upstream and > > get it merged. > > > > Thank you ofr the patch. I did not quite like the hard-coding of 3 PWMs, > I think an array works better and also you enable IRQ before allocating > input device, which is not safe. Also probe() leaks input device > structure in certain scenarios. I have a fixup patch and would > appreciate if you coould try it.
I'll try to test it on monday. Keeping the patch below and adding linux-omap to Cc as there might be other people interested in this patch. > Input: lm8323 fixups > > Signed-off-by: Dmitry Torokhov <[email protected]> > --- > > drivers/input/keyboard/lm8323.c | 236 > ++++++++++++++++----------------------- > include/linux/i2c/lm8323.h | 10 +- > 2 files changed, 100 insertions(+), 146 deletions(-) > > > diff --git a/drivers/input/keyboard/lm8323.c b/drivers/input/keyboard/lm8323.c > index a796680..9f4ef51 100644 > --- a/drivers/input/keyboard/lm8323.c > +++ b/drivers/input/keyboard/lm8323.c > @@ -137,6 +137,7 @@ struct lm8323_pwm { > struct mutex lock; > struct work_struct work; > struct led_classdev cdev; > + struct lm8323_chip *chip; > }; > > struct lm8323_chip { > @@ -154,9 +155,7 @@ struct lm8323_chip { > int size_y; > int debounce_time; > int active_time; > - struct lm8323_pwm pwm1; > - struct lm8323_pwm pwm2; > - struct lm8323_pwm pwm3; > + struct lm8323_pwm pwm[LM8323_NUM_PWMS]; > }; > > #define client_to_lm8323(c) container_of(c, struct lm8323_chip, client) > @@ -165,20 +164,6 @@ struct lm8323_chip { > #define cdev_to_pwm(c) container_of(c, struct lm8323_pwm, cdev) > #define work_to_pwm(w) container_of(w, struct lm8323_pwm, work) > > -static struct lm8323_chip *pwm_to_lm8323(struct lm8323_pwm *pwm) > -{ > - switch (pwm->id) { > - case 1: > - return container_of(pwm, struct lm8323_chip, pwm1); > - case 2: > - return container_of(pwm, struct lm8323_chip, pwm2); > - case 3: > - return container_of(pwm, struct lm8323_chip, pwm3); > - default: > - return NULL; > - } > -} > - > #define LM8323_MAX_DATA 8 > > /* > @@ -395,6 +380,7 @@ static void lm8323_work(struct work_struct *work) > { > struct lm8323_chip *lm = work_to_lm8323(work); > u8 ints; > + int i; > > mutex_lock(&lm->lock); > > @@ -414,17 +400,12 @@ static void lm8323_work(struct work_struct *work) > "reinitialising\n"); > lm8323_configure(lm); > } > - if (ints & INT_PWM1) { > - dev_vdbg(&lm->client->dev, "pwm1 engine completed\n"); > - pwm_done(&lm->pwm1); > - } > - if (ints & INT_PWM2) { > - dev_vdbg(&lm->client->dev, "pwm2 engine completed\n"); > - pwm_done(&lm->pwm2); > - } > - if (ints & INT_PWM3) { > - dev_vdbg(&lm->client->dev, "pwm3 engine completed\n"); > - pwm_done(&lm->pwm3); > + for (i = 0; i < LM8323_NUM_PWMS; i++) { > + if (ints & (1 << (INT_PWM1 + i))) { > + dev_vdbg(&lm->client->dev, > + "pwm%d engine completed\n", i); > + pwm_done(&lm->pwm[i]); > + } > } > } > > @@ -459,9 +440,7 @@ static int lm8323_read_id(struct lm8323_chip *lm, u8 *buf) > > static void lm8323_write_pwm_one(struct lm8323_pwm *pwm, int pos, u16 cmd) > { > - struct lm8323_chip *lm = pwm_to_lm8323(pwm); > - > - lm8323_write(lm, 4, LM8323_CMD_PWM_WRITE, (pos << 2) | pwm->id, > + lm8323_write(pwm->chip, 4, LM8323_CMD_PWM_WRITE, (pos << 2) | pwm->id, > (cmd & 0xff00) >> 8, cmd & 0x00ff); > } > > @@ -474,14 +453,13 @@ static void lm8323_write_pwm_one(struct lm8323_pwm > *pwm, int pos, u16 cmd) > static void lm8323_write_pwm(struct lm8323_pwm *pwm, int kill, > int len, const u16 *cmds) > { > - struct lm8323_chip *lm = pwm_to_lm8323(pwm); > int i; > > for (i = 0; i < len; i++) > lm8323_write_pwm_one(pwm, i, cmds[i]); > > lm8323_write_pwm_one(pwm, i++, PWM_END(kill)); > - lm8323_write(lm, 2, LM8323_CMD_START_PWM, pwm->id); > + lm8323_write(pwm->chip, 2, LM8323_CMD_START_PWM, pwm->id); > pwm->running = 1; > } > > @@ -546,7 +524,7 @@ static void lm8323_pwm_set_brightness(struct led_classdev > *led_cdev, > enum led_brightness brightness) > { > struct lm8323_pwm *pwm = cdev_to_pwm(led_cdev); > - struct lm8323_chip *lm = pwm_to_lm8323(pwm); > + struct lm8323_chip *lm = pwm->chip; > > mutex_lock(&pwm->lock); > pwm->desired_brightness = brightness; > @@ -582,7 +560,7 @@ static ssize_t lm8323_pwm_store_time(struct device *dev, > struct led_classdev *led_cdev = dev_get_drvdata(dev); > struct lm8323_pwm *pwm = cdev_to_pwm(led_cdev); > int ret; > - int time; > + unsigned long time; > > ret = strict_strtoul(buf, 10, &time); > /* Numbers only, please. */ > @@ -598,28 +576,22 @@ static DEVICE_ATTR(time, 0644, lm8323_pwm_show_time, > lm8323_pwm_store_time); > static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev, > const char *name) > { > - struct lm8323_pwm *pwm = NULL; > + struct lm8323_pwm *pwm; > > BUG_ON(id > 3); > > - switch (id) { > - case 1: > - pwm = &lm->pwm1; > - break; > - case 2: > - pwm = &lm->pwm2; > - break; > - case 3: > - pwm = &lm->pwm3; > - break; > - } > + pwm = &lm->pwm[id - 1]; > > pwm->id = id; > pwm->fade_time = 0; > pwm->brightness = 0; > pwm->desired_brightness = 0; > pwm->running = 0; > + pwm->enabled = 0; > + INIT_WORK(&pwm->work, lm8323_pwm_work); > mutex_init(&pwm->lock); > + pwm->chip = lm; > + > if (name) { > pwm->cdev.name = name; > pwm->cdev.brightness_set = lm8323_pwm_set_brightness; > @@ -628,15 +600,12 @@ static int init_pwm(struct lm8323_chip *lm, int id, > struct device *dev, > return -1; > } > if (device_create_file(pwm->cdev.dev, > - &dev_attr_time) < 0) { > + &dev_attr_time) < 0) { > dev_err(dev, "couldn't register time attribute\n"); > led_classdev_unregister(&pwm->cdev); > return -1; > } > - INIT_WORK(&pwm->work, lm8323_pwm_work); > pwm->enabled = 1; > - } else { > - pwm->enabled = 0; > } > > return 0; > @@ -658,7 +627,7 @@ static ssize_t lm8323_set_disable(struct device *dev, > { > struct lm8323_chip *lm = dev_get_drvdata(dev); > int ret; > - int i; > + unsigned long i; > > ret = strict_strtoul(buf, 10, &i); > > @@ -671,46 +640,50 @@ static ssize_t lm8323_set_disable(struct device *dev, > static DEVICE_ATTR(disable_kp, 0644, lm8323_show_disable, > lm8323_set_disable); > > static int lm8323_probe(struct i2c_client *client, > - const struct i2c_device_id *id) > + const struct i2c_device_id *id) > { > - struct lm8323_platform_data *pdata; > + struct lm8323_platform_data *pdata = client->dev.platform_data; > struct input_dev *idev; > struct lm8323_chip *lm; > - int i, err = 0; > + int i, err; > unsigned long tmo; > u8 data[2]; > > - lm = kzalloc(sizeof *lm, GFP_KERNEL); > - if (!lm) > - return -ENOMEM; > - > - i2c_set_clientdata(client, lm); > - lm->client = client; > - pdata = client->dev.platform_data; > if (!pdata || !pdata->size_x || !pdata->size_y) { > dev_err(&client->dev, "missing platform_data\n"); > - err = -EINVAL; > - goto fail2; > + return -EINVAL; > } > > - lm->size_x = pdata->size_x; > - if (lm->size_x > 8) { > + if (pdata->size_x > 8) { > dev_err(&client->dev, "invalid x size %d specified\n", > - lm->size_x); > - err = -EINVAL; > - goto fail2; > + pdata->size_x); > + return -EINVAL; > } > > - lm->size_y = pdata->size_y; > - if (lm->size_y > 12) { > + if (pdata->size_y > 12) { > dev_err(&client->dev, "invalid y size %d specified\n", > - lm->size_y); > - err = -EINVAL; > - goto fail2; > + pdata->size_y); > + return -EINVAL; > } > > + lm = kzalloc(sizeof *lm, GFP_KERNEL); > + idev = input_allocate_device(); > + if (!lm || !idev) { > + err = -ENOMEM; > + goto fail1; > + } > + > + i2c_set_clientdata(client, lm); > + > + lm->client = client; > + lm->idev = idev; > + mutex_init(&lm->lock); > + INIT_WORK(&lm->work, lm8323_work); > + > + lm->size_x = pdata->size_x; > + lm->size_y = pdata->size_y; > dev_vdbg(&client->dev, "Keypad size: %d x %d\n", > - lm->size_x, lm->size_y); > + lm->size_x, lm->size_y); > > lm->debounce_time = pdata->debounce_time; > lm->active_time = pdata->active_time; > @@ -726,61 +699,38 @@ static int lm8323_probe(struct i2c_client *client, > > if (time_after(jiffies, tmo)) { > dev_err(&client->dev, > - "timeout waiting for initialisation\n"); > + "timeout waiting for initialisation\n"); > break; > } > > msleep(1); > } > + > lm8323_configure(lm); > > /* If a true probe check the device */ > if (lm8323_read_id(lm, data) != 0) { > dev_err(&client->dev, "device not found\n"); > err = -ENODEV; > - goto fail2; > + goto fail1; > } > > - if (init_pwm(lm, 1, &client->dev, pdata->pwm1_name) < 0) > - goto fail3; > - if (init_pwm(lm, 2, &client->dev, pdata->pwm2_name) < 0) > - goto fail4; > - if (init_pwm(lm, 3, &client->dev, pdata->pwm3_name) < 0) > - goto fail5; > - > - mutex_init(&lm->lock); > - INIT_WORK(&lm->work, lm8323_work); > - > - err = request_irq(client->irq, lm8323_irq, > - IRQF_TRIGGER_FALLING | IRQF_DISABLED, > - "lm8323", lm); > - if (err) { > - dev_err(&client->dev, "could not get IRQ %d\n", client->irq); > - goto fail6; > + for (i = 0; i < LM8323_NUM_PWMS; i++) { > + err = init_pwm(lm, i + 1, &client->dev, pdata->pwm_names[i]); > + if (err < 0) > + goto fail2; > } > > - device_init_wakeup(&client->dev, 1); > - enable_irq_wake(client->irq); > - > lm->kp_enabled = 1; > err = device_create_file(&client->dev, &dev_attr_disable_kp); > if (err < 0) > - goto fail7; > - > - idev = input_allocate_device(); > - if (!idev) { > - err = -ENOMEM; > - goto fail8; > - } > + goto fail2; > > - if (pdata->name) > - idev->name = pdata->name; > - else > - idev->name = "LM8323 keypad"; > - snprintf(lm->phys, sizeof(lm->phys), "%s/input-kp", client->dev.bus_id); > + idev->name = pdata->name ? : "LM8323 keypad"; > + snprintf(lm->phys, sizeof(lm->phys), > + "%s/input-kp", dev_name(&client->dev)); > idev->phys = lm->phys; > > - lm->keys_down = 0; > idev->evbit[0] = BIT(EV_KEY); > for (i = 0; i < LM8323_KEYMAP_SIZE; i++) { > if (pdata->keymap[i] > 0) > @@ -792,30 +742,36 @@ static int lm8323_probe(struct i2c_client *client, > if (pdata->repeat) > __set_bit(EV_REP, idev->evbit); > > - lm->idev = idev; > err = input_register_device(idev); > if (err) { > dev_dbg(&client->dev, "error registering input device\n"); > - goto fail8; > + goto fail3; > } > > + err = request_irq(client->irq, lm8323_irq, > + IRQF_TRIGGER_FALLING | IRQF_DISABLED, > + "lm8323", lm); > + if (err) { > + dev_err(&client->dev, "could not get IRQ %d\n", client->irq); > + goto fail4; > + } > + > + device_init_wakeup(&client->dev, 1); > + enable_irq_wake(client->irq); > + > return 0; > > -fail8: > - device_remove_file(&client->dev, &dev_attr_disable_kp); > -fail7: > - free_irq(client->irq, lm); > -fail6: > - if (lm->pwm3.enabled) > - led_classdev_unregister(&lm->pwm3.cdev); > -fail5: > - if (lm->pwm2.enabled) > - led_classdev_unregister(&lm->pwm2.cdev); > fail4: > - if (lm->pwm1.enabled) > - led_classdev_unregister(&lm->pwm1.cdev); > + input_unregister_device(idev); > + idev = NULL; > fail3: > + device_remove_file(&client->dev, &dev_attr_disable_kp); > fail2: > + while (--i >= 0) > + if (lm->pwm[i].enabled) > + led_classdev_unregister(&lm->pwm[i].cdev); > +fail1: > + input_free_device(idev); > kfree(lm); > return err; > } > @@ -823,18 +779,20 @@ fail2: > static int lm8323_remove(struct i2c_client *client) > { > struct lm8323_chip *lm = i2c_get_clientdata(client); > + int i; > > disable_irq_wake(client->irq); > free_irq(client->irq, lm); > cancel_work_sync(&lm->work); > + > input_unregister_device(lm->idev); > + > device_remove_file(&lm->client->dev, &dev_attr_disable_kp); > - if (lm->pwm3.enabled) > - led_classdev_unregister(&lm->pwm3.cdev); > - if (lm->pwm2.enabled) > - led_classdev_unregister(&lm->pwm2.cdev); > - if (lm->pwm1.enabled) > - led_classdev_unregister(&lm->pwm1.cdev); > + > + for (i = 0; i < 3; i++) > + if (lm->pwm[i].enabled) > + led_classdev_unregister(&lm->pwm[i].cdev); > + > kfree(lm); > > return 0; > @@ -848,6 +806,7 @@ static int lm8323_remove(struct i2c_client *client) > static int lm8323_suspend(struct i2c_client *client, pm_message_t mesg) > { > struct lm8323_chip *lm = i2c_get_clientdata(client); > + int i; > > set_irq_wake(client->irq, 0); > disable_irq(client->irq); > @@ -856,12 +815,9 @@ static int lm8323_suspend(struct i2c_client *client, > pm_message_t mesg) > lm->pm_suspend = 1; > mutex_unlock(&lm->lock); > > - if (lm->pwm1.enabled) > - led_classdev_suspend(&lm->pwm1.cdev); > - if (lm->pwm2.enabled) > - led_classdev_suspend(&lm->pwm2.cdev); > - if (lm->pwm3.enabled) > - led_classdev_suspend(&lm->pwm3.cdev); > + for (i = 0; i < 3; i++) > + if (lm->pwm[i].enabled) > + led_classdev_suspend(&lm->pwm[i].cdev); > > return 0; > } > @@ -869,17 +825,15 @@ static int lm8323_suspend(struct i2c_client *client, > pm_message_t mesg) > static int lm8323_resume(struct i2c_client *client) > { > struct lm8323_chip *lm = i2c_get_clientdata(client); > + int i; > > mutex_lock(&lm->lock); > lm->pm_suspend = 0; > mutex_unlock(&lm->lock); > > - if (lm->pwm1.enabled) > - led_classdev_resume(&lm->pwm1.cdev); > - if (lm->pwm2.enabled) > - led_classdev_resume(&lm->pwm2.cdev); > - if (lm->pwm3.enabled) > - led_classdev_resume(&lm->pwm3.cdev); > + for (i = 0; i < 3; i++) > + if (lm->pwm[i].enabled) > + led_classdev_resume(&lm->pwm[i].cdev); > > enable_irq(client->irq); > set_irq_wake(client->irq, 1); > diff --git a/include/linux/i2c/lm8323.h b/include/linux/i2c/lm8323.h > index 67e82bc..50fad47 100644 > --- a/include/linux/i2c/lm8323.h > +++ b/include/linux/i2c/lm8323.h > @@ -25,7 +25,9 @@ > * so keys can be mapped directly at the index of the > * LM8323 keycode instead of subtracting one. > */ > -#define LM8323_KEYMAP_SIZE (0x7f + 1) > +#define LM8323_KEYMAP_SIZE (0x7f + 1) > + > +#define LM8323_NUM_PWMS 3 > > struct lm8323_platform_data { > int debounce_time; /* Time to watch for key bouncing, in ms. */ > @@ -36,11 +38,9 @@ struct lm8323_platform_data { > int repeat:1; > const s16 *keymap; > > - char *pwm1_name; /* Device name for PWM1. */ > - char *pwm2_name; /* Device name for PWM2. */ > - char *pwm3_name; /* Device name for PWM3. */ > + const char *pwm_names[LM8323_NUM_PWMS]; > > - char *name; /* Device name. */ > + const char *name; /* Device name. */ > }; > > #endif /* __LINUX_LM8323_H */ -- balbi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
