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

[Daniel Thompson]

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