[PATCH] leds: Add LED class driver for regulator driven LEDs.

[PATCH] leds: Add LED class driver for regulator driven LEDs.

[Mark Brown]

On Wed, Dec 02, 2009 at 06:40:25PM +0100, Antonio Ospite wrote:

> A doubt I had was about leaving the 'supply' variable configurable from
> platform data, or relying on some fixed value like "vled", but leaving it
> configurable covers the case when we have different regulators used for
> different LEDs or vibrators.

There's no need to do this since the regulator API matches consumers
based on struct device as well as name so you can have as many LEDs as
you like all using the same supply name mapping to different regulators.

> Should I add a note in Documentation/ about how to use it? Tell me if so.

If you wish to, it's not essential (only one existing LED driver appears
to do this) and the comment in the header file is probably adequate.

> +static inline int led_regulator_get_max_brightness(struct regulator *supply)
> +{
> +	return regulator_count_voltages(supply);
> +}

More graceful handling of regulators that don't implement list_voltage
might be nice (for simple on/off control - not all regulators have
configurable voltages).  If a regulator doesn't support list_voltage
you'll get -EINVAL.

> +	vcc = regulator_get(&pdev->dev, pdata->supply);
> +	if (IS_ERR(vcc)) {
> +		dev_err(&pdev->dev, "Cannot get vcc for %s\n", pdata->name);
> +		return PTR_ERR(vcc);;
> +	}

You almost certainly want regulator_get_exclusive() here; this driver
can't function properly if something else is using the regulator and
holding the LED on or off without it.  You'll also want to check the
status of the LED on startup and update your internal status to match
that.

[PATCH] leds: Add LED class driver for regulator driven LEDs.

[Liam Girdwood]

On Wed, 2009-12-02 at 18:40 +0100, Antonio Ospite wrote:
> This driver provides an interface for controlling LEDs (or vibrators)
> connected to PMICs for which there is a regulator framework driver.
> 
> This driver can be used, for instance, to control vibrator on all Motorola EZX
> phones using the pcap-regulator driver services.
> 
> Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>

Some very minor points below.

