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

[Jonas Karlman]

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.

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.

Regards,
Jonas

> 
> Reviewed-by: Quentin Schulz <quentin.schulz at cherry.de>
> 
> @Kever Since it's a bugfix, can we have this in your next MR for 
> v2024.10 please? Thanks!
> 
> Thanks!
> Quentin