[PATCH] regulator: rk8xx: Fix buck get and set enabled state on RK806

[Quentin Schulz]

Hi Jonas,

On 9/2/24 4:28 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2024-09-02 13:14, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 8/31/24 12:42 AM, Jonas Karlman wrote:
>>> Wrong POWER_EN reg is used to get and set enabled state for the RK806
>>> buck 4 and 8 regulators, also wrong POWER_SLP_EN0 bit is used for
>>> suspend state for the RK806 buck 1-8 regulators.
>>>
>>> Fix this by not adding one to the zero based buck variable.
>>>
>>> Fixes: f172575d92cd ("power: rk8xx: add support for RK806")
>>
>> Shoot, I made a lot of mistakes in that driver :/
>>
>> Thanks for catching those :)
>>
>>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>>> ---
>>>    drivers/power/regulator/rk8xx.c | 12 ++++++------
>>>    1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c
>>> index 34e61511d884..3f5ec02b3824 100644
>>> --- a/drivers/power/regulator/rk8xx.c
>>> +++ b/drivers/power/regulator/rk8xx.c
>>> @@ -415,7 +415,7 @@ static int _buck_set_enable(struct udevice *pmic, int buck, bool enable)
>>>    		break;
>>>    	case RK806_ID:
>>>    		value = RK806_POWER_EN_CLRSETBITS(buck % 4, enable);
>>> -		en_reg = RK806_POWER_EN((buck + 1) / 4);
>>> +		en_reg = RK806_POWER_EN(buck / 4);
>>>    		ret = pmic_reg_write(pmic, en_reg, value);
>>>    		break;
>>>    	case RK808_ID:
>>> @@ -494,7 +494,7 @@ static int _buck_get_enable(struct udevice *pmic, int buck)
>>>    		break;
>>>    	case RK806_ID:
>>>    		mask = BIT(buck % 4);
>>> -		ret = pmic_reg_read(pmic, RK806_POWER_EN((buck + 1) / 4));
>>> +		ret = pmic_reg_read(pmic, RK806_POWER_EN(buck / 4));
>>>    		break;
>>>    	case RK808_ID:
>>>    	case RK818_ID:
>>> @@ -541,10 +541,10 @@ static int _buck_set_suspend_enable(struct udevice *pmic, int buck, bool enable)
>>>    
>>>    			if (buck + 1 >= 9) {
>>>    				reg = RK806_POWER_SLP_EN1;
>>> -				mask = BIT(buck + 1 - 3);
>>> +				mask = BIT(buck - 2);
>>
>> I like my (+ 1 - 3) here to match buck + 1 above. buck + 1 represents
>> the buck number in the datasheet (index starts at one), so you need to
>> subtract 3 to that index to find the bit index in the register.
> 
> I understand this reasoning and would fully agree if this was the only
> use of buck in BIT(), however each time I tried to scan over this code,
> (with buck is 0-based in mind) this buck + 1 - 3 made me stop and
> re-think if this really was correct.
> 

Fair enough. Should we migrate buck + 1 >= 9 to buck >= 8 with an 
additional comment explaining this is for BUCK9/BUCK10?

> I have no strong opinion and can revert this change if you like.
> 
>>
>>>    			} else {
>>>    				reg = RK806_POWER_SLP_EN0;
>>> -				mask = BIT(buck + 1);
>>> +				mask = BIT(buck);
>>>    			}
>>>    			ret = pmic_clrsetbits(pmic, reg, mask, enable ? mask : 0);
>>>    		}
>>> @@ -592,10 +592,10 @@ static int _buck_get_suspend_enable(struct udevice *pmic, int buck)
>>>    
>>>    			if (buck + 1 >= 9) {
>>>    				reg = RK806_POWER_SLP_EN1;
>>> -				mask = BIT(buck + 1 - 3);
>>> +				mask = BIT(buck - 2);
>>
>> Ditto.
>>
>> Just a matter of taste though!
> 
> Let me know if you want me to send a v2 with this change reverted.
> 

It just creeps in along changes that are actually bugs and I had to do a 
double take on the diff to make sure it was correct.

I could suggest two separate commits:
1) bug fixes from buck + 1 to buck (basically everything but the buck - 
2 changes)
2) change buck + 1 that I added for "readability" to buck by subtracting 
one elsewhere and maybe adding a comment for better readability?

What do you think?

Cheers,
Quentin