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

From: Daniel Thompson
Date: Wed Jun 21 2017 - 07:03:38 EST

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

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.



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.


+#define REG_MAX 0x50
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 */