Re: [PATCH v1 2/2] backlight: mp3309c: Add support for MPS MP3309C

[Daniel Thompson <daniel.thompson@linaro.org>]

On Tue, Aug 29, 2023 at 12:15:46PM +0200, Flavio Suligoi wrote:
> 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>

> 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?

> +	help
> +	  This supports MPS MP3309C backlight WLED Driver in both PWM and
> +	  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

> +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,
> +					  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).


> +			/*
> +			 * 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?


Daniel.