Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 27 May 2022 11:56:07 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     ChiYuan Huang <u0084500@gmail.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Lee Jones <lee.jones@linaro.org>, jingoohan1@gmail.com,
        Pavel Machek <pavel@ucw.cz>, deller@gmx.de,
        cy_huang <cy_huang@richtek.com>, lucas_tsai@richtek.com,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, dri-devel@lists.freedesktop.org,
        linux-fbdev@vger.kernel.org,
        Linux LED Subsystem <linux-leds@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] backlight: rt4831: Add the property parsing for ocp
 level selection
Message-ID: <20220527105607.gk5ge77byuojxkkc@maple.lan>
References: <1653534995-30794-1-git-send-email-u0084500@gmail.com>
 <1653534995-30794-3-git-send-email-u0084500@gmail.com>
 <20220526100510.3utwh5bov4ax2jmg@maple.lan>
 <CADiBU3_3rJ6uCYx_W+TZJpuPzGtt61QEDwZWtxy_abzynTr8VQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CADiBU3_3rJ6uCYx_W+TZJpuPzGtt61QEDwZWtxy_abzynTr8VQ@mail.gmail.com>
Precedence: bulk

On Fri, May 27, 2022 at 10:24:42AM +0800, ChiYuan Huang wrote:
> Daniel Thompson <daniel.thompson@linaro.org> 於 2022年5月26日 週四 下午6:05寫道：
> >
> > On Thu, May 26, 2022 at 11:16:35AM +0800, cy_huang wrote:
> > > From: ChiYuan Huang <cy_huang@richtek.com>
> > >
> > > 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@richtek.com>
> > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > ---
> > >  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.