> ---
> The patch is against:
> git://git.o-hand.com/linux-rpurdie-leds for-mm
> 
> A doubt I had was about leaving the 'supply' variable configurable from
> platform data, or relying on some fixed value like "vled", but leaving it
> configurable covers the case when we have different regulators used for
> different LEDs or vibrators.
> 
> Should I add a note in Documentation/ about how to use it? Tell me if so.
> 
> Thanks,
>    Antonio
> 
> P.S.: for those who get the mail from LKML, please Cc me on reply.
> 
>  drivers/leds/Kconfig           |    6 +
>  drivers/leds/Makefile          |    1 +
>  drivers/leds/leds-regulator.c  |  214 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/leds-regulator.h |   20 ++++
>  4 files changed, 241 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/leds/leds-regulator.c
>  create mode 100644 include/linux/leds-regulator.h
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f12a996..8a0e1ec 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -229,6 +229,12 @@ config LEDS_PWM
>  	help
>  	  This option enables support for pwm driven LEDs
>  
> +config LEDS_REGULATOR
> +	tristate "REGULATOR driven LED support"
> +	depends on LEDS_CLASS && REGULATOR
> +	help
> +	  This option enables support for regulator driven LEDs.
> +
>  config LEDS_BD2802
>  	tristate "LED driver for BD2802 RGB LED"
>  	depends on LEDS_CLASS && I2C
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 176f0c6..9e63869 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_LEDS_DA903X)		+= leds-da903x.o
>  obj-$(CONFIG_LEDS_WM831X_STATUS)	+= leds-wm831x-status.o
>  obj-$(CONFIG_LEDS_WM8350)		+= leds-wm8350.o
>  obj-$(CONFIG_LEDS_PWM)			+= leds-pwm.o
> +obj-$(CONFIG_LEDS_REGULATOR)		+= leds-regulator.o
>  obj-$(CONFIG_LEDS_INTEL_SS4200)		+= leds-ss4200.o
>  obj-$(CONFIG_LEDS_LT3593)		+= leds-lt3593.o
>  obj-$(CONFIG_LEDS_ADP5520)		+= leds-adp5520.o
> diff --git a/drivers/leds/leds-regulator.c b/drivers/leds/leds-regulator.c
> new file mode 100644
> index 0000000..fca5d42
> --- /dev/null
> +++ b/drivers/leds/leds-regulator.c
> @@ -0,0 +1,214 @@
> +/*
> + * leds-regulator.c - LED class driver for regulator driven LEDs.
> + *
> + * Copyright (C) 2009 Antonio Ospite <ospite@studenti.unina.it>
> + *
> + * Inspired by leds-wm8350 driver.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/workqueue.h>
> +#include <linux/leds.h>
> +#include <linux/leds-regulator.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define to_regulator_led(led_cdev) \
> +	container_of(led_cdev, struct regulator_led, cdev)
> +
> +struct regulator_led {
> +	struct led_classdev cdev;
> +	enum led_brightness value;
> +	int enabled;
> +	struct mutex mutex;
> +	struct work_struct work;
> +
> +	struct regulator *vcc;
> +};
> +
> +static inline int led_regulator_get_max_brightness(struct regulator *supply)
> +{
> +	return regulator_count_voltages(supply);
> +}
> +
> +static int led_regulator_get_vdd(struct regulator *supply,
> +		enum led_brightness brightness)
> +{
> +	if (brightness == 0)
> +		return 0;
> +
> +	return regulator_list_voltage(supply, brightness - 1);
> +}
> +
> +
> +static void regulator_led_enable(struct regulator_led *led)
> +{
> +	int ret;
> +
> +	if (led->enabled)
> +		return;
> +
> +	ret = regulator_enable(led->vcc);
> +	if (ret != 0) {
> +		dev_err(led->cdev.dev, "Failed to enable vcc: %d\n", ret);
> +		return;
> +	}
> +
> +	led->enabled = 1;
> +}
> +
> +static void regulator_led_disable(struct regulator_led *led)
> +{
> +	int ret;
> +
> +	if (!led->enabled)
> +		return;
> +
> +	ret = regulator_disable(led->vcc);
> +	if (ret != 0) {
> +		dev_err(led->cdev.dev, "Failed to disable vcc: %d\n", ret);
> +		return;
> +	}
> +
> +	led->enabled = 0;
> +}
> +
> +static void led_work(struct work_struct *work)
> +{
> +	struct regulator_led *led;
> +	int voltage;
> +	int ret;
> +
> +	led = container_of(work, struct regulator_led, work);
> +
> +	mutex_lock(&led->mutex);
> +
> +	if (led->value == 0) {

LED_OFF instead of 0 here ?

> +		regulator_led_disable(led);
> +		goto out;
> +	}
> +
> +	voltage = led_regulator_get_vdd(led->vcc, led->value);
> +	dev_dbg(led->cdev.dev, "brightness: %d voltage: %d",
> +			led->value, voltage);
> +
> +	ret = regulator_set_voltage(led->vcc, voltage, voltage);
> +	if (ret != 0)
> +		dev_err(led->cdev.dev, "Failed to set voltage %d: %d\n",
> +			voltage, ret);

Some regulators may not support voltage change (via hw design or
constraints) so set_voltage() will fail. We probably want to handle this
regulator type so we don't call set_voltage().

> +
> +	regulator_led_enable(led);
> +
> +out:
> +	mutex_unlock(&led->mutex);
> +}
> +
> +static void regulator_led_brightness_set(struct led_classdev *led_cdev,
> +			   enum led_brightness value)
> +{
> +	struct regulator_led *led = to_regulator_led(led_cdev);
> +
> +	led->value = value;
> +	schedule_work(&led->work);
> +}
> +
> +static int regulator_led_probe(struct platform_device *pdev)
> +{
> +	struct led_regulator_platform_data *pdata = pdev->dev.platform_data;
> +	struct regulator_led *led;
> +	struct regulator *vcc;
> +	int ret;
> +
> +	if (pdata == NULL) {
> +		dev_err(&pdev->dev, "no platform data\n");
> +		return -ENODEV;
> +	}
> +
> +	vcc = regulator_get(&pdev->dev, pdata->supply);
> +	if (IS_ERR(vcc)) {
> +		dev_err(&pdev->dev, "Cannot get vcc for %s\n", pdata->name);
> +		return PTR_ERR(vcc);;

double ;; here

> +	}
> +
> +	led = kzalloc(sizeof(*led), GFP_KERNEL);
> +	if (led == NULL) {
> +		ret = -ENOMEM;
> +		goto err_vcc;
> +	}
> +
> +	ret = led_regulator_get_max_brightness(vcc);
> +	if (ret < 0)
> +		goto err_led;
> +
> +	led->cdev.max_brightness = ret;
> +
> +	led->cdev.brightness_set = regulator_led_brightness_set;
> +	led->cdev.name = pdata->name;
> +	led->cdev.flags |= LED_CORE_SUSPENDRESUME;
> +	led->enabled = regulator_is_enabled(vcc);
> +	led->vcc = vcc;
> +
> +	mutex_init(&led->mutex);
> +	INIT_WORK(&led->work, led_work);
> +
> +	led->value = LED_OFF;
> +	platform_set_drvdata(pdev, led);
> +
> +	ret = led_classdev_register(&pdev->dev, &led->cdev);
> +	if (ret < 0) {
> +		cancel_work_sync(&led->work);
> +		goto err_led;
> +	}
> +
> +	return 0;
> +
> +err_led:
> +	kfree(led);
> +err_vcc:
> +	regulator_put(vcc);
> +	return ret;
> +}
> +
> +static int regulator_led_remove(struct platform_device *pdev)
> +{
> +	struct regulator_led *led = platform_get_drvdata(pdev);
> +
> +	led_classdev_unregister(&led->cdev);
> +	cancel_work_sync(&led->work);
> +	regulator_led_disable(led);
> +	regulator_put(led->vcc);
> +	kfree(led);
> +	return 0;
> +}
> +
> +static struct platform_driver regulator_led_driver = {
> +	.driver = {
> +		   .name  = "leds-regulator",
> +		   .owner = THIS_MODULE,
> +		   },
> +	.probe  = regulator_led_probe,
> +	.remove = regulator_led_remove,
> +};
> +
> +static int __devinit regulator_led_init(void)
> +{
> +	return platform_driver_register(&regulator_led_driver);
> +}
> +module_init(regulator_led_init);
> +
> +static void regulator_led_exit(void)
> +{
> +	platform_driver_unregister(&regulator_led_driver);
> +}
> +module_exit(regulator_led_exit);
> +
> +MODULE_AUTHOR("Antonio Ospite <ospite@studenti.unina.it>");
> +MODULE_DESCRIPTION("Regulator driven LED driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:leds-regulator");
> diff --git a/include/linux/leds-regulator.h b/include/linux/leds-regulator.h
> new file mode 100644
> index 0000000..a5850fd
> --- /dev/null
> +++ b/include/linux/leds-regulator.h
> @@ -0,0 +1,20 @@
> +/*
> + * leds-regulator.h - platform data structure for regulator driven LEDs.
> + *
> + * Copyright (C) 2009 Antonio Ospite <ospite@studenti.unina.it>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#ifndef __LINUX_LEDS_REGULATOR_H
> +#define __LINUX_LEDS_REGULATOR_H
> +
> +struct led_regulator_platform_data {
> +	char *name;	/* LED name as expected by LED class */
> +	char *supply;
> +};
> +
> +#endif /* __LINUX_LEDS_REGULATOR_H */

