[1/2,v2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight
diff mbox

Message ID 1363126934-8754-2-git-send-email-achew@nvidia.com
State New, archived
Headers show

Commit Message

achew@nvidia.com March 12, 2013, 10:22 p.m. UTC
The pwm-backlight driver now takes a mandatory regulator that is gotten
during driver probe.  Initialize a dummy regulator to satisfy this
requirement.

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
 arch/arm/mach-omap2/board-4430sdp.c |    5 +++++
 1 file changed, 5 insertions(+)

Comments

Peter Ujfalusi March 13, 2013, 8:51 a.m. UTC | #1
On 03/12/2013 11:22 PM, Andrew Chew wrote:
> 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:

> 
> Signed-off-by: Andrew Chew <achew@nvidia.com>
> ---
>  arch/arm/mach-omap2/board-4430sdp.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/board-4430sdp.c b/arch/arm/mach-omap2/board-4430sdp.c
> index 35f3ad0..62022c0 100644
> --- a/arch/arm/mach-omap2/board-4430sdp.c
> +++ b/arch/arm/mach-omap2/board-4430sdp.c
> @@ -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");

> +
>  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);
>
achew@nvidia.com March 13, 2013, 8:38 p.m. UTC | #2
> > +/* 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding March 13, 2013, 8:59 p.m. UTC | #3
On Wed, Mar 13, 2013 at 01:38:31PM -0700, Andrew Chew wrote:
> > > +/* 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?

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
achew@nvidia.com March 13, 2013, 9:12 p.m. UTC | #4
> From: Thierry Reding [mailto:thierry.reding@avionic-design.de]
> Sent: Wednesday, March 13, 2013 1:59 PM
> To: Andrew Chew
> Cc: Peter Ujfalusi; Alex Courbot; linux-omap@vger.kernel.org
> Subject: Re: [PATCH 1/2 v2] ARM: OMAP: board-4430sdp: Provide regulator
> to pwm-backlight
> 
> * PGP Signed by an unknown key
> 
> On Wed, Mar 13, 2013 at 01:38:31PM -0700, Andrew Chew wrote:
> > > > +/* 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?
> 
> 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.

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/arch/arm/mach-omap2/board-4430sdp.c b/arch/arm/mach-omap2/board-4430sdp.c
index 35f3ad0..62022c0 100644
--- a/arch/arm/mach-omap2/board-4430sdp.c
+++ b/arch/arm/mach-omap2/board-4430sdp.c
@@ -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);