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

[Daniel Thompson <daniel.thompson@linaro.org>]

On Tue, Jul 26, 2022 at 10:20:02AM +0800, ChiaEn Wu wrote:
> On Mon, Jul 25, 2022 at 6:31 PM Daniel Thompson
> <daniel.thompson@linaro.org> 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.

I don't think that is an acceptable approach for the upstream kernel.

To be "compatible" with (broken) software this driver ends up reducing
the capability of the upstream kernel to the point it becomes unable to
meet requirements for delicate adjustment (requirements that were
sufficiently important to change the hardware design so you could meet
them).


> > > +
> > > +       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?

Yes.


> Does that mean the DTS needs to be modified as well?

Yes... the property to set initial brightness needs a 14-bit range.

It would also be a good idea to discuss with the DT maintainers whether
you should introduce a second compatible string (ending 6372) in order
to allow the DT validation checks to detect accidental use of MT6372
ranges on MT6370 hardware.


> Or, for the reasons, I have just explained (just one customer has this
> requirement), then we do not make any changes for compatibility
> reasons?

I'd be curious what the compatiblity reasons are. In other words what
software breaks?

Normally the userspace backlight code reads the max_brightness property
and configures things accordingly (and therefore if you the component
that breaks is something like an Android HAL then fix the HAL instead).


Daniel.