diff mbox

[9/9] backlight: atmel-pwm-bl: use gpio_request_one

Message ID 1382522143-32072-10-git-send-email-jhovold@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Johan Hovold Oct. 23, 2013, 9:55 a.m. UTC
Use devm_gpio_request_one rather than requesting and setting direction
in two calls.

Acked-by: Jingoo Han <jg1.han@samsung.com>:w
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/video/backlight/atmel-pwm-bl.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Jean-Christophe PLAGNIOL-VILLARD Oct. 25, 2013, 11:15 a.m. UTC | #1
On 11:55 Wed 23 Oct     , Johan Hovold wrote:
> Use devm_gpio_request_one rather than requesting and setting direction
> in two calls.

this is the same I do not see any advantage

and as I said for ather backligth It's wrong to enable or disable it at probe
as the bootloader might have already enable it for splash screen

Best Regards,
J.
> 
> Acked-by: Jingoo Han <jg1.han@samsung.com>:w
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
>  drivers/video/backlight/atmel-pwm-bl.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> index 1277e0c..5ea2517 100644
> --- a/drivers/video/backlight/atmel-pwm-bl.c
> +++ b/drivers/video/backlight/atmel-pwm-bl.c
> @@ -124,6 +124,7 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
>  	const struct atmel_pwm_bl_platform_data *pdata;
>  	struct backlight_device *bldev;
>  	struct atmel_pwm_bl *pwmbl;
> +	int flags;
>  	int retval;
>  
>  	pdata = dev_get_platdata(&pdev->dev);
> @@ -149,14 +150,14 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
>  		return retval;
>  
>  	if (gpio_is_valid(pwmbl->gpio_on)) {
> -		retval = devm_gpio_request(&pdev->dev, pwmbl->gpio_on,
> -					"gpio_atmel_pwm_bl");
> -		if (retval)
> -			goto err_free_pwm;
> -
>  		/* Turn display off by default. */
> -		retval = gpio_direction_output(pwmbl->gpio_on,
> -				0 ^ pdata->on_active_low);
> +		if (pdata->on_active_low)
> +			flags = GPIOF_OUT_INIT_HIGH;
> +		else
> +			flags = GPIOF_OUT_INIT_LOW;
> +
> +		retval = devm_gpio_request_one(&pdev->dev, pwmbl->gpio_on,
> +						flags, "gpio_atmel_pwm_bl");
>  		if (retval)
>  			goto err_free_pwm;
>  	}
> -- 
> 1.8.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johan Hovold Oct. 29, 2013, 1:20 p.m. UTC | #2
On Fri, Oct 25, 2013 at 01:15:43PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 11:55 Wed 23 Oct     , Johan Hovold wrote:
> > Use devm_gpio_request_one rather than requesting and setting direction
> > in two calls.
> 
> this is the same I do not see any advantage

It removes one error path.

> and as I said for ather backligth It's wrong to enable or disable it at probe
> as the bootloader might have already enable it for splash screen

