Message-ID: <CABtFH5+in-+=6r3wOvQ8-78DT9CXaMursJukhx+kdwMvvP3djw@mail.gmail.com>
Date:   Tue, 26 Jul 2022 19:28:48 +0800
From:   ChiaEn Wu <peterwu.pub@...il.com>
To:     Daniel Thompson <daniel.thompson@...aro.org>
Cc:     Lee Jones <lee.jones@...aro.org>,
        Jingoo Han <jingoohan1@...il.com>, Pavel Machek <pavel@....cz>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Sebastian Reichel <sre@...nel.org>,
        Chunfeng Yun <chunfeng.yun@...iatek.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Guenter Roeck <linux@...ck-us.net>,
        "Krogerus, Heikki" <heikki.krogerus@...ux.intel.com>,
        Helge Deller <deller@....de>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        ChiaEn Wu <chiaen_wu@...htek.com>,
        Alice Chen <alice_chen@...htek.com>,
        ChiYuan Huang <cy_huang@...htek.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        USB <linux-usb@...r.kernel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        "open list:FRAMEBUFFER LAYER" <linux-fbdev@...r.kernel.org>,
        szuni chen <szunichen@...il.com>
Subject: Re: [PATCH v6 13/13] video: backlight: mt6370: Add MediaTek MT6370 support

On Tue, Jul 26, 2022 at 5:31 PM Daniel Thompson
<daniel.thompson@...aro.org> wrote:
...
> > > 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.

Sorry for I used of the wrong word, I mean is 'driver', not
higher-level 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).

Originally, we just wanted to use one version of the driver to cover
all the SubPMIC of the 6370 series(6370~6372).
And, the users who use this series SubPMIC can directly apply this
driver to drive the backlight device without knowing the underlying
hardware.
To achieve this goal, we have designed it to look like this.

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

hmmm... I have just thought about it,
maybe I can just modify the maximum value of default-brightness and
max-brightness in DT to 16384,
modify the description and add some comments.

And then on the driver side,
we can use mt6370_check_vendor_info( ) to determine whether it is MT6372.
If no, then in mt6370_bl_update_status(), first brightness_val / 8 and then set.
In mt6370_bl_get_brightness(), first brightness_val * 8 and then return;

If I do this change, does this meet your requirements?

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

The reason is as above. We just hope the users who use this series SubPMIC can
directly apply this driver to drive the backlight device without
knowing the underlying hardware.
Not software breaks.

Thanks!

>
> 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.

-- 
Best Regards,
ChiaEn Wu

Powered by blists - more mailing lists