| Message ID | 1382522143-32072-10-git-send-email-jhovold@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 --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; }