I agree with you on that, and it's actually the reason for the below
clean up. I use a second patch:

        if (gpio_is_valid(pwmbl->gpio_on)) {
-               /* Turn display off by default. */
-               if (pdata->on_active_low)
+               /* Turn display off unless already enabled. */
+               if (pdata->gpio_on_enabled ^ pdata->on_active_low)
                        flags = GPIOF_OUT_INIT_HIGH;
                else
                        flags = GPIOF_OUT_INIT_LOW;

to make sure the driver does not temporarily disable the backlight at
probe.

Decided not to submit it as part of this series when I realised that
several other backlight drivers did the same, but perhaps I should?

Thanks,
Johan

> > 
> > Acked-by: Jingoo Han <jg1.han@samsung.com>:w
> > Signed-off-by: Johan Hovold <jhovold@gmail.com>
> > ---
> >  drivers/video/backlight/atmel-pwm-bl.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> > index 1277e0c..5ea2517 100644
> > --- a/drivers/video/backlight/atmel-pwm-bl.c
> > +++ b/drivers/video/backlight/atmel-pwm-bl.c
> > @@ -124,6 +124,7 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
> >  	const struct atmel_pwm_bl_platform_data *pdata;
> >  	struct backlight_device *bldev;
> >  	struct atmel_pwm_bl *pwmbl;
> > +	int flags;
> >  	int retval;
> >  
> >  	pdata = dev_get_platdata(&pdev->dev);
> > @@ -149,14 +150,14 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
> >  		return retval;
> >  
> >  	if (gpio_is_valid(pwmbl->gpio_on)) {
> > -		retval = devm_gpio_request(&pdev->dev, pwmbl->gpio_on,
> > -					"gpio_atmel_pwm_bl");
> > -		if (retval)
> > -			goto err_free_pwm;
> > -
> >  		/* Turn display off by default. */
> > -		retval = gpio_direction_output(pwmbl->gpio_on,
> > -				0 ^ pdata->on_active_low);
> > +		if (pdata->on_active_low)
> > +			flags = GPIOF_OUT_INIT_HIGH;
> > +		else
> > +			flags = GPIOF_OUT_INIT_LOW;
> > +
> > +		retval = devm_gpio_request_one(&pdev->dev, pwmbl->gpio_on,
> > +						flags, "gpio_atmel_pwm_bl");
> >  		if (retval)
> >  			goto err_free_pwm;
> >  	}
> > -- 
> > 1.8.4
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean-Christophe PLAGNIOL-VILLARD Oct. 30, 2013, 7:30 p.m. UTC | #3
On 14:20 Tue 29 Oct     , Johan Hovold wrote:
> On Fri, Oct 25, 2013 at 01:15:43PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 11:55 Wed 23 Oct     , Johan Hovold wrote:
> > > Use devm_gpio_request_one rather than requesting and setting direction
> > > in two calls.
> > 
> > this is the same I do not see any advantage
> 
> It removes one error path.
> 
> > and as I said for ather backligth It's wrong to enable or disable it at probe
> > as the bootloader might have already enable it for splash screen
> 
> I agree with you on that, and it's actually the reason for the below
> clean up. I use a second patch:
> 
>         if (gpio_is_valid(pwmbl->gpio_on)) {
> -               /* Turn display off by default. */
> -               if (pdata->on_active_low)
> +               /* Turn display off unless already enabled. */
> +               if (pdata->gpio_on_enabled ^ pdata->on_active_low)
>                         flags = GPIOF_OUT_INIT_HIGH;
>                 else
>                         flags = GPIOF_OUT_INIT_LOW;
> 
> to make sure the driver does not temporarily disable the backlight at
> probe.
> 
> Decided not to submit it as part of this series when I realised that
> several other backlight drivers did the same, but perhaps I should?

I'm not happy with this idea to play with enable at probe time

we need to detect the current status if possible and only enable or disable
when requiered

Best Regards,
J.
> 
> Thanks,
> Johan
> 
> > > 
> > > Acked-by: Jingoo Han <jg1.han@samsung.com>:w
> > > Signed-off-by: Johan Hovold <jhovold@gmail.com>
> > > ---
> > >  drivers/video/backlight/atmel-pwm-bl.c | 15 ++++++++-------
> > >  1 file changed, 8 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> > > index 1277e0c..5ea2517 100644
> > > --- a/drivers/video/backlight/atmel-pwm-bl.c
> > > +++ b/drivers/video/backlight/atmel-pwm-bl.c
> > > @@ -124,6 +124,7 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
> > >  	const struct atmel_pwm_bl_platform_data *pdata;
> > >  	struct backlight_device *bldev;
> > >  	struct atmel_pwm_bl *pwmbl;
> > > +	int flags;
> > >  	int retval;
> > >  
> > >  	pdata = dev_get_platdata(&pdev->dev);
> > > @@ -149,14 +150,14 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
> > >  		return retval;
> > >  
> > >  	if (gpio_is_valid(pwmbl->gpio_on)) {
> > > -		retval = devm_gpio_request(&pdev->dev, pwmbl->gpio_on,
> > > -					"gpio_atmel_pwm_bl");
> > > -		if (retval)
> > > -			goto err_free_pwm;
> > > -
> > >  		/* Turn display off by default. */
> > > -		retval = gpio_direction_output(pwmbl->gpio_on,
> > > -				0 ^ pdata->on_active_low);
> > > +		if (pdata->on_active_low)
> > > +			flags = GPIOF_OUT_INIT_HIGH;
> > > +		else
> > > +			flags = GPIOF_OUT_INIT_LOW;
> > > +
> > > +		retval = devm_gpio_request_one(&pdev->dev, pwmbl->gpio_on,
> > > +						flags, "gpio_atmel_pwm_bl");
> > >  		if (retval)
> > >  			goto err_free_pwm;
> > >  	}
> > > -- 
> > > 1.8.4
> > > 
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
index 1277e0c..5ea2517 100644
--- a/drivers/video/backlight/atmel-pwm-bl.c
+++ b/drivers/video/backlight/atmel-pwm-bl.c
@@ -124,6 +124,7 @@  static int atmel_pwm_bl_probe(struct platform_device *pdev)
 	const struct atmel_pwm_bl_platform_data *pdata;
 	struct backlight_device *bldev;
 	struct atmel_pwm_bl *pwmbl;
+	int flags;
 	int retval;
 
 	pdata = dev_get_platdata(&pdev->dev);
@@ -149,14 +150,14 @@  static int atmel_pwm_bl_probe(struct platform_device *pdev)
 		return retval;
 
 	if (gpio_is_valid(pwmbl->gpio_on)) {
-		retval = devm_gpio_request(&pdev->dev, pwmbl->gpio_on,
-					"gpio_atmel_pwm_bl");
-		if (retval)
-			goto err_free_pwm;
-
 		/* Turn display off by default. */
-		retval = gpio_direction_output(pwmbl->gpio_on,
-				0 ^ pdata->on_active_low);
+		if (pdata->on_active_low)
+			flags = GPIOF_OUT_INIT_HIGH;
+		else
+			flags = GPIOF_OUT_INIT_LOW;
+
+		retval = devm_gpio_request_one(&pdev->dev, pwmbl->gpio_on,
+						flags, "gpio_atmel_pwm_bl");
 		if (retval)
 			goto err_free_pwm;
 	}