Re: [PATCH v3 1/2] backlight: pwm_bl: Move the checks for initial power state to a separate function

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

On 11/21/2016 10:10 AM, Thierry Reding wrote:
> On Tue, Nov 01, 2016 at 02:59:32PM +0200, Peter Ujfalusi wrote:
>> Move the checks to select the initial state for the backlight to a new
>> function and document the checks we are doing.
>>
>> With the separate function it is going to be easier to fix or improve the
>> initial power state configuration later and it is easier to read the code.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/video/backlight/pwm_bl.c | 53 ++++++++++++++++++++++++++--------------
>>  1 file changed, 34 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>> index 12614006211e..4b07da278b4f 100644
>> --- a/drivers/video/backlight/pwm_bl.c
>> +++ b/drivers/video/backlight/pwm_bl.c
>> @@ -192,6 +192,32 @@ static int pwm_backlight_parse_dt(struct device *dev,
>>  }
>>  #endif
>>  
>> +static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
>> +{
>> +	struct device_node *node = pb->dev->of_node;
>> +
>> +	/* Not booted with device tree or no phandle link to the node */
>> +	if (!node || !node->phandle)
>> +		return FB_BLANK_UNBLANK;
>> +
>> +	/*
>> +	 * 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 the enable GPIO is disabled, do not enable the backlight */
>> +	if (pb->enable_gpio && gpiod_get_value(pb->enable_gpio) == 0)
>> +		return FB_BLANK_POWERDOWN;
>> +
>> +	/* The regulator is disabled, do not enable the backlight */
>> +	if (!regulator_is_enabled(pb->power_supply))
>> +		return FB_BLANK_POWERDOWN;
>> +
>> +	return FB_BLANK_UNBLANK;
>> +}
>> +
>>  static int pwm_backlight_probe(struct platform_device *pdev)
>>  {
>>  	struct platform_pwm_backlight_data *data = dev_get_platdata(&pdev->dev);
>> @@ -200,7 +226,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>>  	struct backlight_device *bl;
>>  	struct device_node *node = pdev->dev.of_node;
>>  	struct pwm_bl_data *pb;
>> -	int initial_blank = FB_BLANK_UNBLANK;
>>  	struct pwm_args pargs;
>>  	int ret;
>>  
>> @@ -267,20 +292,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>>  		pb->enable_gpio = gpio_to_desc(data->enable_gpio);
>>  	}
>>  
>> -	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 (node && node->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);
>> -	}
>> +	/*
>> +	 * If the GPIO is configured as input, change the direction to output
>> +	 * and set the GPIO as active.
>> +	 */
>> +	if (pb->enable_gpio &&
>> +	    gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_IN)
>> +		gpiod_direction_output(pb->enable_gpio, 1);
> 
> I'm confused about this. Isn't it redundant to check for the direction
> if you're going to configure it as output either way? Why not just set
> it as output unconditionally?
> 
> Also, is this not counterproductive? pwm_backlight_initial_power_state()
> will check the value of the GPIO to determine whether or not to mark the
> backlight as enabled. If we're setting this to active, then the check in
> the initial state retrieval will only be false if the GPIO is an output
> and inactive.
> 
> Oh wait... I guess that's exactly why you're doing this. =) Perhaps this
> could be made somewhat clearer by beefing up the comment. As it is, the
> comment /seems/ rather useless because it restates what the code does. I
> think it'd be better to explain more verbosely what's going on, to avoid
> confusing people like me.

Extending the commit to:
/*
 * If the GPIO is configured as input, change the direction to output
 * and set the GPIO as active.
 * Do not force the GPIO to active when it was already output as it
 * could cause backlight flickering or we would enable the backlight too
 * early. Leave the decision of the initial backlight state for later.
 */

> Either way, though, the patch looks correct now that I understand it
> properly, I'll leave it up to Lee if he wants to insist on a clarifying
> comment.
> 
> Reviewed-by: Thierry Reding <treding@nvidia.com>
> 

-- 
Péter