Add mandatory regulator for all users of pwm-backlight.
Subsystems (6)
v2 cover Add mandatory regulator for all users of pwm-backlight.
Many backlights are enabled via GPIO. We can generalize the GPIO to a fixed regulator.
The enable regulator needs to be mandatory because there was no good way to determine the difference between opting out of the regulator, and probe deferral.
This series of patches is intended to add a dummy regulator (or a GPIO regulator) for all users of the pwm-backlight.
The last patch in the series will always be the pwm-backlight change to add this mandatory regulator. Patches following up to that patch add the mandatory regulator on a per mach family basis. Once all users of pwm-backlight have been patched, this series can be applied in order to maintain bisectability.
Andrew Chew (2):
ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight
pwm_bl: Add mandatory backlight enable regulator
.../bindings/video/backlight/pwm-backlight.txt | 14 +++++ arch/arm/mach-omap2/board-4430sdp.c | 5 ++ drivers/video/backlight/pwm_bl.c | 55 ++++++++++++++++---- 3 files changed, 64 insertions(+), 10 deletions(-)
Signature · 1 line
1.7.9.5
PATCH v2 1/2 ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight
The pwm-backlight driver now takes a mandatory regulator that is gotten during driver probe. Initialize a dummy regulator to satisfy this requirement.
--- arch/arm/mach-omap2/board-4430sdp.c | 5 +++++ 1 file changed, 5 insertions(+)
arch/arm/mach-omap2/board-4430sdp.c +5 -0
@@ -291,6 +291,10 @@ static struct platform_device sdp4430_leds_pwm = { }, }; +/* Dummy regulator for pwm-backlight driver */ +static struct regulator_consumer_supply backlight_supply = +REGULATOR_SUPPLY ("enable" ,NULL ); +static struct platform_pwm_backlight_data sdp4430_backlight_data = { .max_brightness =127 , .dft_brightness =127 ,
@@ -718,6 +722,7 @@ static void __init omap_4430sdp_init(void)omap4_i2c_init ();omap_sfh7741prox_init (); +regulator_register_always_on (-1 ,"bl-enable" , &backlight_supply,1 ,0 );platform_add_devices (sdp4430_devices,ARRAY_SIZE (sdp4430_devices));omap_serial_init ();omap_sdrc_init (NULL ,NULL );
Signature · 1 line
1.7.9.5
Re: [PATCH 1/2 v2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight
Quoted 3 lines · Andrew Chew
The pwm-backlight driver now takes a mandatory regulator that is gotten during driver probe. Initialize a dummy regulator to satisfy this requirement.
I can test this tomorrow, but I have one comment:
Quoted 11 lines
Signed-off-by Andrew Chew <[email protected]>--- arch/arm/mach-omap2/board-4430sdp.c | 5 +++++ 1 file changed, 5 insertions(+)arch/arm/mach-omap2/board-4430sdp.c +0
-0
arch/arm/mach-omap2/board-4430sdp.c static struct platform_device sdp4430_leds_pwm = { · line 296
Quoted 6 lines
arch/arm/mach-omap2/board-4430sdp.c +3
-0@@ -291,6 +291,10 @@ static struct platform_device sdp4430_leds_pwm = { }, }; +/* Dummy regulator for pwm-backlight driver */ +static struct regulator_consumer_supply backlight_supply = +REGULATOR_SUPPLY ("enable" ,NULL );
'enable' is just too generic, the device name should be also provided:
REGULATOR_SUPPLY("enable", "pwm-backlight");Quoted 13 lines
arch/arm/mach-omap2/board-4430sdp.c +2
-0+static struct platform_pwm_backlight_data sdp4430_backlight_data = { .max_brightness =127 , .dft_brightness =127 ,@@ -718,6 +722,7 @@ static void __init omap_4430sdp_init(void)omap4_i2c_init ();omap_sfh7741prox_init (); +regulator_register_always_on (-1 ,"bl-enable" , &backlight_supply,1 ,0 );platform_add_devices (sdp4430_devices,ARRAY_SIZE (sdp4430_devices));omap_serial_init ();omap_sdrc_init (NULL ,NULL );
Signature · 5 lines
Péter -- 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
RE: [PATCH 1/2 v2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+/* Dummy regulator for pwm-backlight driver */ static struct +regulator_consumer_supply backlight_supply = + REGULATOR_SUPPLY("enable", NULL);'enable' is just too generic, the device name should be also provided:
REGULATOR_SUPPLY("enable", "pwm-backlight");
You're right. I don't like how generic it is as well. But I don't think "pwm-backlight" works...at least, not for me when I test it. What does work is "backlight.x" where x is some number (for me, it's 1). Problem is, I don't know what it would be for you. If only there was a way to wildcard that...
Would it be better if we called the regulator something other than "enable"? Maybe "backlight-enable", or "bl-enable" for brevity?
Re: [PATCH 1/2 v2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight
Quoted 7 lines · Andrew Chew
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+/* Dummy regulator for pwm-backlight driver */ static struct +regulator_consumer_supply backlight_supply = + REGULATOR_SUPPLY("enable", NULL);'enable' is just too generic, the device name should be also provided:
REGULATOR_SUPPLY("enable", "pwm-backlight");
Quoted 8 lines
You're right. I don't like how generic it is as well. But I don't think "pwm-backlight" works...at least, not for me when I test it. What does work is "backlight.x" where x is some number (for me, it's 1). Problem is, I don't know what it would be for you. If only there was a way to wildcard that...
Would it be better if we called the regulator something other than "enable"? Maybe "backlight-enable", or "bl-enable" for brevity?
The second parameter needs to match the device name. For the 4430sdp board this should be "pwm-backlight" since the name will be generated from the .name and .id fields of the struct platform_device. .id = -1 will result in no .<id> suffix being attached, so the device should be named "pwm-backlight". The first parameter needs to match the name of the supply that the driver requests, so "enable" is correct since the call to regulator_get() uses that.
Thierry
RE: [PATCH 1/2 v2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight
Quoted 26 lines
From: Thierry Reding [mailto:[email protected]]
Sent: Wednesday, March 13, 2013 1:59 PM
To: Andrew Chew
Cc: Peter Ujfalusi; Alex Courbot; [email protected]
Subject: Re: [PATCH 1/2 v2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight* PGP Signed by an unknown key
Quoted 7 lines · Andrew Chew
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+/* Dummy regulator for pwm-backlight driver */ static struct +regulator_consumer_supply backlight_supply = + REGULATOR_SUPPLY("enable", NULL);'enable' is just too generic, the device name should be also provided:
REGULATOR_SUPPLY("enable", "pwm-backlight");Quoted 8 lines
You're right. I don't like how generic it is as well. But I don't think "pwm-backlight" works...at least, not for me when I test it. What does work is "backlight.x" where x is some number (for me, it's 1). Problem is, I don't know what it would be for you. If only there was a way to wildcard that...
Would it be better if we called the regulator something other than "enable"? Maybe "backlight-enable", or "bl-enable" for brevity?
Quoted 6 lines
The second parameter needs to match the device name. For the 4430sdp board this should be "pwm-backlight" since the name will be generated from the .name and .id fields of the struct
platform_device. .id = -1 will result in no .<id> suffix being attached, so the device should be named "pwm-backlight". The first parameter needs to match the name of the supply that the driver requests, so "enable" is correct since the call toregulator_get()uses that.
Ah, I see. That makes sense. Thanks, Thierry! "pwm-backlight" it is, then, and I'll make sure to check for this for the other boards.
PATCH v2 2/2 pwm_bl: Add mandatory backlight enable regulator
In reply to Andrew ChewMany backlights need to be explicitly enabled. Typically, this is done with a GPIO. For flexibility, we generalize the enable mechanism to a regulator.
If an enable regulator is not needed, then a dummy regulator can be given to the backlight driver. If a GPIO is used to enable the backlight, then a fixed regulator can be instantiated to control the GPIO.
The backlight enable regulator can be specified in the device tree node for the backlight, or can be done with legacy board setup code in the usual way.
--- .../bindings/video/backlight/pwm-backlight.txt | 14 +++++ drivers/video/backlight/pwm_bl.c | 55 ++++++++++++++++---- 2 files changed, 59 insertions(+), 10 deletions(-)
Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt +14 -0
@@ -10,6 +10,11 @@ Required properties: last value in the array represents a 100% duty cycle (brightest). - default-brightness-level: the default brightness level (index into the array defined by the "brightness-levels" property) + - enable-supply: A phandle to the regulator device tree node. This + regulator will be turned on and off as the pwm is enabled and disabled. + Many backlights are enabled via a GPIO. In this case, we instantiate + a fixed regulator and give that to enable-supply. If a regulator + is not needed, then provide a dummy fixed regulator. Optional properties: - pwm-names: a list of names for the PWM devices specified in the
@@ -19,10 +24,19 @@ Optional properties: Example: + bl_en: fixed-regulator { + compatible = "regulator-fixed"; + regulator-name = "bl-en-supply"; + regulator-boot-on; + gpio = <&some_gpio>; + enable-active-high; + }; + backlight { compatible = "pwm-backlight"; pwms = <&pwm 0 5000000>; brightness-levels = <0 4 8 16 32 64 128 255>; default-brightness-level = <6>; + enable-supply = <&bl_en>; };
drivers/video/backlight/pwm_bl.c +45 -10
107 lines not on this page · raw message
Signature · 1 line
1.7.9.5
Re: [PATCH 2/2 v2] pwm_bl: Add mandatory backlight enable regulator
Quoted 78 lines · Andrew Chew
Many backlights need to be explicitly enabled. Typically, this is done with a GPIO. For flexibility, we generalize the enable mechanism to a regulator.
If an enable regulator is not needed, then a dummy regulator can be given to the backlight driver. If a GPIO is used to enable the backlight, then a fixed regulator can be instantiated to control the GPIO.
The backlight enable regulator can be specified in the device tree node for the backlight, or can be done with legacy board setup code in the usual way.
Signed-off-by Andrew Chew <[email protected]>Reviewed-by Alexandre Courbot <[email protected]>--- .../bindings/video/backlight/pwm-backlight.txt | 14 +++++ drivers/video/backlight/pwm_bl.c | 55 ++++++++++++++++---- 2 files changed, 59 insertions(+), 10 deletions(-)Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt +14
-0@@ -10,6 +10,11 @@ Required properties: last value in the array represents a 100% duty cycle (brightest). - default-brightness-level: the default brightness level (index into the array defined by the "brightness-levels" property) + - enable-supply: A phandle to the regulator device tree node. This + regulator will be turned on and off as the pwm is enabled and disabled. + Many backlights are enabled via a GPIO. In this case, we instantiate + a fixed regulator and give that to enable-supply. If a regulator + is not needed, then provide a dummy fixed regulator. Optional properties: - pwm-names: a list of names for the PWM devices specified in the@@ -19,10 +24,19 @@ Optional properties: Example: + bl_en: fixed-regulator { + compatible = "regulator-fixed"; + regulator-name = "bl-en-supply"; + regulator-boot-on; + gpio = <&some_gpio>; + enable-active-high; + }; + backlight { compatible = "pwm-backlight"; pwms = <&pwm 0 5000000>; brightness-levels = <0 4 8 16 32 64 128 255>; default-brightness-level = <6>; + enable-supply = <&bl_en>; };drivers/video/backlight/pwm_bl.c +4
-0@@ -20,10 +20,13 @@#include <linux/pwm.h> #include <linux/pwm_backlight.h> #include <linux/slab.h> +#include <linux/regulator/consumer.h> struct pwm_bl_data {struct pwm_device *pwm;struct device *dev; +bool enabled; +struct regulator *enable_supply;unsigned int period;unsigned int lth_brightness;unsigned int *levels;@@ -35,6 +38,38 @@ struct pwm_bl_data {void (*exit)(struct device *); }; +static void pwm_backlight_enable (struct backlight_device *bl)
drivers/video/backlight/pwm_bl.c struct pwm_bl_data { · line 51
Quoted 10 lines
drivers/video/backlight/pwm_bl.c +10
-0+{ +struct pwm_bl_data *pb =dev_get_drvdata (&bl->dev); + +/* Bail if we are already enabled. */ +if (pb->enabled) +return ; + +pwm_enable (pb->pwm); + +if (regulator_enable (pb->enable_supply) !=0 )
I would loose the '!= 0'
Quoted 10 lines
drivers/video/backlight/pwm_bl.c +10
-0+dev_warn (&bl->dev,"Failed to enable regulator" ); + + pb->enabled = true; +} + +static void pwm_backlight_disable (struct backlight_device *bl) +{ +struct pwm_bl_data *pb =dev_get_drvdata (&bl->dev); + +/* Bail if we are already disabled. */
drivers/video/backlight/pwm_bl.c struct pwm_bl_data { · line 71
Quoted 10 lines
drivers/video/backlight/pwm_bl.c +10
-0+if (!pb->enabled) +return ; + +if (regulator_disable (pb->enable_supply) !=0 ) +dev_warn (&bl->dev,"Failed to disable regulator" ); + +pwm_disable (pb->pwm); + + pb->enabled = false; +}
Would it not be better to have some locking here since the code started to use flag for state tracking?
Quoted 67 lines
drivers/video/backlight/pwm_bl.c +11
-10+static int pwm_backlight_update_status (struct backlight_device *bl) {struct pwm_bl_data *pb =bl_get_data (bl);@@ -51,7 +86,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)if (brightness ==0 ) {pwm_config (pb->pwm,0 , pb->period); -pwm_disable (pb->pwm); +pwm_backlight_disable (bl); }else {int duty_cycle;@@ -65,7 +100,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl) duty_cycle = pb->lth_brightness + (duty_cycle * (pb->period - pb->lth_brightness) / max);pwm_config (pb->pwm, duty_cycle, pb->period); -pwm_enable (pb->pwm); +pwm_backlight_enable (bl); }if (pb->notify_after)@@ -138,12 +173,6 @@ static int pwm_backlight_parse_dt(struct device *dev, data->max_brightness--; } -/* -* TODO: Most users of this driver use a number of GPIOs to control -* backlight power. Support for specifying these needs to be -* added. -*/ -return 0 ; }@@ -206,6 +235,12 @@ static int pwm_backlight_probe(struct platform_device *pdev) pb->exit = data->exit; pb->dev = &pdev->dev; + pb->enable_supply =devm_regulator_get (&pdev->dev,"enable" ); +if (IS_ERR (pb->enable_supply)) { + ret =PTR_ERR (pb->enable_supply); + goto err_alloc; + } + pb->pwm =devm_pwm_get (&pdev->dev,NULL );if (IS_ERR (pb->pwm)) {dev_err (&pdev->dev,"unable to request PWM, trying legacy API\n" );@@ -268,7 +303,7 @@ static int pwm_backlight_remove(struct platform_device *pdev)backlight_device_unregister (bl);pwm_config (pb->pwm,0 , pb->period); -pwm_disable (pb->pwm); +pwm_backlight_disable (bl);if (pb->exit) pb->exit (&pdev->dev);return 0 ;@@ -283,7 +318,7 @@ static int pwm_backlight_suspend(struct device *dev)if (pb->notify) pb->notify (pb->dev,0 );pwm_config (pb->pwm,0 , pb->period); -pwm_disable (pb->pwm); +pwm_backlight_disable (bl);if (pb->notify_after) pb->notify_after (pb->dev,0 );return 0 ;
Signature · 5 lines
Péter -- 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
Re: [PATCH 2/2 v2] pwm_bl: Add mandatory backlight enable regulator
Quoted 1 line · Peter Ujfalusi
On 03/12/2013 11:22 PM, Andrew Chew wrote:
[...]
Quoted 10 lines
Quoted 10 lines
drivers/video/backlight/pwm_bl.c +10
-0+static void pwm_backlight_disable (struct backlight_device *bl) +{ +struct pwm_bl_data *pb =dev_get_drvdata (&bl->dev); + +/* Bail if we are already disabled. */ +if (!pb->enabled) +return ; + +if (regulator_disable (pb->enable_supply) !=0 ) +dev_warn (&bl->dev,"Failed to disable regulator" );
Quoted 8 lines
drivers/video/backlight/pwm_bl.c struct pwm_bl_data { · line 71
Quoted 5 lines
drivers/video/backlight/pwm_bl.c +5
-0+ +pwm_disable (pb->pwm); + + pb->enabled = false; +}Would it not be better to have some locking here since the code started to use flag for state tracking?
I don't think that's necessary. The backlight core already uses the ops_lock mutex to serial accesses. I just noticed that the documentation mentions that update_lock is used for this purpose, but the code doesn't use it after it is initialized. Still, the ops_lock should be enough.
Thierry
RE: [PATCH 2/2 v2] pwm_bl: Add mandatory backlight enable regulator
In reply to Peter UjfalusiQuoted 4 lines
Quoted 4 lines
drivers/video/backlight/pwm_bl.c +4
-0+static void pwm_backlight_enable (struct backlight_device *bl) { +struct pwm_bl_data *pb =dev_get_drvdata (&bl->dev); + +/* Bail if we are already enabled. */
Quoted 8 lines
drivers/video/backlight/pwm_bl.c struct pwm_bl_data { · line 51
Quoted 6 lines
drivers/video/backlight/pwm_bl.c +6
-0+if (pb->enabled) +return ; + +pwm_enable (pb->pwm); + +if (regulator_enable (pb->enable_supply) !=0 )I would loose the '!= 0'
I think I prefer the '!= 0'. Without it, it looks at first glance like regulator_enable() is following boolean semantics, so it reads kind of weird. But I'll defer to Thierry on this one. Thierry, what's your preference?
Re: [PATCH 2/2 v2] pwm_bl: Add mandatory backlight enable regulator
Quoted 13 lines · Andrew Chew
Quoted 4 lines
Quoted 4 lines
drivers/video/backlight/pwm_bl.c +4
-0+static void pwm_backlight_enable (struct backlight_device *bl) { +struct pwm_bl_data *pb =dev_get_drvdata (&bl->dev); + +/* Bail if we are already enabled. */ Quoted 8 lines
drivers/video/backlight/pwm_bl.c struct pwm_bl_data { · line 51
Quoted 6 lines
drivers/video/backlight/pwm_bl.c +6
-0+if (pb->enabled) +return ; + +pwm_enable (pb->pwm); + +if (regulator_enable (pb->enable_supply) !=0 )I would loose the '!= 0'
Quoted 4 lines
I think I prefer the '!= 0'. Without it, it looks at first glance like
regulator_enable()is following boolean semantics, so it reads kind of weird. But I'll defer to Thierry on this one. Thierry, what's your preference?
Why not assign the return value of regulator_enable() to a variable, for instance "err", and make that part of the warning message so that people will have a better chance to diagnose what's going wrong?
Thierry
RE: [PATCH 2/2 v2] pwm_bl: Add mandatory backlight enable regulator
Quoted 14 lines · Andrew Chew
Quoted 13 lines
Quoted 12 lines
drivers/video/backlight/pwm_bl.c struct pwm_bl_data { · line 51
Quoted 10 lines
drivers/video/backlight/pwm_bl.c +10
-0+static void pwm_backlight_enable (struct backlight_device *bl) { +struct pwm_bl_data *pb =dev_get_drvdata (&bl->dev); + +/* Bail if we are already enabled. */ +if (pb->enabled) +return ; + +pwm_enable (pb->pwm); + +if (regulator_enable (pb->enable_supply) !=0 )I would loose the '!= 0'
Quoted 8 lines
Quoted 4 lines
I think I prefer the '!= 0'. Without it, it looks at first glance like
regulator_enable()is following boolean semantics, so it reads kind of weird. But I'll defer to Thierry on this one. Thierry, what's your preference?Why not assign the return value of
regulator_enable()to a variable, for instance "err", and make that part of the warning message so that people will have a better chance to diagnose what's going wrong?
That's a good idea. I'll have that modification in the next patch series that I post.