[PATCH] leds: Add LED class driver for regulator driven LEDs.

[Antonio Ospite]

On Wed, 2 Dec 2009 18:06:58 +0000
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Wed, Dec 02, 2009 at 06:40:25PM +0100, Antonio Ospite wrote:
> 
> > A doubt I had was about leaving the 'supply' variable configurable from
> > platform data, or relying on some fixed value like "vled", but leaving it
> > configurable covers the case when we have different regulators used for
> > different LEDs or vibrators.
> 
> There's no need to do this since the regulator API matches consumers
> based on struct device as well as name so you can have as many LEDs as
> you like all using the same supply name mapping to different regulators.
>

I need some more explanation here, I am currently using the driver with
this code:

+/* VVIB: Vibrator on A780, A1200, A910, E6, E2 */
+static struct regulator_consumer_supply pcap_regulator_VVIB_consumers
[] = {
+	{ .dev_name = "leds-regulator", .supply = "vibrator", },
                                                   ^^^^^^^^
+};
+
+static struct regulator_init_data pcap_regulator_VVIB_data = {
+	.constraints = {
+		.min_uV = 1300000,
+		.max_uV = 3000000,
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS |
+					REGULATOR_CHANGE_VOLTAGE,
+	},
+	.num_consumer_supplies = ARRAY_SIZE
(pcap_regulator_VVIB_consumers),
+	.consumer_supplies = pcap_regulator_VVIB_consumers,
+};

