[RFC,v1,3/6] led: pmic8058: Add PMIC8058 leds driver

Add support for Qualcomm PMIC8058 keyboard
backlight, flash and low current leds.

Cc: Richard Purdie <rpurdie@rpsys.net>
Signed-off-by: Trilok Soni <tsoni@codeaurora.org>
---
 drivers/leds/Kconfig          |   11 +
 drivers/leds/Makefile         |    1 +
 drivers/leds/leds-pmic8058.c  |  405 +++++++++++++++++++++++++++++++++++++++++
 include/linux/leds-pmic8058.h |   63 +++++++
 4 files changed, 480 insertions(+), 0 deletions(-)
 create mode 100644 drivers/leds/leds-pmic8058.c
 create mode 100644 include/linux/leds-pmic8058.h

Trilok Soni wrote:
> Add support for Qualcomm PMIC8058 keyboard
> backlight, flash and low current leds.
>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> Signed-off-by: Trilok Soni <tsoni@codeaurora.org>
> ---
>  drivers/leds/Kconfig          |   11 +
>  drivers/leds/Makefile         |    1 +
>  drivers/leds/leds-pmic8058.c  |  405 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/leds-pmic8058.h |   63 +++++++
>  4 files changed, 480 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/leds/leds-pmic8058.c
>  create mode 100644 include/linux/leds-pmic8058.h
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index cc2a88d..e1ebcad 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -214,6 +214,17 @@ config LEDS_PCA955X
>  	  LED driver chips accessed via the I2C bus.  Supported
>  	  devices include PCA9550, PCA9551, PCA9552, and PCA9553.
>
> +config LEDS_PMIC8058
> +	tristate "LED Support for Qualcomm PMIC8058"
> +	depends on PMIC8058
> +	help
> +	  This option enables support for LEDs connected over PMIC8058
> +	  (Power Management IC) chip on Qualcomm reference boards,
> +	  for example SURF and FFAs.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called leds-pmic8058.
> +
>  config LEDS_WM831X_STATUS
>  	tristate "LED support for status LEDs on WM831x PMICs"
>  	depends on MFD_WM831X
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 9c96db4..6c51883 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_LEDS_COBALT_QUBE)		+= leds-cobalt-qube.o
>  obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
>  obj-$(CONFIG_LEDS_SUNFIRE)		+= leds-sunfire.o
>  obj-$(CONFIG_LEDS_PCA9532)		+= leds-pca9532.o
> +obj-$(CONFIG_LEDS_PMIC8058)		+= leds-pmic8058.o
>  obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
>  obj-$(CONFIG_LEDS_LP3944)		+= leds-lp3944.o
>  obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
> diff --git a/drivers/leds/leds-pmic8058.c b/drivers/leds/leds-pmic8058.c
> new file mode 100644
> index 0000000..2933eb0
> --- /dev/null
> +++ b/drivers/leds/leds-pmic8058.c
> @@ -0,0 +1,405 @@
> +/* Copyright (c) 2010, Code Aurora Forum. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> + * 02110-1301, USA.
> + */
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/leds.h>
> +#include <linux/workqueue.h>
> +#include <linux/spinlock.h>
> +#include <linux/mfd/pmic8058.h>
> +#include <linux/leds-pmic8058.h>
> +
> +#define SSBI_REG_ADDR_DRV_KEYPAD	0x48
> +#define PM8058_DRV_KEYPAD_BL_MASK	0xf0
> +#define PM8058_DRV_KEYPAD_BL_SHIFT	0x04
> +
> +#define SSBI_REG_ADDR_FLASH_DRV0        0x49
> +#define PM8058_DRV_FLASH_MASK           0xf0
> +#define PM8058_DRV_FLASH_SHIFT          0x04
> +
> +#define SSBI_REG_ADDR_FLASH_DRV1        0xFB
> +
> +#define SSBI_REG_ADDR_LED_CTRL_BASE	0x131
> +#define SSBI_REG_ADDR_LED_CTRL(n)	(SSBI_REG_ADDR_LED_CTRL_BASE + (n))
> +#define PM8058_DRV_LED_CTRL_MASK	0xf8
> +#define PM8058_DRV_LED_CTRL_SHIFT	0x03
> +
> +#define MAX_FLASH_LED_CURRENT	300
> +#define MAX_LC_LED_CURRENT	40
> +#define MAX_KP_BL_LED_CURRENT	300
> +
> +#define MAX_KEYPAD_BL_LEVEL	(1 << 4)
> +#define MAX_LED_DRV_LEVEL	20 /* 2 * 20 mA */
> +
> +#define PMIC8058_LED_OFFSET(id) ((id) - PMIC8058_ID_LED_0)
> +
> +#define PMIC8058_MAX_LEDS	7
> +
> +/**
> + * struct pmic8058_led_data - internal led data structure
> + * @led_classdev - led class device
> + * @id	- led index
> + * @led_brightness - led brightness levels
> + * @pm_chip - parent MFD core device
> + * @work - workqueue for led
> + * @lock - to protect the transactions
> + * @value_lock - to protect the register value writes
> + * @reg_kp - cached value of keypad led backlight register
> + * @reg_led_ctrl - cached values of low-current led registers
> + * @reg_flash_led0 - cached value of first flash led control register
> + * @reg_flash_led1 - cached value of second flash led control register
> + */
> +struct pmic8058_led_data {
> +	struct led_classdev	cdev;
> +	int			id;
"enum pmic8058_leds" instead of int
> +	enum led_brightness	brightness;
> +	struct pm8058_chip	*pm_chip;
> +	struct work_struct	work;
> +	struct mutex		lock;
> +	spinlock_t		value_lock;
> +	u8			reg_kp;
> +	u8			reg_led_ctrl[3];
> +	u8			reg_flash_led0;
> +	u8			reg_flash_led1;

You allocate a separate pmic8058_led_data for each led, so one "u8 reg" should be
sufficient.

> +};
> +
> +#define PM8058_MAX_LEDS		7
> +
> +static void led_kp_set(struct pmic8058_led_data *led, enum led_brightness value)
> +{
> +	int rc;
> +	u8 level;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&led->value_lock, flags);
This function is only ever called from within the workqueue so there is no need for
locking.

