diff mbox

pwm-backlight: Avoid backlight flicker when probed from DT

Message ID 1446133906-3076-1-git-send-email-p.zabel@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Philipp Zabel Oct. 29, 2015, 3:51 p.m. UTC 
If the driver is probed from the device tree, and there is a phandle
property set on it, and the enable GPIO is already configured as output,
and the backlight is currently disabled, keep it disabled.
If all these conditions are met, assume there will be some other driver
that can enable the backlight at the appropriate time.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
Changes since v2:
 - Leave GPIO configuration as is (GPIOD_ASIS) when requesting,
   so we can actually check the initial state.
---
 drivers/video/backlight/pwm_bl.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

Comments

Lee Jones Oct. 30, 2015, 6:21 p.m. UTC | #1 
On Thu, 29 Oct 2015, Philipp Zabel wrote:

> If the driver is probed from the device tree, and there is a phandle
> property set on it, and the enable GPIO is already configured as output,
> and the backlight is currently disabled, keep it disabled.
> If all these conditions are met, assume there will be some other driver
> that can enable the backlight at the appropriate time.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> ---
> Changes since v2:
>  - Leave GPIO configuration as is (GPIOD_ASIS) when requesting,
>    so we can actually check the initial state.
> ---
>  drivers/video/backlight/pwm_bl.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index ae3c6b6..8e9c261 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -199,6 +199,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  	struct backlight_properties props;
>  	struct backlight_device *bl;
>  	struct pwm_bl_data *pb;
> +	int initial_blank = FB_BLANK_UNBLANK;
> +	bool phandle;
>  	int ret;
>  
>  	if (!data) {
> @@ -242,7 +244,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  	pb->enabled = false;
>  
>  	pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> -						  GPIOD_OUT_HIGH);
> +						  GPIOD_ASIS);
>  	if (IS_ERR(pb->enable_gpio)) {
>  		ret = PTR_ERR(pb->enable_gpio);
>  		goto err_alloc;
> @@ -264,12 +266,32 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  		pb->enable_gpio = gpio_to_desc(data->enable_gpio);
>  	}
>  
> +	phandle = of_find_property(pdev->dev.of_node, "phandle", NULL) != NULL;

This is a little ugly.

If this is the only way to identify the probedness of a device then
you probably want to neaten it up a little.  I suggest something along
the lines of:

if (of_find_property(pdev->dev.of_node, "phandle", NULL))
	already_probed = true;

Also, do you also want to check for "linux,phandle", as this is always
catered for in drivers/of/*

> +	if (pb->enable_gpio) {
> +		/*
> +		 * If the driver is probed from the device tree and there is a
> +		 * phandle link pointing to the backlight node, it is safe to
> +		 * assume that another driver will enable the backlight at the
> +		 * appropriate time. Therefore, if it is disabled, keep it so.
> +		 */
> +		if (phandle &&
> +		    gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_OUT &&
> +		    gpiod_get_value(pb->enable_gpio) == 0)
> +			initial_blank = FB_BLANK_POWERDOWN;
> +		else
> +			gpiod_direction_output(pb->enable_gpio, 1);
> +	}
> +
>  	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
>  	if (IS_ERR(pb->power_supply)) {
>  		ret = PTR_ERR(pb->power_supply);
>  		goto err_alloc;
>  	}
>  
> +	if (phandle && !regulator_is_enabled(pb->power_supply))
> +		initial_blank = FB_BLANK_POWERDOWN;
> +
>  	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
>  	if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER
>  	    && !pdev->dev.of_node) {
> @@ -320,6 +342,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  	}
>  
>  	bl->props.brightness = data->dft_brightness;
> +	bl->props.power = initial_blank;
>  	backlight_update_status(bl);
>  
>  	platform_set_drvdata(pdev, bl);
Philipp Zabel Nov. 2, 2015, 4:30 p.m. UTC | #2 
Hi Lee,

Am Freitag, den 30.10.2015, 18:21 +0000 schrieb Lee Jones:
> > @@ -264,12 +266,32 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> >  		pb->enable_gpio = gpio_to_desc(data->enable_gpio);
> >  	}
> >  
> > +	phandle = of_find_property(pdev->dev.of_node, "phandle", NULL) != NULL;
> 
> This is a little ugly.
> 
> If this is the only way to identify the probedness of a device then
> you probably want to neaten it up a little.  I suggest something along
> the lines of:
>
> if (of_find_property(pdev->dev.of_node, "phandle", NULL))
> 	already_probed = true;
> 
> Also, do you also want to check for "linux,phandle", as this is always
> catered for in drivers/of/*

Thanks for pointing this out. I suppose I should just check
pdev->dev.of_node->phandle != 0 instead. That will already be
initialized by unflatten_dt_node if either property was found.

regards
Philipp

--
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/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index ae3c6b6..8e9c261 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -199,6 +199,8 @@  static int pwm_backlight_probe(struct platform_device *pdev)
 	struct backlight_properties props;
 	struct backlight_device *bl;
 	struct pwm_bl_data *pb;
+	int initial_blank = FB_BLANK_UNBLANK;
+	bool phandle;
 	int ret;
 
 	if (!data) {
@@ -242,7 +244,7 @@  static int pwm_backlight_probe(struct platform_device *pdev)
 	pb->enabled = false;
 
 	pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
-						  GPIOD_OUT_HIGH);
+						  GPIOD_ASIS);
 	if (IS_ERR(pb->enable_gpio)) {
 		ret = PTR_ERR(pb->enable_gpio);
 		goto err_alloc;
@@ -264,12 +266,32 @@  static int pwm_backlight_probe(struct platform_device *pdev)
 		pb->enable_gpio = gpio_to_desc(data->enable_gpio);
 	}
 
+	phandle = of_find_property(pdev->dev.of_node, "phandle", NULL) != NULL;
+
+	if (pb->enable_gpio) {
+		/*
+		 * If the driver is probed from the device tree and there is a
+		 * phandle link pointing to the backlight node, it is safe to
+		 * assume that another driver will enable the backlight at the
+		 * appropriate time. Therefore, if it is disabled, keep it so.
+		 */
+		if (phandle &&
+		    gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_OUT &&
+		    gpiod_get_value(pb->enable_gpio) == 0)
+			initial_blank = FB_BLANK_POWERDOWN;
+		else
+			gpiod_direction_output(pb->enable_gpio, 1);
+	}
+
 	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
 	if (IS_ERR(pb->power_supply)) {
 		ret = PTR_ERR(pb->power_supply);
 		goto err_alloc;
 	}
 
+	if (phandle && !regulator_is_enabled(pb->power_supply))
+		initial_blank = FB_BLANK_POWERDOWN;
+
 	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
 	if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER
 	    && !pdev->dev.of_node) {
@@ -320,6 +342,7 @@  static int pwm_backlight_probe(struct platform_device *pdev)
 	}
 
 	bl->props.brightness = data->dft_brightness;
+	bl->props.power = initial_blank;
 	backlight_update_status(bl);
 
 	platform_set_drvdata(pdev, bl);