@@ -749,6 +766,10 @@ static struct pcap_subdev a780_pcap_subdevs[] = {
 		.name		= "pcap-regulator",
 		.id		= VAUX3,
 		.platform_data	= &pcap_regulator_VAUX3_data,
+	}, {
+		.name		= "pcap-regulator",
+		.id		= VVIB,
+		.platform_data	= &pcap_regulator_VVIB_data,
 	},
 };


and then:

 
@@ -872,8 +893,24 @@ static struct platform_device a780_camera = {
 	},
 };
 
+/* vibrator */
+static struct led_regulator_platform_data a780_vibrator_data = {
+	.name   = "a780::vibrator",
+	.supply = "vibrator"
                   ^^^^^^^^ I'll get the regulator with this.
+};
+
+static struct platform_device a780_vibrator = {
+	.name = "leds-regulator",
+	.id   = -1,
+	.dev  = {
+		.platform_data = &a780_vibrator_data,
+	},
+};
+
+
 static struct platform_device *a780_devices[] __initdata = {
 	&a780_gpio_keys,
+	&a780_vibrator
 };

If I set the .supply value fixed, how can I assign different
regulators to different leds? Should I use the address to the platform
device (a780_vibrator in this case) for .dev when defining the
regulator in the first place?

> > Should I add a note in Documentation/ about how to use it? Tell me if so.
> 
> If you wish to, it's not essential (only one existing LED driver appears
> to do this) and the comment in the header file is probably adequate.
>

Ok.

> > +static inline int led_regulator_get_max_brightness(struct regulator *supply)
> > +{
> > +	return regulator_count_voltages(supply);
> > +}
> 
> More graceful handling of regulators that don't implement list_voltage
> might be nice (for simple on/off control - not all regulators have
> configurable voltages).  If a regulator doesn't support list_voltage
> you'll get -EINVAL.
>

Ok, I need to study the regulator framework a bit more, but I think I
got the logic you are referring to, if we can actually list voltages
then max_brightness is the number of voltages as it is now, else if we
can only change status then max_brightness is 1 (one).

> > +	vcc = regulator_get(&pdev->dev, pdata->supply);
> > +	if (IS_ERR(vcc)) {
> > +		dev_err(&pdev->dev, "Cannot get vcc for %s\n", pdata->name);
> > +		return PTR_ERR(vcc);;
> > +	}
> 
> You almost certainly want regulator_get_exclusive() here; this driver
> can't function properly if something else is using the regulator and
> holding the LED on or off without it.  You'll also want to check the
> status of the LED on startup and update your internal status to match
> that.

Will do, thanks for reviewing the code.

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20091202/beae4876/attachment.sig>

[PATCH] leds: Add LED class driver for regulator driven LEDs.

[Antonio Ospite]

On Wed, 02 Dec 2009 18:23:55 +0000
Liam Girdwood <lrg@slimlogic.co.uk> wrote:

> On Wed, 2009-12-02 at 18:40 +0100, Antonio Ospite wrote:
> > This driver provides an interface for controlling LEDs (or vibrators)
> > connected to PMICs for which there is a regulator framework driver.
> > 
> > This driver can be used, for instance, to control vibrator on all Motorola EZX
> > phones using the pcap-regulator driver services.
> > 
> > Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
> 
> Some very minor points below.
> 