> +	level = (value << PM8058_DRV_KEYPAD_BL_SHIFT) &
> +				 PM8058_DRV_KEYPAD_BL_MASK;
> +
> +	led->reg_kp &= ~PM8058_DRV_KEYPAD_BL_MASK;
> +	led->reg_kp |= level;
> +	spin_unlock_irqrestore(&led->value_lock, flags);
> +
> +	rc = pm8058_write(led->pm_chip, SSBI_REG_ADDR_DRV_KEYPAD,
> +				 &led->reg_kp, 1);
> +	if (rc < 0)
> +		dev_err(led->cdev.dev, "can't set keypad backlight level\n");
> +}
> +
> +static enum led_brightness led_kp_get(struct pmic8058_led_data *led)
> +{
> +	if ((led->reg_kp & PM8058_DRV_KEYPAD_BL_MASK) >>
> +			 PM8058_DRV_KEYPAD_BL_SHIFT)
> +		return LED_FULL;
> +	else
> +		return LED_OFF;
> +}
> +

Shouldn't you be returning the actual brightness here instead of only either on or
off? The brightness is btw. stored in led->brightness, so you can use the same getter
for all three types of leds.


> +static void led_lc_set(struct pmic8058_led_data *led, enum led_brightness value)
> +{
> +	unsigned long flags;
> +	int rc, offset;
> +	u8 level, tmp;
> +
> +	spin_lock_irqsave(&led->value_lock, flags);
This function is only ever called from within the workqueue so there is no need for
locking.

> +
> +	level = (led->brightness << PM8058_DRV_LED_CTRL_SHIFT) &
> +		PM8058_DRV_LED_CTRL_MASK;
> +
> +	offset = PMIC8058_LED_OFFSET(led->id);
> +	tmp = led->reg_led_ctrl[offset];
> +
> +	tmp &= ~PM8058_DRV_LED_CTRL_MASK;
> +	tmp |= level;
> +	spin_unlock_irqrestore(&led->value_lock, flags);
> +
> +	rc = pm8058_write(led->pm_chip,	SSBI_REG_ADDR_LED_CTRL(offset),
> +			&tmp, 1);
> +	if (rc) {
> +		dev_err(led->cdev.dev, "can't set (%d) led value\n",
> +				led->id);
> +		return;
> +	}
> +
> +	spin_lock_irqsave(&led->value_lock, flags);
> +	led->reg_led_ctrl[offset] = tmp;
> +	spin_unlock_irqrestore(&led->value_lock, flags);
> +}
> +
> +static enum led_brightness led_lc_get(struct pmic8058_led_data *led)
> +{
> +	int offset;
> +	u8 value;
> +
> +	offset = PMIC8058_LED_OFFSET(led->id);
> +	value = led->reg_led_ctrl[offset];
> +
> +	if ((value & PM8058_DRV_LED_CTRL_MASK) >>
> +			PM8058_DRV_LED_CTRL_SHIFT)
> +		return LED_FULL;
> +	else
> +		return LED_OFF;
> +}
See above.
> +
> +static void
> +led_flash_set(struct pmic8058_led_data *led, enum led_brightness value)
> +{
> +	int rc;
> +	u8 level;
> +	unsigned long flags;
> +	u8 reg_flash_led;
> +	u16 reg_addr;
> +
> +	spin_lock_irqsave(&led->value_lock, flags);
This function is only ever called from within the workqueue so there is no need for
locking.
> +	level = (value << PM8058_DRV_FLASH_SHIFT) &
> +				 PM8058_DRV_FLASH_MASK;
> +
> +	if (led->id == PMIC8058_ID_FLASH_LED_0) {
> +		led->reg_flash_led0 &= ~PM8058_DRV_FLASH_MASK;
> +		led->reg_flash_led0 |= level;
> +		reg_flash_led	    = led->reg_flash_led0;
> +		reg_addr	    = SSBI_REG_ADDR_FLASH_DRV0;
> +	} else {
> +		led->reg_flash_led1 &= ~PM8058_DRV_FLASH_MASK;
> +		led->reg_flash_led1 |= level;
> +		reg_flash_led	    = led->reg_flash_led1;
> +		reg_addr	    = SSBI_REG_ADDR_FLASH_DRV1;
> +	}
> +	spin_unlock_irqrestore(&led->value_lock, flags);
> +
> +	rc = pm8058_write(led->pm_chip, reg_addr, &reg_flash_led, 1);
> +	if (rc < 0)
> +		dev_err(led->cdev.dev, "can't set flash led%d level\n",
> +			 led->id);
> +}
> +
> +static void pmic8058_led_work(struct work_struct *work)
> +{
> +	struct pmic8058_led_data *led = container_of(work,
> +					 struct pmic8058_led_data, work);
> +
> +	mutex_lock(&led->lock);
> +
Since this is a workqueue and there will only one running instance per led at a time
there is no need to take a lock here.
> +	switch (led->id) {
> +	case PMIC8058_ID_LED_KB_LIGHT:
> +		led_kp_set(led, led->brightness);
> +		break;
> +	case PMIC8058_ID_LED_0:
> +	case PMIC8058_ID_LED_1:
> +	case PMIC8058_ID_LED_2:
> +		led_lc_set(led, led->brightness);
> +		break;
> +	case PMIC8058_ID_FLASH_LED_0:
> +	case PMIC8058_ID_FLASH_LED_1:
> +		led_flash_set(led, led->brightness);
> +		break;
> +	}
> +
> +	mutex_unlock(&led->lock);
> +}
> +
> +static void pmic8058_led_set(struct led_classdev *led_cdev,
> +	enum led_brightness value)
> +{
> +	struct pmic8058_led_data *led;
> +	unsigned long flags;
> +
> +	led = container_of(led_cdev, struct pmic8058_led_data, cdev);
> +
> +	spin_lock_irqsave(&led->value_lock, flags);
Locking is not really required here since it is only a single assignment...
> +	led->brightness = value;
> +	schedule_work(&led->work);
and scheudule_work does not have to be inside of the lock.
> +	spin_unlock_irqrestore(&led->value_lock, flags);
> +}
> +
> +static enum led_brightness pmic8058_led_get(struct led_classdev *led_cdev)
> +{
> +	struct pmic8058_led_data *led;
> +
> +	led = container_of(led_cdev, struct pmic8058_led_data, cdev);
return led->brightness; (See above)
> +
> +	switch (led->id) {
> +	case PMIC8058_ID_LED_KB_LIGHT:
> +		return led_kp_get(led);
> +	case PMIC8058_ID_LED_0:
> +	case PMIC8058_ID_LED_1:
> +	case PMIC8058_ID_LED_2:
> +		return led_lc_get(led);
> +	}
> +	return LED_OFF;
> +}
> +
> +static int pmic8058_led_probe(struct platform_device *pdev)
__devinit
> +{
> +	struct pmic8058_leds_platform_data *pdata = pdev->dev.platform_data;
> +	struct pmic8058_led_data *led_dat;
> +	struct pmic8058_led *curr_led;
> +	int rc, i = 0;
> +	struct pm8058_chip	*pm_chip;
> +	u8			reg_kp;
> +	u8			reg_led_ctrl[3];
> +	u8			reg_flash_led0;
> +	u8			reg_flash_led1;
> +	static struct pmic8058_led_data *led;
> +
> +	pm_chip = platform_get_drvdata(pdev);
This looks at least a bit bogus since you'll overwrite the drvdata later. Can't you
get the pm8058_chip through pdev->dev.parent somehow?
> +	if (pm_chip == NULL) {
> +		dev_err(&pdev->dev, "no parent data passed in\n");
> +		return -EFAULT;
-EINVAL
> +	}
> +
> +	if (pdata == NULL) {
> +		dev_err(&pdev->dev, "platform data not supplied\n");
> +		return -EINVAL;
> +	}
> +
> +	if (pdata->num_leds > PMIC8058_MAX_LEDS) {
> +		dev_err(&pdev->dev, "can't handle more than %d LEDS\n",
> +				 PMIC8058_MAX_LEDS);
> +		return -EFAULT;
-EINVAL
> +	}
> +
> +	led = kzalloc(sizeof(*led) * pdata->num_leds, GFP_KERNEL);
Use kcalloc instead of kzalloc.

> +	if (led == NULL) {
> +		dev_err(&pdev->dev, "failed to alloc memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	rc = pm8058_read(pm_chip, SSBI_REG_ADDR_DRV_KEYPAD, &reg_kp,
> +				1);
> +	if (rc) {
> +		dev_err(&pdev->dev, "can't get keypad backlight level\n");
> +		goto err_reg_read;
> +	}
> +
> +	rc = pm8058_read(pm_chip, SSBI_REG_ADDR_LED_CTRL_BASE,
> +			reg_led_ctrl, 3);
> +	if (rc) {
> +		dev_err(&pdev->dev, "can't get led levels\n");
> +		goto err_reg_read;
> +	}
> +
> +	rc = pm8058_read(pm_chip, SSBI_REG_ADDR_FLASH_DRV0,
> +			&reg_flash_led0, 1);
> +	if (rc) {
> +		dev_err(&pdev->dev, "can't read flash led0\n");
> +		goto err_reg_read;
> +	}
> +
> +	rc = pm8058_read(pm_chip, SSBI_REG_ADDR_FLASH_DRV1,
> +			&reg_flash_led1, 1);
> +	if (rc) {
> +		dev_err(&pdev->dev, "can't get flash led1\n");
> +		goto err_reg_read;
> +	}
How about adding a helper function which will initializes the leds 'reg' field
depending on its id? It will certainly to improve code readability.

> +
> +	for (i = 0; i < pdata->num_leds; i++) {
> +		curr_led	= &pdata->leds[i];
> +		led_dat		= &led[curr_led->id];
> +
> +		led_dat->cdev.name		= curr_led->name;
> +		led_dat->cdev.default_trigger   = curr_led->default_trigger;
> +		led_dat->cdev.brightness_set    = pmic8058_led_set;
> +		led_dat->cdev.brightness_get    = pmic8058_led_get;
> +		led_dat->cdev.brightness	= LED_OFF;
> +		led_dat->cdev.max_brightness	= curr_led->max_brightness;
> +		led_dat->cdev.flags		= LED_CORE_SUSPENDRESUME;
> +
> +		led_dat->id		        = curr_led->id;
> +		led_dat->reg_kp			= reg_kp;
> +		memcpy(led->reg_led_ctrl, reg_led_ctrl,
> +					 sizeof(reg_led_ctrl));
> +		led_dat->reg_flash_led0		= reg_flash_led0;
> +		led_dat->reg_flash_led1		= reg_flash_led1;
> +
> +		if (!((led_dat->id >= PMIC8058_ID_LED_KB_LIGHT) &&
> +				(led_dat->id <= PMIC8058_ID_FLASH_LED_1))) {
> +			dev_err(&pdev->dev, "invalid LED ID (%d) specified\n",
> +						 led_dat->id);
> +			rc = -EINVAL;
> +			goto fail_id_check;
> +		}
> +
> +		led_dat->pm_chip		= pm_chip;
> +
> +		mutex_init(&led_dat->lock);
> +		spin_lock_init(&led_dat->value_lock);
> +		INIT_WORK(&led_dat->work, pmic8058_led_work);
> +
> +		rc = led_classdev_register(&pdev->dev, &led_dat->cdev);
> +		if (rc) {
> +			dev_err(&pdev->dev, "unable to register led %d\n",
> +						 led_dat->id);
> +			goto fail_id_check;
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, led);
> +
> +	return 0;
> +
> +err_reg_read:
> +	kfree(led);
> +fail_id_check:
> +	if (i > 0) {
> +		for (i = i - 1; i >= 0; i--)
> +			led_classdev_unregister(&led[i].cdev);
> +	}
> +	return rc;
> +}
> +
> +static int __devexit pmic8058_led_remove(struct platform_device *pdev)
> +{
> +	int i;
> +	struct pmic8058_leds_platform_data *pdata = pdev->dev.platform_data;
> +	struct pmic8058_led_data *led = platform_get_drvdata(pdev);
> +
> +	for (i = 0; i < pdata->num_leds; i++) {
> +		mutex_destroy(&led[led->id].lock);
> +		led_classdev_unregister(&led[led->id].cdev);
> +		cancel_work_sync(&led[led->id].work);
> +	}

You need to free the 'led' array.

> +
> +	return 0;
> +}
> +
> +static struct platform_driver pmic8058_led_driver = {
> +	.probe		= pmic8058_led_probe,
> +	.remove		= __devexit_p(pmic8058_led_remove),
> +	.driver		= {
> +		.name	= "pm8058-led",
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +static int __init pmic8058_led_init(void)
> +{
> +	return platform_driver_register(&pmic8058_led_driver);
> +}
> +module_init(pmic8058_led_init);
> +
> +static void __exit pmic8058_led_exit(void)
> +{
> +	platform_driver_unregister(&pmic8058_led_driver);
> +}
> +module_exit(pmic8058_led_exit);
> +
> +MODULE_DESCRIPTION("PMIC8058 LEDs driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION("1.0");
> +MODULE_ALIAS("platform:pmic8058-led");
> +MODULE_AUTHOR("Trilok Soni <tsoni@codeaurora.org>");
> diff --git a/include/linux/leds-pmic8058.h b/include/linux/leds-pmic8058.h
> new file mode 100644
> index 0000000..c54c7e6
> --- /dev/null
> +++ b/include/linux/leds-pmic8058.h
> @@ -0,0 +1,63 @@
> +/* Copyright (c) 2010, Code Aurora Forum. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> + * 02110-1301, USA.
> + */
> +
> +#ifndef __LEDS_PMIC8058_H__
> +#define __LEDS_PMIC8058_H__
> +
> +/**
> + * enum pmic8058_leds - PMIC8058 supported led ids
> + * @PMIC8058_ID_LED_KB_LIGHT - keyboard backlight led
> + * @PMIC8058_ID_LED_0 - First low current led
> + * @PMIC8058_ID_LED_1 - Second low current led
> + * @PMIC8058_ID_LED_2 - Third low current led
> + * @PMIC8058_ID_FLASH_LED_0 - First flash led
> + * @PMIC8058_ID_FLASH_LED_0 - Second flash led
> + */
> +enum pmic8058_leds {
> +	PMIC8058_ID_LED_KB_LIGHT = 1,
> +	PMIC8058_ID_LED_0,
> +	PMIC8058_ID_LED_1,
> +	PMIC8058_ID_LED_2,
> +	PMIC8058_ID_FLASH_LED_0,
> +	PMIC8058_ID_FLASH_LED_1,
> +};
> +
> +/**
> + * struct pmic8058_led - per led data
> + * @name - name of the led
> + * @default_trigger - default trigger which needs to e attached
> + * @max_brightness - maximum brightness level supported by the led
> + * @id - supported led id
> + */
> +struct pmic8058_led {
> +	const char	*name;
> +	const char	*default_trigger;
> +	unsigned	max_brightness;
Should max_brightness not rather be hardcoded in the driver? As far as I can tell it
depend on the hardware and is 4 bits wide for flash and bl leds and 5 bits for the
others.
> +	int		id;

enum pmic8058_leds instead of int



> +};
> +
> +/**
> + * struct pmic8058_leds_platform_data - platform data for leds
> + * @num_leds - number of leds
> + * @leds - array of struct pmic8058_led
> + */
> +struct pmic8058_leds_platform_data {
> +	int			num_leds;
size_t
> +	struct pmic8058_led	*leds;
> +};


If max_brightness is hardcoded in the driver you can reuse "struct led_info" and
"struct struct led_platform_data" instead of adding your own structs.


> +
> +#endif /* __LEDS_PMIC8058_H__ */

On Wed, Nov 10, 2010 at 09:45:05PM +0100, Lars-Peter Clausen wrote:
> Trilok Soni wrote:
> > +
> > +static void led_kp_set(struct pmic8058_led_data *led, enum led_brightness value)
> > +{
> > +	int rc;
> > +	u8 level;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&led->value_lock, flags);
> This function is only ever called from within the workqueue so there is no need for
> locking.
> 

That is a common misconception, unfortunately. The same work may
be executing on several CPUs at the same time if it was scheduled on
multi-threaded work queue.

...

> > +	schedule_work(&led->work);

And sure enough, keventd is such workqueue.

Now, whether having the same work run simultaneously is OK or not is a
different question altogether...

Dmitry Torokhov wrote:
> On Wed, Nov 10, 2010 at 09:45:05PM +0100, Lars-Peter Clausen wrote:
>> Trilok Soni wrote:
>>> +
>>> +static void led_kp_set(struct pmic8058_led_data *led, enum led_brightness value)
>>> +{
>>> +	int rc;
>>> +	u8 level;
>>> +	unsigned long flags;
>>> +
>>> +	spin_lock_irqsave(&led->value_lock, flags);
>> This function is only ever called from within the workqueue so there is no need for
>> locking.
>>
> 
> That is a common misconception, unfortunately. The same work may
> be executing on several CPUs at the same time if it was scheduled on
> multi-threaded work queue.
> 

Hm, right my fault.
Still the comment above is still valid, because the original workqueue handler was
locked by a mutex. But the comment regarding the mutex should have been:
 You can remove the mutex if you queue the work on the system_nrt_wq workqueue

> ...
> 
>>> +	schedule_work(&led->work);
> 
> And sure enough, keventd is such workqueue.
> 
> Now, whether having the same work run simultaneously is OK or not is a
> different question altogether...
> 

- Lars

Hi Peter,

>> +struct pmic8058_led_data {
>> +	struct led_classdev	cdev;
>> +	int			id;
> "enum pmic8058_leds" instead of int

Ack.

>> +	enum led_brightness	brightness;
>> +	struct pm8058_chip	*pm_chip;
>> +	struct work_struct	work;
>> +	struct mutex		lock;
>> +	spinlock_t		value_lock;
>> +	u8			reg_kp;
>> +	u8			reg_led_ctrl[3];
>> +	u8			reg_flash_led0;
>> +	u8			reg_flash_led1;
> 
> You allocate a separate pmic8058_led_data for each led, so one "u8 reg" should be
> sufficient.

Agree. I will check and update.

> 
>> +};
>> +
>> +#define PM8058_MAX_LEDS		7
>> +
>> +static void led_kp_set(struct pmic8058_led_data *led, enum led_brightness value)
>> +{
>> +	int rc;
>> +	u8 level;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&led->value_lock, flags);
> This function is only ever called from within the workqueue so there is no need for
> locking.

There is a historical reason here, but not application to upstream. You can see that we
have flash_drv lines also for flash leds in this code, which is used to drive
the camera flash leds. The core problem here is that there is no "camera flash" framework
in the v4l2 which could drive these pins from the kernel space, so internally we had
to also export this _set functions so that v4l2 drivers can use them to control
the brightness of the camera flash.

Looking at another way, you can use all of these low level leds, flash leds and keyboard
leds drive strengths by combining them into the h/w and driver single camera flash requiring
around 990mA of current.

I could remove these spin locks from this code though, as we don't have generic solution
in upstream to handle in-kernel usage of LED apis.


>> +
>> +static enum led_brightness led_kp_get(struct pmic8058_led_data *led)
>> +{
>> +	if ((led->reg_kp & PM8058_DRV_KEYPAD_BL_MASK) >>
>> +			 PM8058_DRV_KEYPAD_BL_SHIFT)
>> +		return LED_FULL;
>> +	else
>> +		return LED_OFF;
>> +}
>> +
> 
> Shouldn't you be returning the actual brightness here instead of only either on or
> off? The brightness is btw. stored in led->brightness, so you can use the same getter
> for all three types of leds.

Ack.

> 
> 
>> +static void led_lc_set(struct pmic8058_led_data *led, enum led_brightness value)
>> +{
>> +	unsigned long flags;
>> +	int rc, offset;
>> +	u8 level, tmp;
>> +
>> +	spin_lock_irqsave(&led->value_lock, flags);
> This function is only ever called from within the workqueue so there is no need for
> locking.

As pointed above. Ack.

>> +
>> +static void pmic8058_led_work(struct work_struct *work)
>> +{
>> +	struct pmic8058_led_data *led = container_of(work,
>> +					 struct pmic8058_led_data, work);
>> +
>> +	mutex_lock(&led->lock);
>> +
> Since this is a workqueue and there will only one running instance per led at a time
> there is no need to take a lock here.

I will check this. 

>> +
>> +static void pmic8058_led_set(struct led_classdev *led_cdev,
>> +	enum led_brightness value)
>> +{
>> +	struct pmic8058_led_data *led;
>> +	unsigned long flags;
>> +
>> +	led = container_of(led_cdev, struct pmic8058_led_data, cdev);
>> +
>> +	spin_lock_irqsave(&led->value_lock, flags);
> Locking is not really required here since it is only a single assignment...
>> +	led->brightness = value;
>> +	schedule_work(&led->work);
> and scheudule_work does not have to be inside of the lock.
>> +	spin_unlock_irqrestore(&led->value_lock, flags);

locks will go away.

>> +}
>> +
>> +static enum led_brightness pmic8058_led_get(struct led_classdev *led_cdev)
>> +{
>> +	struct pmic8058_led_data *led;
>> +
>> +	led = container_of(led_cdev, struct pmic8058_led_data, cdev);
> return led->brightness; (See above)

Ack.

>> +
>> +static int pmic8058_led_probe(struct platform_device *pdev)
> __devinit

Ack.

>> +
>> +	pm_chip = platform_get_drvdata(pdev);
> This looks at least a bit bogus since you'll overwrite the drvdata later. Can't you
> get the pm8058_chip through pdev->dev.parent somehow?

I will update this once the PMIC8058 core driver gets updated.

>> +	if (pm_chip == NULL) {
>> +		dev_err(&pdev->dev, "no parent data passed in\n");
>> +		return -EFAULT;
> -EINVAL

Sure. 

>> +
>> +	if (pdata->num_leds > PMIC8058_MAX_LEDS) {
>> +		dev_err(&pdev->dev, "can't handle more than %d LEDS\n",
>> +				 PMIC8058_MAX_LEDS);
>> +		return -EFAULT;
> -EINVAL

Ack.

>> +	}
>> +
>> +	led = kzalloc(sizeof(*led) * pdata->num_leds, GFP_KERNEL);
> Use kcalloc instead of kzalloc.

Ok.

>> +
>> +	rc = pm8058_read(pm_chip, SSBI_REG_ADDR_DRV_KEYPAD, &reg_kp,
>> +				1);
>> +	if (rc) {
>> +		dev_err(&pdev->dev, "can't get keypad backlight level\n");
>> +		goto err_reg_read;
>> +	}
>> +
>> +	rc = pm8058_read(pm_chip, SSBI_REG_ADDR_LED_CTRL_BASE,
>> +			reg_led_ctrl, 3);
>> +	if (rc) {
>> +		dev_err(&pdev->dev, "can't get led levels\n");
>> +		goto err_reg_read;
>> +	}
>> +
>> +	rc = pm8058_read(pm_chip, SSBI_REG_ADDR_FLASH_DRV0,
>> +			&reg_flash_led0, 1);
>> +	if (rc) {
>> +		dev_err(&pdev->dev, "can't read flash led0\n");
>> +		goto err_reg_read;
>> +	}
>> +
>> +	rc = pm8058_read(pm_chip, SSBI_REG_ADDR_FLASH_DRV1,
>> +			&reg_flash_led1, 1);
>> +	if (rc) {
>> +		dev_err(&pdev->dev, "can't get flash led1\n");
>> +		goto err_reg_read;
>> +	}
> How about adding a helper function which will initializes the leds 'reg' field
> depending on its id? It will certainly to improve code readability.

Sure. I will add it.

>> +
>> +static int __devexit pmic8058_led_remove(struct platform_device *pdev)
>> +{
>> +	int i;
>> +	struct pmic8058_leds_platform_data *pdata = pdev->dev.platform_data;
>> +	struct pmic8058_led_data *led = platform_get_drvdata(pdev);
>> +
>> +	for (i = 0; i < pdata->num_leds; i++) {
>> +		mutex_destroy(&led[led->id].lock);
>> +		led_classdev_unregister(&led[led->id].cdev);
>> +		cancel_work_sync(&led[led->id].work);
>> +	}
> 
> You need to free the 'led' array.

Thanks. Missed it.

>> +
>> +/**
>> + * struct pmic8058_led - per led data
>> + * @name - name of the led
>> + * @default_trigger - default trigger which needs to e attached
>> + * @max_brightness - maximum brightness level supported by the led
>> + * @id - supported led id
>> + */
>> +struct pmic8058_led {
>> +	const char	*name;
>> +	const char	*default_trigger;
>> +	unsigned	max_brightness;
> Should max_brightness not rather be hardcoded in the driver? As far as I can tell it
> depend on the hardware and is 4 bits wide for flash and bl leds and 5 bits for the
> others.
>> +	int		id;
> 
> enum pmic8058_leds instead of int

Ack.

>> +struct pmic8058_leds_platform_data {
>> +	int			num_leds;
> size_t

Ack.

>> +	struct pmic8058_led	*leds;
>> +};
> 
> 
> If max_brightness is hardcoded in the driver you can reuse "struct led_info" and
> "struct struct led_platform_data" instead of adding your own structs.

I will check this and see if I can add it in next version.

Thanks for the review. 

---Trilok Soni

Hi Peter,

> 
>>> +
>>> +/**
>>> + * struct pmic8058_led - per led data
>>> + * @name - name of the led
>>> + * @default_trigger - default trigger which needs to e attached
>>> + * @max_brightness - maximum brightness level supported by the led
>>> + * @id - supported led id
>>> + */
>>> +struct pmic8058_led {
>>> +	const char	*name;
>>> +	const char	*default_trigger;
>>> +	unsigned	max_brightness;
>> Should max_brightness not rather be hardcoded in the driver? As far as I can tell it
>> depend on the hardware and is 4 bits wide for flash and bl leds and 5 bits for the
>> others.
>>> +	int		id;
>>
>> enum pmic8058_leds instead of int
> 
> Ack.
> 
>>> +struct pmic8058_leds_platform_data {
>>> +	int			num_leds;
>> size_t
> 
> Ack.
> 
>>> +	struct pmic8058_led	*leds;
>>> +};
>>
>>
>> If max_brightness is hardcoded in the driver you can reuse "struct led_info" and
>> "struct struct led_platform_data" instead of adding your own structs.
> 

I couldn't remove these pmic8058_led structure due to the "enum pmic8058_led id" member
info which I need from every led. This can be removed completely only if I abuse
the "flags" parameter in struct led_info to pass the led id. Let me know what you think.

---Trilok Soni

On 12/06/2010 02:44 PM, Trilok Soni wrote:
> Hi Peter,
> 
>>
>>>> +
>>>> +/**
>>>> + * struct pmic8058_led - per led data
>>>> + * @name - name of the led
>>>> + * @default_trigger - default trigger which needs to e attached
>>>> + * @max_brightness - maximum brightness level supported by the led
>>>> + * @id - supported led id
>>>> + */
>>>> +struct pmic8058_led {
>>>> +	const char	*name;
>>>> +	const char	*default_trigger;
>>>> +	unsigned	max_brightness;
>>> Should max_brightness not rather be hardcoded in the driver? As far as I can tell it
>>> depend on the hardware and is 4 bits wide for flash and bl leds and 5 bits for the
>>> others.
>>>> +	int		id;
>>>
>>> enum pmic8058_leds instead of int
>>
>> Ack.
>>
>>>> +struct pmic8058_leds_platform_data {
>>>> +	int			num_leds;
>>> size_t
>>
>> Ack.
>>
>>>> +	struct pmic8058_led	*leds;
>>>> +};
>>>
>>>
>>> If max_brightness is hardcoded in the driver you can reuse "struct led_info" and
>>> "struct struct led_platform_data" instead of adding your own structs.
>>
> 
> I couldn't remove these pmic8058_led structure due to the "enum pmic8058_led id" member
> info which I need from every led. This can be removed completely only if I abuse
> the "flags" parameter in struct led_info to pass the led id. Let me know what you think.
> 
> ---Trilok Soni
> 

Hi

I think that would be ok, other drivers seem to do the same.

- Lars

Hi Peter,

>>>>
>>>> If max_brightness is hardcoded in the driver you can reuse "struct led_info" and
>>>> "struct struct led_platform_data" instead of adding your own structs.
>>>
>>
>> I couldn't remove these pmic8058_led structure due to the "enum pmic8058_led id" member
>> info which I need from every led. This can be removed completely only if I abuse
>> the "flags" parameter in struct led_info to pass the led id. Let me know what you think.
> 
> Hi
> 
> I think that would be ok, other drivers seem to do the same.

Thanks. I will use "flags" parameter for "id" then and submit the re-worked version as V2.

---Trilok Soni