Re: [PATCH v2] backlight: lm3630a: bump REG_MAX value to 0x50 instead of 0x1F

[Daniel Thompson]

On 21/06/17 11:52, Bhushan Shah wrote:
> On Wed, Jun 21, 2017 at 10:37:43AM +0100, Daniel Thompson wrote:
> > On 21/06/17 06:31, Bhushan Shah wrote:
> > > In the lm3630a_chip_init we try to write to 0x50 register, which is
> > > higher value then the max_register value, this resulted in regmap_write
> > > return -EIO.
> > >
> > > Fix this by bumping REG_MAX value to 0x50. >
> > > Signed-off-by: Bhushan Shah <bshah@xxxxxxx>
> > > Suggested-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> >
> > Can we get a "Fixes" on this? It looks to me like it has been broken since
> > 2a0c316bf3cc ("fix signedness bug in lm3630a_chip_init()").
>
> I don't think 2a0c316bf3cc is the right commit to mention in Fixes tag?
> Because it was broken since the introduction of chip revision in commit
> 28e64a68a2ef ("backlight: lm3630: apply chip revision").
>
> commit 2a0c316bf3cc just made it error out correctly instead of failing
> silently.
>
> What do you think?

Depends if we chase the seeds of disaster or problems user experience.

For sure I don't really attach very much blame to 2a0c316bf3cc. However 
that *is* the commit from which the probe() will have stopped working 
and is therefore is probably what would be picked out by a bisect... if 
you had the patience to get a 2013 upstream kernel running on your Nexus ;-)

The seeds of disaster come from 28e64a68a2ef ("backlight: lm3630: apply 
chip revision") which was the commit that (didn't) add support for this 
register.

Personally I think it would be OK to put in two commits both commits in 
the Fixes: (if you really want to be even handed explain in the patch 
description).

> > Also I assume you find this by trying to use the driver on real hardware? If
> > so can you confirm what you tested on in the patch description. As far as I
> > can tell the code to set the filter strength has never worked, so you'll be
> > the first user of it!
>
> Yes I hit this problem by trying it on LGE Nexus 5 (hammerhead). I will
> mention this in the v3 revision of patch.

Thanks.


> > > ---
> > >
> > > Changes since v1:
> > >       - Fix the lm3630a_write call to use proper value (sent worng patch earlier)
> > >
> > >    drivers/video/backlight/lm3630a_bl.c | 5 +++--
> > >    1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
> > > index 60d6c2ac87aa..b641f706dbc9 100644
> > > --- a/drivers/video/backlight/lm3630a_bl.c
> > > +++ b/drivers/video/backlight/lm3630a_bl.c
> > > @@ -31,7 +31,8 @@
> > >    #define REG_FAULT	0x0B
> > >    #define REG_PWM_OUTLOW	0x12
> > >    #define REG_PWM_OUTHIGH	0x13
> > > -#define REG_MAX		0x1F
> > > +#define REG_FLTR_STR	0x50
> >
> > Can we expand this to REG_FILTER_STRENGTH?
>
> Sure, will include in Patch v3.
>
> > Daniel.
> >
> > > +#define REG_MAX		0x50
> > >    #define INT_DEBOUNCE_MSEC	10
> > >    struct lm3630a_chip {
> > > @@ -80,7 +81,7 @@ static int lm3630a_chip_init(struct lm3630a_chip *pchip)
> > >    	usleep_range(1000, 2000);
> > >    	/* set Filter Strength Register */
> > > -	rval = lm3630a_write(pchip, 0x50, 0x03);
> > > +	rval = lm3630a_write(pchip, REG_FLTR_STR, 0x03);
> > >    	/* set Cofig. register */
> > >    	rval |= lm3630a_update(pchip, REG_CONFIG, 0x07, pdata->pwm_ctrl);
> > >    	/* set boost control */