Thanks for reviewing the code.

[...]
> > +
> > +static void led_work(struct work_struct *work)
> > +{
> > +	struct regulator_led *led;
> > +	int voltage;
> > +	int ret;
> > +
> > +	led = container_of(work, struct regulator_led, work);
> > +
> > +	mutex_lock(&led->mutex);
> > +
> > +	if (led->value == 0) {
> 
> LED_OFF instead of 0 here ?
>

Ok.

> > +		regulator_led_disable(led);
> > +		goto out;
> > +	}
> > +
> > +	voltage = led_regulator_get_vdd(led->vcc, led->value);
> > +	dev_dbg(led->cdev.dev, "brightness: %d voltage: %d",
> > +			led->value, voltage);
> > +
> > +	ret = regulator_set_voltage(led->vcc, voltage, voltage);
> > +	if (ret != 0)
> > +		dev_err(led->cdev.dev, "Failed to set voltage %d: %d\n",
> > +			voltage, ret);
> 
> Some regulators may not support voltage change (via hw design or
> constraints) so set_voltage() will fail. We probably want to handle this
> regulator type so we don't call set_voltage().
>

Ok, I'll read more about the regulator framework to find out how to
check regulator capabilities.

> > +
> > +	regulator_led_enable(led);
> > +
> > +out:
> > +	mutex_unlock(&led->mutex);
> > +}
> > +
[...]
> > +static int regulator_led_probe(struct platform_device *pdev)
> > +{
> > +	struct led_regulator_platform_data *pdata = pdev->dev.platform_data;
> > +	struct regulator_led *led;
> > +	struct regulator *vcc;
> > +	int ret;
> > +
> > +	if (pdata == NULL) {
> > +		dev_err(&pdev->dev, "no platform data\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	vcc = regulator_get(&pdev->dev, pdata->supply);
> > +	if (IS_ERR(vcc)) {
> > +		dev_err(&pdev->dev, "Cannot get vcc for %s\n", pdata->name);
> > +		return PTR_ERR(vcc);;
> 
> double ;; here
>

Thanks.

[...]

Regards,
   Antonio 

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20091202/9a79008c/attachment.sig>

[PATCH] leds: Add LED class driver for regulator driven LEDs.

[Mark Brown]

On Wed, Dec 02, 2009 at 09:25:21PM +0100, Antonio Ospite wrote:
> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> > There's no need to do this since the regulator API matches consumers
> > based on struct device as well as name so you can have as many LEDs as
> > you like all using the same supply name mapping to different regulators.

> I need some more explanation here, I am currently using the driver with
> this code:

> +/* VVIB: Vibrator on A780, A1200, A910, E6, E2 */
> +static struct regulator_consumer_supply pcap_regulator_VVIB_consumers
> [] = {
> +	{ .dev_name = "leds-regulator", .supply = "vibrator", },

So you're instantiating the device with .id set to -1 (as your code
below shows), meaning there's only one leds-regulator in the system and
there's no need to number them.  If you had more than one of them then
you'd number them and then have something like:

	{ .dev_name = "leds-regulator.0", supply = "vled" },
	{ .dev_name = "leds-regulator.1", supply = "vled" },

when setting up the supplies.

> If I set the .supply value fixed, how can I assign different
> regulators to different leds? Should I use the address to the platform
> device (a780_vibrator in this case) for .dev when defining the
> regulator in the first place?

There is no need to use the .dev field, that is kept to avoid build
breakage transitioning to dev_name.

[PATCH] leds: Add LED class driver for regulator driven LEDs.

[Mark Brown]

On Wed, Dec 02, 2009 at 09:31:31PM +0100, Antonio Ospite wrote:
> Liam Girdwood <lrg@slimlogic.co.uk> wrote:

> > Some regulators may not support voltage change (via hw design or
> > constraints) so set_voltage() will fail. We probably want to handle this
> > regulator type so we don't call set_voltage().

> Ok, I'll read more about the regulator framework to find out how to
> check regulator capabilities.

Checking for errors when counting and listing the voltages ought to
cover this.

[PATCH] leds: Add LED class driver for regulator driven LEDs.

[Antonio Ospite]

On Wed, 2 Dec 2009 20:40:26 +0000
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Wed, Dec 02, 2009 at 09:25:21PM +0100, Antonio Ospite wrote:
> > Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> 
> > > There's no need to do this since the regulator API matches consumers
> > > based on struct device as well as name so you can have as many LEDs as
> > > you like all using the same supply name mapping to different regulators.
> 
> > I need some more explanation here, I am currently using the driver with
> > this code:
> 
> > +/* VVIB: Vibrator on A780, A1200, A910, E6, E2 */
> > +static struct regulator_consumer_supply pcap_regulator_VVIB_consumers
> > [] = {
> > +	{ .dev_name = "leds-regulator", .supply = "vibrator", },
> 
> So you're instantiating the device with .id set to -1 (as your code
> below shows), meaning there's only one leds-regulator in the system and
> there's no need to number them.  If you had more than one of them then
> you'd number them and then have something like:
> 
> 	{ .dev_name = "leds-regulator.0", supply = "vled" },
> 	{ .dev_name = "leds-regulator.1", supply = "vled" },
> 
> when setting up the supplies.
>

Ok, the .id in .dev_name is what I was missing.

> > If I set the .supply value fixed, how can I assign different
> > regulators to different leds? Should I use the address to the platform
> > device (a780_vibrator in this case) for .dev when defining the
> > regulator in the first place?
> 
> There is no need to use the .dev field, that is kept to avoid build
> breakage transitioning to dev_name.

Now everything is clear.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20091202/fd58a9fa/attachment.sig>

[PATCH] leds: Add LED class driver for regulator driven LEDs.

[Antonio Ospite]

On Wed, 2 Dec 2009 18:06:58 +0000
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> > +	vcc = regulator_get(&pdev->dev, pdata->supply);
> > +	if (IS_ERR(vcc)) {
> > +		dev_err(&pdev->dev, "Cannot get vcc for %s\n", pdata->name);
> > +		return PTR_ERR(vcc);;
> > +	}
> 
> You almost certainly want regulator_get_exclusive() here; this driver
> can't function properly if something else is using the regulator and
> holding the LED on or off without it.  You'll also want to check the
> status of the LED on startup and update your internal status to match
> that.

When you refer to "the status of the LED on startup" do you mean a
initial brightness value passed via leds-regulator platform data, or the
initial status or the regulator itself?

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20091204/315580c2/attachment.sig>

[PATCH] leds: Add LED class driver for regulator driven LEDs.

[Mark Brown]

On Fri, Dec 04, 2009 at 01:39:43PM +0100, Antonio Ospite wrote:
> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> > You almost certainly want regulator_get_exclusive() here; this driver
> > can't function properly if something else is using the regulator and
> > holding the LED on or off without it.  You'll also want to check the
> > status of the LED on startup and update your internal status to match
> > that.

> When you refer to "the status of the LED on startup" do you mean a
> initial brightness value passed via leds-regulator platform data, or the
> initial status or the regulator itself?

The hardware state at startup; I would imagine that you'd want to make
setting the initial state via platform data optional in order to enable
a smooth handover from bootloader to Linux where the state may depend on
the runtime state (eg, charging indication).

[PATCH] leds: Add LED class driver for regulator driven LEDs.

[Antonio Ospite]

On Wed, 2 Dec 2009 20:41:22 +0000
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Wed, Dec 02, 2009 at 09:31:31PM +0100, Antonio Ospite wrote:
> > Liam Girdwood <lrg@slimlogic.co.uk> wrote:
> 
> > > Some regulators may not support voltage change (via hw design or
> > > constraints) so set_voltage() will fail. We probably want to handle this
> > > regulator type so we don't call set_voltage().
> 
> > Ok, I'll read more about the regulator framework to find out how to
> > check regulator capabilities.
> 
> Checking for errors when counting and listing the voltages ought to
> cover this.

This is the incremental change I have in mind; if it's ok, then
a v2 is on its way.

diff --git a/drivers/leds/leds-regulator.c b/drivers/leds/leds-regulator.c
index f4c508b..8146d12 100644
--- a/drivers/leds/leds-regulator.c
+++ b/drivers/leds/leds-regulator.c
@@ -34,14 +34,23 @@ struct regulator_led {

 static inline int led_regulator_get_max_brightness(struct regulator *supply)
 {
-       return regulator_count_voltages(supply);
+       int ret = regulator_count_voltages(supply);
+
+       /* even if regulator can't change voltages,
+        * we still assume it can change status
+        * and the LED can be turned on and off.
+        */
+       if (ret == -EINVAL)
+               return 1;
+
+       return ret;
 }

 static int led_regulator_get_vdd(struct regulator *supply,
                enum led_brightness brightness)
 {
        if (brightness == 0)
-               return 0;
+               return -EINVAL;

        return regulator_list_voltage(supply, brightness - 1);
 }
@@ -95,13 +104,15 @@ static void led_work(struct work_struct *work)
        }

        voltage = led_regulator_get_vdd(led->vcc, led->value);
-       dev_dbg(led->cdev.dev, "brightness: %d voltage: %d",
-                       led->value, voltage);
-
-       ret = regulator_set_voltage(led->vcc, voltage, voltage);
-       if (ret != 0)
-               dev_err(led->cdev.dev, "Failed to set voltage %d: %d\n",
-                       voltage, ret);
+       if (voltage) {
+               dev_dbg(led->cdev.dev, "brightness: %d voltage: %d",
+                               led->value, voltage);
+
+               ret = regulator_set_voltage(led->vcc, voltage, voltage);
+               if (ret != 0)
+                       dev_err(led->cdev.dev, "Failed to set voltage %d: %d\n",
+                               voltage, ret);
+       }

        regulator_led_enable(led);


In the last hunk I could have checked (max_brightness > 1) in place
of (voltage) given the first hunk, any opinion?

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20091204/8e19f4b9/attachment.sig>

[PATCH] leds: Add LED class driver for regulator driven LEDs.

[Mark Brown]

On Fri, Dec 04, 2009 at 01:57:57PM +0100, Antonio Ospite wrote:

> This is the incremental change I have in mind; if it's ok, then
> a v2 is on its way.

Yes, this looks reasonable I think.

[PATCH v2] leds: Add LED class driver for regulator driven LEDs.

[Antonio Ospite]

On Mon,  7 Dec 2009 15:08:13 +0100
Antonio Ospite <ospite@studenti.unina.it> wrote:

> This driver provides an interface for controlling LEDs (or vibrators)
> connected to PMICs for which there is a regulator framework driver.
> 
> This driver can be used, for instance, to control vibrator on all Motorola EZX
> phones using the pcap-regulator driver services.
> 
> Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
> ---
> 
> Addressed the observations from Mark Brown, Liam Girdwood and Linus Walleij.
> 
> Patch against:
> git://git.o-hand.com/linux-rpurdie-leds for-mm
> 
> Changes since v1:
>  - Use "vled" as supply id.
>  - use regulator_get_exclusive
>  - remove double semicolon
>  - check for LED_OFF instead of 0
>  - handle regulators which can't change voltages
>  - rename led_regulator_get_vdd to led_regulator_get_voltage
>  - split regulator_led_set_value out of led_work
>  - allow setting an initial brightness value
>  - fix sections markers (__devinit, __devexit, etc.)
> 
> The only slight doubt I still have is about
> led_regulator_get_max_brightness(), I have to use regulator_set_voltage() there
> to tell if a regulator has REGULATOR_CHANGE_VOLTAGE enabled.
> 
> Regards,
>    Antonio

Ping?

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20091215/93535f4a/attachment.sig>

[PATCH v2] leds: Add LED class driver for regulator driven LEDs.

[Mark Brown]

On Mon, Dec 07, 2009 at 03:08:13PM +0100, Antonio Ospite wrote:
> This driver provides an interface for controlling LEDs (or vibrators)
> connected to PMICs for which there is a regulator framework driver.
> 
> This driver can be used, for instance, to control vibrator on all Motorola EZX
> phones using the pcap-regulator driver services.
> 
> Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>

Looks OK to me.

Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>