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

On 21/06/17 11:52, Bhushan Shah wrote:

On Wed, Jun 21, 2017 at 10:37:43AM +0100, Daniel Thompson wrote:

quoted

On 21/06/17 06:31, Bhushan Shah wrote:

quoted

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 <redacted>
Suggested-by: Bjorn Andersson <redacted>

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

quoted

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.

quoted

quoted

---

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.

quoted

Daniel.

quoted

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