Re: [PATCH v6 13/13] video: backlight: mt6370: Add MediaTek MT6370 support

[ChiaEn Wu]

On Mon, Jul 25, 2022 at 6:31 PM Daniel Thompson
<daniel.thompson@xxxxxxxxxx> wrote:
>
> On Fri, Jul 22, 2022 at 06:24:07PM +0800, ChiaEn Wu wrote:
> > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> > index a003e02..846dbe7 100644
> > --- a/drivers/video/backlight/Kconfig
> > +++ b/drivers/video/backlight/Kconfig
> > @@ -268,6 +268,18 @@ config BACKLIGHT_MAX8925
> >         If you have a LCD backlight connected to the WLED output of MAX8925
> >         WLED output, say Y here to enable this driver.
> >
> > +config BACKLIGHT_MT6370
> > +     tristate "MediaTek MT6370 Backlight Driver"
> > +     depends on MFD_MT6370
> > +     help
> > +       This enables support for Mediatek MT6370 Backlight driver.
> > +       It's commonly used to drive the display WLED. There are 4 channels
> > +       inside, and each channel supports up to 30mA of current capability
> > +       with 2048 current steps in exponential or linear mapping curves.
>
> Does the MT6372 support more steps than this? In other words does it use
> a fourteen bit scale or does it use an 11-bit scale at a different
> register location?

Hi Daniel,

Thanks for your reply.
Yes, MT6372 can support 16384 steps and uses a 14-bit scale register
location. But the maximum current of each
channel of MT6372 is the same as MT6370 and MT6371, both 30mA.
The main reason why MT6372 is designed this way is that one of the
customers asked for a more delicate
adjustment of the backlight brightness. But other customers actually
do not have such requirements.
Therefore, we designed it this way for maximum compatibility in software.

>
>
> > +
> > +       This driver can also be built as a module. If so, the module
> > +       will be called "mt6370-backlight".
> > +
> > [...]
> > diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c
> > new file mode 100644
> > index 0000000..ba00a8f
> > --- /dev/null
> > +++ b/drivers/video/backlight/mt6370-backlight.c
> > [...]
> > +static int mt6370_bl_update_status(struct backlight_device *bl_dev)
> > +{
> > +     struct mt6370_priv *priv = bl_get_data(bl_dev);
> > +     int brightness = backlight_get_brightness(bl_dev);
> > +     unsigned int enable_val;
> > +     u8 brightness_val[2];
> > +     int ret;
> > +
> > +     if (brightness) {
> > +             brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK;
> > +             brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK);
> > +
> > +             /*
> > +              * To make MT6372 using 14 bits to control the brightness
> > +              * backward compatible with 11 bits brightness control
> > +              * (like MT6370 and MT6371 do), we left shift the value
> > +              * and pad with 1 to remaining bits. Hence, the MT6372's
> > +              * backlight brightness will be almost the same as MT6370's
> > +              * and MT6371's.
> > +              */
> > +             if (priv->vid_type == MT6370_VID_6372) {
> > +                     brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT;
> > +                     brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK;
> > +             }
>
> This somewhat depends on the answer to the first question above, but
> what is the point of this shifting? If the range is 14-bit then the
> driver should set max_brightness to 16384 and present the full range of
> the MT6372 to the user.

So should we make all 16384 steps of MT6372 available to users?
Does that mean the DTS needs to be modified as well?
Or, for the reasons, I have just explained (just one customer has this
requirement), then we do not make any changes for compatibility
reasons?
Thanks.

>
> Especially when using linear mappings (which are a totally pointless
> scale to use for a backlight) the extra steps are useful for backlight
> animation.
>
>
> Daniel.

-- 
Best Regards,
ChiaEn Wu