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
