Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 4 Jul 2023 16:03:10 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Mans Rullgard <mans@mansr.com>
Cc:     Lee Jones <lee@kernel.org>, Jingoo Han <jingoohan1@gmail.com>,
        Helge Deller <deller@gmx.de>,
        Tomi Valkeinen <tomi.valkeinen@ti.com>,
        dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] backlight: led_bl: fix initial power state
Message-ID: <20230704150310.GA385243@aspen.lan>
References: <20230704140750.25799-1-mans@mansr.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230704140750.25799-1-mans@mansr.com>
Precedence: bulk

On Tue, Jul 04, 2023 at 03:07:50PM +0100, Mans Rullgard wrote:
> The condition for the initial power state based on the default
> brightness value is reversed.  Fix it.
>
> Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
>  drivers/video/backlight/led_bl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index 3259292fda76..28e83618a296 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -200,8 +200,8 @@ static int led_bl_probe(struct platform_device *pdev)
>  	props.type = BACKLIGHT_RAW;
>  	props.max_brightness = priv->max_brightness;
>  	props.brightness = priv->default_brightness;
> -	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
> -		      FB_BLANK_UNBLANK;
> +	props.power = (priv->default_brightness > 0) ? FB_BLANK_UNBLANK :
> +		      FB_BLANK_POWERDOWN;

The logic was wrong before but I think will still be wrong after the
change too (e.g. the bogus logic is probably avoiding backlight flicker
in some use cases).

The logic here needs to be similar to what pwm_bl.c implements in
pwm_backlight_initial_power_state(). Whilst it might be better
to implement this in led_bl_get_leds() let me show what I mean
in code that fits in the current line:

	/*
	 * Activate the backlight if the LEDs are already lit *or*
	 * there is no phandle link (meaning the backlight power
	 * state cannot be synced with the display state).
	 */
	props.power = (active_at_boot || !dev->node->phandle) ?
			FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;

Note that active_at_boot is not the same as (priv->default_brightness > 0)
since the value read by led_bl_get_leds() can be clobbered when we
parse the properties.


Daniel.