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

[Bhushan Shah]

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?

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

> 
> > ---
> > 
> > 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 */
> > 
> 

-- 
Bhushan Shah
http://blog.bshah.in
IRC Nick : bshah on Freenode
GPG key fingerprint : 0AAC 775B B643 7A8D 9AF7 A3AC FE07 8411 7FBC E11D

Attachment: signature.asc
Description: PGP signature