Subject: Re: [PATCH v2] backlight: lm3630a: bump REG_MAX value to 0x50 instead
 of 0x1F
To: Bhushan Shah <bshah@kde.org>
Cc: Lee Jones <lee.jones@linaro.org>, Jingoo Han <jingoohan1@gmail.com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
        linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20170621105230.GA13390@archbox>
From: Daniel Thompson <daniel.thompson@linaro.org>
Message-ID: <6fc13c32-da09-b644-ac3b-7926cc767c9e@linaro.org>
Date: Wed, 21 Jun 2017 12:03:29 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.1.0
MIME-Version: 1.0
In-Reply-To: <20170621105230.GA13390@archbox>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3290
Lines: 93

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@kde.org>
>>> Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>
>> 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 */
>>>
>>
>