On Tue, Jul 04, 2023 at 07:07:31PM +0200, Sam Ravnborg wrote:

Hi Daniel,

quoted

quoted

@@ -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;

The following code does the same using helpers:

	if (active_at_boot || !dev->node->phandle))
		backlight_enable(bd);
	else
		backlight_disable(bd);

The code needs to execute after backlight_device_register() so maybe not
so great an idea?!?

It would introduce a need for a bunch of new locks since userspace
could get in between device creation and the call to the helpers.

Additionally, it is even correct for a driver to forcefully set
fb_blank to powerdown during the probe? All current drivers set
fb_blank to unblank during the probe.


Daniel.