Re: [PATCH 2/2] backlight: rt4831: Add the property parsing for ocp level selection

[Daniel Thompson]

On Fri, May 27, 2022 at 10:24:42AM +0800, ChiYuan Huang wrote:
> Daniel Thompson <daniel.thompson@xxxxxxxxxx> 於 2022年5月26日 週四 下午6:05寫道：
> >
> > On Thu, May 26, 2022 at 11:16:35AM +0800, cy_huang wrote:
> > > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > >
> > > Add the property parsing for ocp level selection.
> >
> > Isn't this just restating the Subject: line?
> >
> Ah, that's my fault. I didn't state too much in the patch comment.
> I only left it in the cover letter.
> 
> > It would be better to provide information useful to the reviewer here.
> > Something like:
> >
> > "Currently this driver simply inherits whatever over-current protection
> > level is programmed into the hardware when it is handed over. Typically
> > this will be the reset value, <whatever>A, although the bootloader could
> > have established a different value.
> >
> > Allow the correct OCP value to be provided by the DT."
> >
> > BTW please don't uncritically copy the above into the patch header. It is
> > just made something up as an example and I did no fact checking...
> >
> OK, got it.
> >
> > >
> > > Reported-by: Lucas Tsai <lucas_tsai@xxxxxxxxxxx>
> > > Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > > ---
> > >  drivers/video/backlight/rt4831-backlight.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/drivers/video/backlight/rt4831-backlight.c b/drivers/video/backlight/rt4831-backlight.c
> > > index 42155c7..c81f7d9 100644
> > > --- a/drivers/video/backlight/rt4831-backlight.c
> > > +++ b/drivers/video/backlight/rt4831-backlight.c
> > > @@ -12,6 +12,7 @@
> > >  #define RT4831_REG_BLCFG     0x02
> > >  #define RT4831_REG_BLDIML    0x04
> > >  #define RT4831_REG_ENABLE    0x08
> > > +#define RT4831_REG_BLOPT2    0x11
> > >
> > >  #define RT4831_BLMAX_BRIGHTNESS      2048
> > >
> > > @@ -23,6 +24,8 @@
> > >  #define RT4831_BLDIML_MASK   GENMASK(2, 0)
> > >  #define RT4831_BLDIMH_MASK   GENMASK(10, 3)
> > >  #define RT4831_BLDIMH_SHIFT  3
> > > +#define RT4831_BLOCP_MASK    GENMASK(1, 0)
> > > +#define RT4831_BLOCP_SHIFT   0
> > >
> > >  struct rt4831_priv {
> > >       struct device *dev;
> > > @@ -120,6 +123,16 @@ static int rt4831_parse_backlight_properties(struct rt4831_priv *priv,
> > >       if (ret)
> > >               return ret;
> > >
> > > +     ret = device_property_read_u8(dev, "richtek,bled-ocp-sel", &propval);
> > > +     if (ret)
> > > +             propval = RT4831_BLOCPLVL_1P2A;
> >
> > Is 1.2A the reset value for the register?
> Yes, it's the HW default value.
> >
> > Additionally, it looks like adding a hard-coded default would cause
> > problems for existing platforms where the bootloader doesn't use
> > richtek,bled-ocp-sel and pre-configures a different value itself.
> >
> > Would it be safer (in terms of working nicely with older bootloaders)
> > to only write to the RT4831_BLOCP_MASK if the new property is set?
> >
> Ah, my excuse. I really didn't consider the case that you mentioned.
> It seems it's better to do the judgement here for two cases.
> 1) property not exist, keep the current HW value
> 2) property exist, clamp align and update the suitable selector to HW.

Ok, great.

When you make this change can you make sure there is a comment in the
source code explaining that concerns about older firmware is *why* we
treat bled-ocp-sel differently to bled-ovp-sel!


Thanks

Daniel.