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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220728113109.7gf3b36mqjxlhcq3@maple.lan>
Date:   Thu, 28 Jul 2022 12:31:09 +0100
From:   Daniel Thompson <daniel.thompson@...aro.org>
To:     ChiaEn Wu <peterwu.pub@...il.com>
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 Wed, Jul 27, 2022 at 02:24:46PM +0800, ChiaEn Wu wrote:
> On Tue, Jul 26, 2022 at 7:59 PM Daniel Thompson
> <daniel.thompson@...aro.org> wrote:
> > > > > 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.

[snip]

> > > > 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.
> >
> > As above, ignoring the max_brightness property is a bug in the
> > userspace. I'm still unclear why sending faked ranges to userspace
> > it a better solution than fixing the userspace.
>
> Ok, I got it!
> If I add a second compatible string (like 'mediatek,mt6372-backlight')
> in the DT section,
> and append 'if-then-else' to determine the correct maximum value of
> 'default-brightness' and 'max-brightness',
> Also, I will append 'bled exponential mode' to make user control using
> linear or exponential mode.

I'd be very pleased to see support for exponential mode added: it's a
much better way to control LEDs and backlights.


> These changes I will explain to DT's maintainer again.

Excellent. I know DT maintainers are copied into this thread but they
will probably not be following this patch's thread so it is better to
discuss in the mail thread for the DT bindings!


> Back to the driver section,
> do I still need to use the register to confirm again whether this
> SubPMIC used now is MT6372 and record this information? (using
> 'mt6370_check_vendor_info()')
> I am afraid that the user who uses the MT6370 hardware, but the DT
> compatible string is set to 'mediatek,mt6372-backlight'.
> This may cause errors when update/get brightness values.
> So I hope the driver here can check again to make sure the
> 'default-brightness', 'max-brightness', can be updated/got correctly.
> I don't know if this will make you feel redundant if I do this??

Yes, it's good idea to check the hardware model during probe. I'd
suggest just reporting this as an error ("Buggy DT, wrong compatible
string") rather than trying to re-intepret the parameters.


Daniel.