diff mbox series

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

Message ID 1653534995-30794-3-git-send-email-u0084500@gmail.com
State New
Headers show
Series Add the property to make ocp level selectable | expand

Commit Message

cy_huang May 26, 2022, 3:16 a.m. UTC
From: ChiYuan Huang <cy_huang@richtek.com>

Add the property parsing for ocp level selection.

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(+)

Comments

Daniel Thompson May 26, 2022, 10:05 a.m. UTC | #1
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?

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


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

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?


Daniel.



> +
> +	propval = min_t(u8, propval, RT4831_BLOCPLVL_1P8A);
> +	ret = regmap_update_bits(priv->regmap, RT4831_REG_BLOPT2, RT4831_BLOCP_MASK,
> +				 propval << RT4831_BLOCP_SHIFT);
> +	if (ret)
> +		return ret;
> +
>  	ret = device_property_read_u8(dev, "richtek,channel-use", &propval);
>  	if (ret) {
>  		dev_err(dev, "richtek,channel-use DT property missing\n");
> -- 
> 2.7.4
>
cy_huang May 27, 2022, 2:24 a.m. UTC | #2
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.

Thanks.
>
> Daniel.
>
>
>
> > +
> > +     propval = min_t(u8, propval, RT4831_BLOCPLVL_1P8A);
> > +     ret = regmap_update_bits(priv->regmap, RT4831_REG_BLOPT2, RT4831_BLOCP_MASK,
> > +                              propval << RT4831_BLOCP_SHIFT);
> > +     if (ret)
> > +             return ret;
> > +
> >       ret = device_property_read_u8(dev, "richtek,channel-use", &propval);
> >       if (ret) {
> >               dev_err(dev, "richtek,channel-use DT property missing\n");
> > --
> > 2.7.4
> >
Daniel Thompson May 27, 2022, 10:56 a.m. UTC | #3
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.
diff mbox series

Patch

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;
+
+	propval = min_t(u8, propval, RT4831_BLOCPLVL_1P8A);
+	ret = regmap_update_bits(priv->regmap, RT4831_REG_BLOPT2, RT4831_BLOCP_MASK,
+				 propval << RT4831_BLOCP_SHIFT);
+	if (ret)
+		return ret;
+
 	ret = device_property_read_u8(dev, "richtek,channel-use", &propval);
 	if (ret) {
 		dev_err(dev, "richtek,channel-use DT property missing\n");