diff mbox

[05/10] ARM: SAMSUNG: Initialize PWM backlight enable_gpio field

Message ID 1379972467-11243-6-git-send-email-treding@nvidia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thierry Reding Sept. 23, 2013, 9:41 p.m. UTC
The GPIO API defines 0 as being a valid GPIO number, so this field needs
to be initialized explicitly.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 arch/arm/mach-s3c24xx/mach-h1940.c    | 1 +
 arch/arm/mach-s3c24xx/mach-rx1950.c   | 1 +
 arch/arm/mach-s3c64xx/mach-crag6410.c | 1 +
 arch/arm/mach-s3c64xx/mach-hmt.c      | 1 +
 arch/arm/mach-s3c64xx/mach-smartq.c   | 1 +
 arch/arm/mach-s3c64xx/mach-smdk6410.c | 1 +
 arch/arm/mach-s5p64x0/mach-smdk6440.c | 1 +
 arch/arm/mach-s5p64x0/mach-smdk6450.c | 1 +
 arch/arm/mach-s5pc100/mach-smdkc100.c | 1 +
 arch/arm/mach-s5pv210/mach-smdkv210.c | 1 +
 arch/arm/plat-samsung/dev-backlight.c | 5 +++++
 11 files changed, 15 insertions(+)

Comments

Stephen Warren Oct. 1, 2013, 6:31 p.m. UTC | #1
On 09/23/2013 03:41 PM, Thierry Reding wrote:
> The GPIO API defines 0 as being a valid GPIO number, so this field needs
> to be initialized explicitly.

>  static void __init smdkv210_map_io(void)

