HI Daniel,

-----Original Message-----
From: Daniel Thompson <redacted>
Sent: Tuesday, August 29, 2023 6:28 PM
To: Flavio Suligoi <f.suligoi@asem.it>
Cc: Lee Jones <lee@kernel.org>; Jingoo Han <jingoohan1@gmail.com>; Helge
Deller [off-list ref]; Pavel Machek [off-list ref]; Rob Herring
[off-list ref]; Krzysztof Kozlowski
[off-list ref]; Conor Dooley [off-list ref];
dri-devel@lists.freedesktop.org; linux-leds@vger.kernel.org;
devicetree@vger.kernel.org; linux-fbdev@vger.kernel.org; linux-
kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/2] backlight: mp3309c: Add support for MPS
MP3309C

On Tue, Aug 29, 2023 at 12:15:46PM +0200, Flavio Suligoi wrote:

quoted

The Monolithic Power (MPS) MP3309C is a WLED step-up converter,
featuring a programmable switching frequency to optimize efficiency.
The brightness can be controlled either by I2C commands (called "analog"
mode) or by a PWM input signal (PWM mode).
This driver supports both modes.

For DT configuration details, please refer to:
- Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml

The datasheet is available at:
- https://www.monolithicpower.com/en/mp3309c.html

Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>

quoted

diff --git a/drivers/video/backlight/Kconfig

b/drivers/video/backlight/Kconfig index 51387b1ef012..65d0ac9f611d
100644

--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig

@@ -389,6 +389,19 @@ config BACKLIGHT_LM3639
 	help
 	  This supports TI LM3639 Backlight + 1.5A Flash LED Driver

+config BACKLIGHT_MP3309C
+	tristate "Backlight Driver for MPS MP3309C"
+	depends on I2C
+	select REGMAP_I2C
+	select NEW_LEDS
+	select LEDS_CLASS

This doesn't seem right.

Shouldn't PWM and GPIOLIB be listed here? Why are NEW_LEDS and
LEDS_CLASS needed?

ok, I'll fix it

quoted

+	help
+	  This supports MPS MP3309C backlight WLED Driver in both PWM

and

quoted

+	  analog/I2C dimming modes.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called mp3309c_bl.
+
 config BACKLIGHT_LP855X
 	tristate "Backlight driver for TI LP855X"
 	depends on I2C && PWM

quoted

+static int mp3309c_bl_update_status(struct backlight_device *bl) {
+	struct mp3309c_chip *chip = bl_get_data(bl);
+	int brightness = backlight_get_brightness(bl);
+	struct pwm_state pwmstate;
+	unsigned int analog_val, bits_val;
+	int i, ret;
+
+	if (chip->pdata->dimming_mode == DIMMING_PWM) {
+		/*
+		 * PWM dimming mode
+		 */
+		pwm_init_state(chip->pwmd, &pwmstate);
+		pwm_set_relative_duty_cycle(&pwmstate, brightness,
+					    chip->pdata->max_brightness);
+		pwmstate.enabled = true;
+		ret = pwm_apply_state(chip->pwmd, &pwmstate);
+		if (ret)
+			return ret;
+	} else {
+		/*
+		 * Analog dimming mode by I2C commands
+		 *
+		 * The 5 bits of the dimming analog value D4..D0 is allocated
+		 * in the I2C register #0, in the following way:
+		 *
+		 *     +--+--+--+--+--+--+--+--+
+		 *     |EN|D0|D1|D2|D3|D4|XX|XX|
+		 *     +--+--+--+--+--+--+--+--+
+		 */
+		analog_val = DIV_ROUND_UP(ANALOG_MAX_VAL *

brightness,

quoted

+					  chip->pdata->max_brightness);
+		bits_val = 0;
+		for (i = 0; i <= 5; i++)
+			bits_val += ((analog_val >> i) & 0x01) << (6 - i);
+		ret = regmap_update_bits(chip->regmap, REG_I2C_0,
+					 ANALOG_REG_MASK, bits_val);
+		if (ret)
+			return ret;
+	}
+
+	if (brightness > 0) {
+		switch (chip->pdata->status) {
+		case FIRST_POWER_ON:
+			/*
+			 * Only for the first time, wait for the optional
+			 * switch-on delay and then enable the device.
+			 * Otherwise enable the backlight immediately.
+			 */
+			schedule_delayed_work(&chip->enable_work,
+					      msecs_to_jiffies(chip->pdata-
switch_on_delay_ms));

Delaying this work makes no sense to me, especially when it is only going to
happen at initial power on.

If you are (ab)using this property to try and sequence the backlight power-on
with display initialization then this is not the way to do it.
Normally backlight drivers that support sequencing versus the panel work by
having a backlight property set on the panel linking it to the backlight. When
the panel is ready this power status of the backlight will be updated
accordingly.

All the backlight driver need do is make sure that is the initial power status is
"powerdown" on systems when the link is present (e.g.
leave the backlight off and wait to be told the display has settled).

OK, I'll remove this delay. It was used for one of our boards, as a workaround.

quoted

+			/*
+			 * Optional external device GPIO reset, with
+			 * delay pulse length
+			 */
+			if (chip->pdata->reset_pulse_enable)
+				schedule_delayed_work(&chip-
reset_gpio_work,
+						      msecs_to_jiffies(chip-
pdata->reset_on_delay_ms));

Similarly I don't understand what this property is for. A backlight is directly
perceivable by the user. There is nothing downstream of a light that needs to
be reset!

What is this used for?

Also this property, this gpio, was used for one of our boards.
It is not necessary, I'll remove it.

Daniel.

Thanks, Daniel!
Flavio