HI Daniel,
> -----Original Message-----
> From: Daniel Thompson <daniel.thomp...@linaro.org>
> Sent: Tuesday, August 29, 2023 6:28 PM
> To: Flavio Suligoi <f.suli...@asem.it>
> Cc: Lee Jones <l...@kernel.org>; Jingoo Han <jingooh...@gmail.com>; Helge
> Deller <del...@gmx.de>; Pavel Machek <pa...@ucw.cz>; Rob Herring
> <robh...@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski...@linaro.org>; Conor Dooley <conor...@kernel.org>;
> dri-devel@lists.freedesktop.org; linux-l...@vger.kernel.org;
> devicet...@vger.kernel.org; linux-fb...@vger.kernel.org; linux-
> ker...@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:
> > 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.suli...@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?
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