> @@ -70,6 +70,7 @@ static struct samsung_bl_drvdata samsung_dfl_bl_data __initdata = {
>  		.max_brightness = 255,
>  		.dft_brightness = 255,
>  		.pwm_period_ns  = 78770,
> +		.enable_gpio    = -1,
>  		.init           = samsung_bl_init,
>  		.exit           = samsung_bl_exit,
>  	},
> @@ -121,6 +122,10 @@ void __init samsung_bl_set(struct samsung_bl_gpio_info *gpio_info,
>  		samsung_bl_data->lth_brightness = bl_data->lth_brightness;
>  	if (bl_data->pwm_period_ns)
>  		samsung_bl_data->pwm_period_ns = bl_data->pwm_period_ns;
> +	if (bl_data->enable_gpio)
> +		samsung_bl_data->enable_gpio = bl_data->enable_gpio;
> +	if (bl_data->enable_gpio_flags)
> +		samsung_bl_data->enable_gpio_flags = bl_data->enable_gpio_flags;

Won't this cause the core pwm_bl driver to request/manipulate the GPIO,
whereas this driver already does that inside the samsung_bl_init/exit
callbacks? I think you either need to adjust those callbacks, or not set
the new standard GPIO property in samsung_bl_data.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Oct. 1, 2013, 8:43 p.m. UTC | #2
On Tue, Oct 01, 2013 at 12:31:04PM -0600, Stephen Warren wrote:
> On 09/23/2013 03:41 PM, Thierry Reding wrote:
> > The GPIO API defines 0 as being a valid GPIO number, so this field needs
> > to be initialized explicitly.
> 
> >  static void __init smdkv210_map_io(void)
> 
> > @@ -70,6 +70,7 @@ static struct samsung_bl_drvdata samsung_dfl_bl_data __initdata = {
> >  		.max_brightness = 255,
> >  		.dft_brightness = 255,
> >  		.pwm_period_ns  = 78770,
> > +		.enable_gpio    = -1,
> >  		.init           = samsung_bl_init,
> >  		.exit           = samsung_bl_exit,
> >  	},
> > @@ -121,6 +122,10 @@ void __init samsung_bl_set(struct samsung_bl_gpio_info *gpio_info,
> >  		samsung_bl_data->lth_brightness = bl_data->lth_brightness;
> >  	if (bl_data->pwm_period_ns)
> >  		samsung_bl_data->pwm_period_ns = bl_data->pwm_period_ns;
> > +	if (bl_data->enable_gpio)
> > +		samsung_bl_data->enable_gpio = bl_data->enable_gpio;
> > +	if (bl_data->enable_gpio_flags)
> > +		samsung_bl_data->enable_gpio_flags = bl_data->enable_gpio_flags;
> 
> Won't this cause the core pwm_bl driver to request/manipulate the GPIO,
> whereas this driver already does that inside the samsung_bl_init/exit
> callbacks? I think you either need to adjust those callbacks, or not set
> the new standard GPIO property in samsung_bl_data.

I don't think so. The samsung_bl_data is a copy of samsung_dfl_bl_data
augmented by board-specific settings. So in fact copying these values
here is essential to allow boards to override the enable_gpio and flags
fields. Currently no board sets the enable_gpio to a valid GPIO so it's
all still handled by the callbacks only.

Thierry
Stephen Warren Oct. 1, 2013, 8:58 p.m. UTC | #3
On 10/01/2013 02:43 PM, Thierry Reding wrote:
> On Tue, Oct 01, 2013 at 12:31:04PM -0600, Stephen Warren wrote:
>> On 09/23/2013 03:41 PM, Thierry Reding wrote:
>>> The GPIO API defines 0 as being a valid GPIO number, so this
>>> field needs to be initialized explicitly.
>> 
>>> static void __init smdkv210_map_io(void)
>> 
>>> @@ -70,6 +70,7 @@ static struct samsung_bl_drvdata
>>> samsung_dfl_bl_data __initdata = { .max_brightness = 255, 
>>> .dft_brightness = 255, .pwm_period_ns  = 78770, +		.enable_gpio
>>> = -1, .init           = samsung_bl_init, .exit           =
>>> samsung_bl_exit, }, @@ -121,6 +122,10 @@ void __init
>>> samsung_bl_set(struct samsung_bl_gpio_info *gpio_info, 
>>> samsung_bl_data->lth_brightness = bl_data->lth_brightness; if
>>> (bl_data->pwm_period_ns) samsung_bl_data->pwm_period_ns =
>>> bl_data->pwm_period_ns; +	if (bl_data->enable_gpio) +
>>> samsung_bl_data->enable_gpio = bl_data->enable_gpio; +	if
>>> (bl_data->enable_gpio_flags) +
>>> samsung_bl_data->enable_gpio_flags =
>>> bl_data->enable_gpio_flags;
>> 
>> Won't this cause the core pwm_bl driver to request/manipulate the
>> GPIO, whereas this driver already does that inside the
>> samsung_bl_init/exit callbacks? I think you either need to adjust
>> those callbacks, or not set the new standard GPIO property in
>> samsung_bl_data.
> 
> I don't think so. The samsung_bl_data is a copy of
> samsung_dfl_bl_data augmented by board-specific settings. So in
> fact copying these values here is essential to allow boards to
> override the enable_gpio and flags fields. Currently no board sets
> the enable_gpio to a valid GPIO so it's all still handled by the
> callbacks only.

Oh yes, you're right. I was confusing the new enable_gpio field in
pwm_bl's platform data with some other field in a custom data structure.

One minor point though:

>>> +	if (bl_data->enable_gpio) +		samsung_bl_data->enable_gpio =
>>> bl_data->enable_gpio;

That assumes that enable_gpio==0 means "none", whereas you've gone to
great pains in the rest of the series to allow 0 to be a valid GPIO
ID. right now, the default value of samsung_bl_data->enable_gpio is
-1, and if !bl_data->enable_gpio, that value won't be propagated across.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Oct. 1, 2013, 9:23 p.m. UTC | #4
On Tue, Oct 01, 2013 at 02:58:22PM -0600, Stephen Warren wrote:
> On 10/01/2013 02:43 PM, Thierry Reding wrote:
> > On Tue, Oct 01, 2013 at 12:31:04PM -0600, Stephen Warren wrote:
> >> On 09/23/2013 03:41 PM, Thierry Reding wrote:
> >>> The GPIO API defines 0 as being a valid GPIO number, so this
> >>> field needs to be initialized explicitly.
> >> 
> >>> static void __init smdkv210_map_io(void)
> >> 
> >>> @@ -70,6 +70,7 @@ static struct samsung_bl_drvdata
> >>> samsung_dfl_bl_data __initdata = { .max_brightness = 255, 
> >>> .dft_brightness = 255, .pwm_period_ns  = 78770, +		.enable_gpio
> >>> = -1, .init           = samsung_bl_init, .exit           =
> >>> samsung_bl_exit, }, @@ -121,6 +122,10 @@ void __init
> >>> samsung_bl_set(struct samsung_bl_gpio_info *gpio_info, 
> >>> samsung_bl_data->lth_brightness = bl_data->lth_brightness; if
> >>> (bl_data->pwm_period_ns) samsung_bl_data->pwm_period_ns =
> >>> bl_data->pwm_period_ns; +	if (bl_data->enable_gpio) +
> >>> samsung_bl_data->enable_gpio = bl_data->enable_gpio; +	if
> >>> (bl_data->enable_gpio_flags) +
> >>> samsung_bl_data->enable_gpio_flags =
> >>> bl_data->enable_gpio_flags;
> >> 
> >> Won't this cause the core pwm_bl driver to request/manipulate the
> >> GPIO, whereas this driver already does that inside the
> >> samsung_bl_init/exit callbacks? I think you either need to adjust
> >> those callbacks, or not set the new standard GPIO property in
> >> samsung_bl_data.
> > 
> > I don't think so. The samsung_bl_data is a copy of
> > samsung_dfl_bl_data augmented by board-specific settings. So in
> > fact copying these values here is essential to allow boards to
> > override the enable_gpio and flags fields. Currently no board sets
> > the enable_gpio to a valid GPIO so it's all still handled by the
> > callbacks only.
> 
> Oh yes, you're right. I was confusing the new enable_gpio field in
> pwm_bl's platform data with some other field in a custom data structure.
> 
> One minor point though:
> 
> >>> +	if (bl_data->enable_gpio) +		samsung_bl_data->enable_gpio =
> >>> bl_data->enable_gpio;
> 
> That assumes that enable_gpio==0 means "none", whereas you've gone to
> great pains in the rest of the series to allow 0 to be a valid GPIO
> ID. right now, the default value of samsung_bl_data->enable_gpio is
> -1, and if !bl_data->enable_gpio, that value won't be propagated across.

Right, that check should now be:

	if (bl_data->enable_gpio >= 0)

Well, it depends. It would be possible for the default to specify a
valid GPIO and for a board to override it with -1 (and provide a set of
corresponding callbacks). In that case the right thing to do here would
be not to check at all.

Then again, I don't think that will ever happen, because no fixed GPIO
will ever be a good default. So changing to >= 0 instead of != 0 should
work fine.

Again, starting with 3.13 this should become a lot easier to handle
since the GPIO subsystem will gain functionality to use a per-board
lookup table, similarly to what the regulator and PWM subsystems do.
Once that's in place I plan to make another pass over all users of the
pwm-backlight driver and replace the enable_gpio field with a GPIO
lookup table, so that the driver can uniformly request them using a
simple gpiod_get().

Thierry
diff mbox

Patch

diff --git a/arch/arm/mach-s3c24xx/mach-h1940.c b/arch/arm/mach-s3c24xx/mach-h1940.c
index 74dd479..952b6a0 100644
--- a/arch/arm/mach-s3c24xx/mach-h1940.c
+++ b/arch/arm/mach-s3c24xx/mach-h1940.c
@@ -504,6 +504,7 @@  static struct platform_pwm_backlight_data backlight_data = {
 	.dft_brightness = 50,
 	/* tcnt = 0x31 */
 	.pwm_period_ns  = 36296,
+	.enable_gpio    = -1,
 	.init           = h1940_backlight_init,
 	.notify		= h1940_backlight_notify,
 	.exit           = h1940_backlight_exit,
diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c
index 206b1f7..034b7fe 100644
--- a/arch/arm/mach-s3c24xx/mach-rx1950.c
+++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
@@ -522,6 +522,7 @@  static struct platform_pwm_backlight_data rx1950_backlight_data = {
 	.max_brightness = 24,
 	.dft_brightness = 4,
 	.pwm_period_ns = 48000,
+	.enable_gpio = -1,
 	.init = rx1950_backlight_init,
 	.notify = rx1950_backlight_notify,
 	.exit = rx1950_backlight_exit,
diff --git a/arch/arm/mach-s3c64xx/mach-crag6410.c b/arch/arm/mach-s3c64xx/mach-crag6410.c
index 1a911df..b319bf9 100644
--- a/arch/arm/mach-s3c64xx/mach-crag6410.c
+++ b/arch/arm/mach-s3c64xx/mach-crag6410.c
@@ -114,6 +114,7 @@  static struct platform_pwm_backlight_data crag6410_backlight_data = {
 	.max_brightness	= 1000,
 	.dft_brightness	= 600,
 	.pwm_period_ns	= 100000,	/* about 1kHz */
+	.enable_gpio	= -1,
 };
 
 static struct platform_device crag6410_backlight_device = {
diff --git a/arch/arm/mach-s3c64xx/mach-hmt.c b/arch/arm/mach-s3c64xx/mach-hmt.c
index e806404..614a03a 100644
--- a/arch/arm/mach-s3c64xx/mach-hmt.c
+++ b/arch/arm/mach-s3c64xx/mach-hmt.c
@@ -114,6 +114,7 @@  static struct platform_pwm_backlight_data hmt_backlight_data = {
 	.max_brightness	= 100 * 256,
 	.dft_brightness	= 40 * 256,
 	.pwm_period_ns	= 1000000000 / (100 * 256 * 20),
+	.enable_gpio	= -1,
 	.init		= hmt_bl_init,
 	.notify		= hmt_bl_notify,
 	.exit		= hmt_bl_exit,
diff --git a/arch/arm/mach-s3c64xx/mach-smartq.c b/arch/arm/mach-s3c64xx/mach-smartq.c
index 0f47237..a6b338f 100644
--- a/arch/arm/mach-s3c64xx/mach-smartq.c
+++ b/arch/arm/mach-s3c64xx/mach-smartq.c
@@ -151,6 +151,7 @@  static struct platform_pwm_backlight_data smartq_backlight_data = {
 	.max_brightness	= 1000,
 	.dft_brightness	= 600,
 	.pwm_period_ns	= 1000000000 / (1000 * 20),
+	.enable_gpio	= -1,
 	.init		= smartq_bl_init,
 };
 
diff --git a/arch/arm/mach-s3c64xx/mach-smdk6410.c b/arch/arm/mach-s3c64xx/mach-smdk6410.c
index 2a7b32c..d5ea938 100644
--- a/arch/arm/mach-s3c64xx/mach-smdk6410.c
+++ b/arch/arm/mach-s3c64xx/mach-smdk6410.c
@@ -625,6 +625,7 @@  static struct samsung_bl_gpio_info smdk6410_bl_gpio_info = {
 
 static struct platform_pwm_backlight_data smdk6410_bl_data = {
 	.pwm_id = 1,
+	.enable_gpio = -1,
 };
 
 static struct s3c_hsotg_plat smdk6410_hsotg_pdata;
diff --git a/arch/arm/mach-s5p64x0/mach-smdk6440.c b/arch/arm/mach-s5p64x0/mach-smdk6440.c
index 0b00304..9efdcc0 100644
--- a/arch/arm/mach-s5p64x0/mach-smdk6440.c
+++ b/arch/arm/mach-s5p64x0/mach-smdk6440.c
@@ -223,6 +223,7 @@  static struct samsung_bl_gpio_info smdk6440_bl_gpio_info = {
 
 static struct platform_pwm_backlight_data smdk6440_bl_data = {
 	.pwm_id = 1,
+	.enable_gpio = -1,
 };
 
 static void __init smdk6440_map_io(void)
diff --git a/arch/arm/mach-s5p64x0/mach-smdk6450.c b/arch/arm/mach-s5p64x0/mach-smdk6450.c
index 5949296..c3cacc0 100644
--- a/arch/arm/mach-s5p64x0/mach-smdk6450.c
+++ b/arch/arm/mach-s5p64x0/mach-smdk6450.c
@@ -242,6 +242,7 @@  static struct samsung_bl_gpio_info smdk6450_bl_gpio_info = {
 
 static struct platform_pwm_backlight_data smdk6450_bl_data = {
 	.pwm_id = 1,
+	.enable_gpio = -1,
 };
 
 static void __init smdk6450_map_io(void)
diff --git a/arch/arm/mach-s5pc100/mach-smdkc100.c b/arch/arm/mach-s5pc100/mach-smdkc100.c
index 7c57a22..9e256b9 100644
--- a/arch/arm/mach-s5pc100/mach-smdkc100.c
+++ b/arch/arm/mach-s5pc100/mach-smdkc100.c
@@ -216,6 +216,7 @@  static struct samsung_bl_gpio_info smdkc100_bl_gpio_info = {
 
 static struct platform_pwm_backlight_data smdkc100_bl_data = {
 	.pwm_id = 0,
+	.enable_gpio = -1,
 };
 
 static void __init smdkc100_map_io(void)
diff --git a/arch/arm/mach-s5pv210/mach-smdkv210.c b/arch/arm/mach-s5pv210/mach-smdkv210.c
index 6d72bb99..f52cc15 100644
--- a/arch/arm/mach-s5pv210/mach-smdkv210.c
+++ b/arch/arm/mach-s5pv210/mach-smdkv210.c
@@ -279,6 +279,7 @@  static struct samsung_bl_gpio_info smdkv210_bl_gpio_info = {
 static struct platform_pwm_backlight_data smdkv210_bl_data = {
 	.pwm_id = 3,
 	.pwm_period_ns = 1000,
+	.enable_gpio = -1,
 };
 
 static void __init smdkv210_map_io(void)
diff --git a/arch/arm/plat-samsung/dev-backlight.c b/arch/arm/plat-samsung/dev-backlight.c
index d51f956..80e7450 100644
--- a/arch/arm/plat-samsung/dev-backlight.c
+++ b/arch/arm/plat-samsung/dev-backlight.c
@@ -70,6 +70,7 @@  static struct samsung_bl_drvdata samsung_dfl_bl_data __initdata = {
 		.max_brightness = 255,
 		.dft_brightness = 255,
 		.pwm_period_ns  = 78770,
+		.enable_gpio    = -1,
 		.init           = samsung_bl_init,
 		.exit           = samsung_bl_exit,
 	},
@@ -121,6 +122,10 @@  void __init samsung_bl_set(struct samsung_bl_gpio_info *gpio_info,
 		samsung_bl_data->lth_brightness = bl_data->lth_brightness;
 	if (bl_data->pwm_period_ns)
 		samsung_bl_data->pwm_period_ns = bl_data->pwm_period_ns;
+	if (bl_data->enable_gpio)
+		samsung_bl_data->enable_gpio = bl_data->enable_gpio;
+	if (bl_data->enable_gpio_flags)
+		samsung_bl_data->enable_gpio_flags = bl_data->enable_gpio_flags;
 	if (bl_data->init)
 		samsung_bl_data->init = bl_data->init;
 	if (bl_data->notify)