lkml.org 
[lkml]   [2023]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v1 2/2] backlight: mp3309c: Add support for MPS MP3309C
    Date
    HI Daniel,

    > -----Original Message-----
    > From: Daniel Thompson <daniel.thompson@linaro.org>
    > 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 <deller@gmx.de>; Pavel Machek <pavel@ucw.cz>; Rob Herring
    > <robh+dt@kernel.org>; Krzysztof Kozlowski
    > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
    > 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:
    > > 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?

    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

    \
     
     \ /
      Last update: 2023-08-30 21:16    [W:6.197 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site