Re: [PATCH] input: keyboard: introduce lm8323 driver

[Felipe Balbi <me@felipebalbi.com>]

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 <felipe.balbi@nokia.com>
> > 
> > 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 <dtor@mail.ru>
> ---
> 
>  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