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

[Flavio Suligoi]

HI Daniel,

> -----Original Message-----
> From: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> Sent: Tuesday, August 29, 2023 6:28 PM
> To: Flavio Suligoi <f.suligoi@xxxxxxx>
> Cc: Lee Jones <lee@xxxxxxxxxx>; Jingoo Han <jingoohan1@xxxxxxxxx>; Helge
> Deller <deller@xxxxxx>; Pavel Machek <pavel@xxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>;
> dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-leds@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-fbdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> 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:
> > 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@xxxxxxx>
> 
> > 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

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

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

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