[PATCH v1 0/2] TPS65219 U-Boot Driver Clean-Up

[PATCH v1 0/2] TPS65219 U-Boot Driver Clean-Up

[Shree Ramamoorthy]

This clean-up series is sent in preparation to add 2 PMIC devices to the 
TPS65219 Driver. The changes involve replacing magic numbers with macros 
and replacing printf() instances with pr_err(). The intention is to remove 
unnecessary noise from the new PMIC device patches added to this driver.

Test Log (AM62B-P1 + TPS65219 EVM):
https://gist.github.com/ramamoorthyhs/8c81e33cf6d46c8b5fc135424a5f224b

Kernel baseline used in test log:
Reg: https://lore.kernel.org/all/20241217204526.1010989-1-s-ramamoorthy@ti.com/
GPIO: https://lore.kernel.org/all/20241217204755.1011731-1-s-ramamoorthy@ti.com/
MFD: https://lore.kernel.org/all/20241217204935.1012106-1-s-ramamoorthy@ti.com/

Shree Ramamoorthy (2):
  power: regulator: replace printf() with pr_err()
  power: replace magic numbers with macros

 drivers/power/regulator/tps65219_regulator.c | 30 ++++++++++----------
 include/power/tps65219.h                     | 14 +++++++--
 2 files changed, 27 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH v1 1/2] power: regulator: replace printf() with pr_err()

[Shree Ramamoorthy]

Replace printf() with pr_err() because pr_err() has a uniform print format
and takes into consideration the log levels supported.

Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
---
 drivers/power/regulator/tps65219_regulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/power/regulator/tps65219_regulator.c b/drivers/power/regulator/tps65219_regulator.c
index b7124fed0245..4b0fb205909a 100644
--- a/drivers/power/regulator/tps65219_regulator.c
+++ b/drivers/power/regulator/tps65219_regulator.c
@@ -250,7 +250,7 @@ static int tps65219_ldo_probe(struct udevice *dev)
 	/* idx must be in 1..TPS65219_LDO_NUM */
 	idx = dev->driver_data;
 	if (idx < 1 || idx > TPS65219_LDO_NUM) {
-		printf("Wrong ID for regulator\n");
+		pr_err("Wrong ID for regulator\n");
 		return -EINVAL;
 	}
 
@@ -271,7 +271,7 @@ static int tps65219_buck_probe(struct udevice *dev)
 	/* idx must be in 1..TPS65219_BUCK_NUM */
 	idx = dev->driver_data;
 	if (idx < 1 || idx > TPS65219_BUCK_NUM) {
-		printf("Wrong ID for regulator\n");
+		pr_err("Wrong ID for regulator\n");
 		return -EINVAL;
 	}
 
-- 
2.34.1

[PATCH v1 2/2] power: replace magic numbers with macros

[Shree Ramamoorthy]

Replace magic numbers in buckval2votl() & buckvolt2val() with macros to
help with clarity and correlate what the numbers correspond to in the
TPS65219 datasheet.

Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
---
 drivers/power/regulator/tps65219_regulator.c | 26 ++++++++++----------
 include/power/tps65219.h                     | 14 +++++++++--
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/power/regulator/tps65219_regulator.c b/drivers/power/regulator/tps65219_regulator.c
index 4b0fb205909a..88abc896b3a2 100644
--- a/drivers/power/regulator/tps65219_regulator.c
+++ b/drivers/power/regulator/tps65219_regulator.c
@@ -72,12 +72,12 @@ static int tps65219_buck_enable(struct udevice *dev, int op, bool *enable)
 
 static int tps65219_buck_volt2val(int uV)
 {
-	if (uV > TPS65219_BUCK_VOLT_MAX)
+	if (uV > TPS65219_BUCK_3V4)
 		return -EINVAL;
-	else if (uV >= 1400000)
-		return (uV - 1400000) / 100000 + 0x20;
-	else if (uV >= 600000)
-		return (uV - 600000) / 25000 + 0x00;
+	else if (uV >= TPS65219_BUCK_1V4)
+		return (uV - TPS65219_BUCK_1V4) / TPS65219_VOLT_STEP_100MV + TPS65219_BUCK_REG_1V4;
+	else if (uV >= TPS65219_BUCK_0V6)
+		return (uV - TPS65219_BUCK_0V6) / TPS65219_VOLT_STEP_25MV + TPS65219_BUCK_REG_0V6;
 	else
 		return -EINVAL;
 }
@@ -86,12 +86,12 @@ static int tps65219_buck_val2volt(int val)
 {
 	if (val > TPS65219_VOLT_MASK)
 		return -EINVAL;
-	else if (val > 0x34)
-		return TPS65219_BUCK_VOLT_MAX;
-	else if (val > 0x20)
-		return 1400000 + (val - 0x20) * 100000;
-	else if (val >= 0)
-		return 600000 + val * 25000;
+	else if (val > TPS65219_BUCK_REG_3V4)
+		return TPS65219_BUCK_3V4;
+	else if (val > TPS65219_BUCK_REG_1V4)
+		return TPS65219_BUCK_1V4 + (val - TPS65219_BUCK_REG_1V4) * TPS65219_VOLT_STEP_100MV;
+	else if (val >= TPS65219_BUCK_REG_0V6)
+		return TPS65219_BUCK_0V6 + val * TPS65219_VOLT_STEP_25MV;
 	else
 		return -EINVAL;
 }
@@ -161,7 +161,7 @@ static int tps65219_ldo_volt2val(int idx, int uV)
 	if (uV > max)
 		return -EINVAL;
 	else if (uV >= base)
-		return (uV - TPS65219_LDO12_VOLT_MIN) / 50000;
+		return (uV - TPS65219_LDO12_VOLT_MIN) / TPS65219_VOLT_STEP_50MV;
 	else
 		return -EINVAL;
 }
@@ -187,7 +187,7 @@ static int tps65219_ldo_val2volt(int idx, int val)
 	else if (val <= reg_base)
 		return base;
 	else if (val >= 0)
-		return TPS65219_LDO12_VOLT_MIN + (50000 * val);
+		return TPS65219_LDO12_VOLT_MIN + (TPS65219_VOLT_STEP_50MV * val);
 	else
 		return -EINVAL;
 }
diff --git a/include/power/tps65219.h b/include/power/tps65219.h
index aa81b92266fd..e8780af2d811 100644
--- a/include/power/tps65219.h
+++ b/include/power/tps65219.h
@@ -17,10 +17,20 @@
 #define TPS65219_BUCK_DRIVER		"tps65219_buck"
 
 #define TPS65219_VOLT_MASK		0x3F
-#define TPS65219_BUCK_VOLT_MAX		3400000
-
 #define TPS65219_ENABLE_CTRL_REG	0x2
 
+#define TPS65219_VOLT_STEP_25MV		25000
+#define TPS65219_VOLT_STEP_50MV		50000
+#define TPS65219_VOLT_STEP_100MV	100000
+
+#define TPS65219_BUCK_0V6		600000
+#define TPS65219_BUCK_1V4		1400000
+#define TPS65219_BUCK_3V4		3400000
+
+#define TPS65219_BUCK_REG_0V6		0x00
+#define TPS65219_BUCK_REG_1V4		0x20
+#define TPS65219_BUCK_REG_3V4		0x34
+
 #define TPS65219_BUCK1_VOUT_REG		0xa
 #define TPS65219_BUCK2_VOUT_REG		0x9
 #define TPS65219_BUCK3_VOUT_REG		0x8
-- 
2.34.1

RE: [PATCH v1 1/2] power: regulator: replace printf() with pr_err()

[Jaehoon Chung]

> -----Original Message-----
> From: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> Sent: Thursday, December 19, 2024 2:13 AM
>
> Replace printf() with pr_err() because pr_err() has a uniform print format
> and takes into consideration the log levels supported.
>
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
>  drivers/power/regulator/tps65219_regulator.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/regulator/tps65219_regulator.c
> b/drivers/power/regulator/tps65219_regulator.c
> index b7124fed0245..4b0fb205909a 100644
> --- a/drivers/power/regulator/tps65219_regulator.c
> +++ b/drivers/power/regulator/tps65219_regulator.c
> @@ -250,7 +250,7 @@ static int tps65219_ldo_probe(struct udevice *dev)
>  	/* idx must be in 1..TPS65219_LDO_NUM */
>  	idx = dev->driver_data;
>  	if (idx < 1 || idx > TPS65219_LDO_NUM) {
> -		printf("Wrong ID for regulator\n");
> +		pr_err("Wrong ID for regulator\n");
>  		return -EINVAL;
>  	}
>
> @@ -271,7 +271,7 @@ static int tps65219_buck_probe(struct udevice *dev)
>  	/* idx must be in 1..TPS65219_BUCK_NUM */
>  	idx = dev->driver_data;
>  	if (idx < 1 || idx > TPS65219_BUCK_NUM) {
> -		printf("Wrong ID for regulator\n");
> +		pr_err("Wrong ID for regulator\n");
>  		return -EINVAL;
>  	}
>
> --
> 2.34.1

RE: [PATCH v1 2/2] power: replace magic numbers with macros

[Jaehoon Chung]

Hi,

> -----Original Message-----
> From: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> Sent: Thursday, December 19, 2024 2:13 AM
>
> Replace magic numbers in buckval2votl() & buckvolt2val() with macros to
> help with clarity and correlate what the numbers correspond to in the
> TPS65219 datasheet.
>
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> ---
>  drivers/power/regulator/tps65219_regulator.c | 26 ++++++++++----------
>  include/power/tps65219.h                     | 14 +++++++++--
>  2 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/power/regulator/tps65219_regulator.c
> b/drivers/power/regulator/tps65219_regulator.c
> index 4b0fb205909a..88abc896b3a2 100644
> --- a/drivers/power/regulator/tps65219_regulator.c
> +++ b/drivers/power/regulator/tps65219_regulator.c
> @@ -72,12 +72,12 @@ static int tps65219_buck_enable(struct udevice *dev, int op, bool *enable)
>
>  static int tps65219_buck_volt2val(int uV)
>  {
> -	if (uV > TPS65219_BUCK_VOLT_MAX)
> +	if (uV > TPS65219_BUCK_3V4)
>  		return -EINVAL;
> -	else if (uV >= 1400000)
> -		return (uV - 1400000) / 100000 + 0x20;
> -	else if (uV >= 600000)
> -		return (uV - 600000) / 25000 + 0x00;
> +	else if (uV >= TPS65219_BUCK_1V4)
> +		return (uV - TPS65219_BUCK_1V4) / TPS65219_VOLT_STEP_100MV + TPS65219_BUCK_REG_1V4;

Even though Not relevant to this subject. If uV is 340000, the return value is correct?

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> +	else if (uV >= TPS65219_BUCK_0V6)
> +		return (uV - TPS65219_BUCK_0V6) / TPS65219_VOLT_STEP_25MV + TPS65219_BUCK_REG_0V6;
>  	else
>  		return -EINVAL;
>  }
> @@ -86,12 +86,12 @@ static int tps65219_buck_val2volt(int val)
>  {
>  	if (val > TPS65219_VOLT_MASK)
>  		return -EINVAL;
> -	else if (val > 0x34)
> -		return TPS65219_BUCK_VOLT_MAX;
> -	else if (val > 0x20)
> -		return 1400000 + (val - 0x20) * 100000;
> -	else if (val >= 0)
> -		return 600000 + val * 25000;
> +	else if (val > TPS65219_BUCK_REG_3V4)
> +		return TPS65219_BUCK_3V4;
> +	else if (val > TPS65219_BUCK_REG_1V4)
> +		return TPS65219_BUCK_1V4 + (val - TPS65219_BUCK_REG_1V4) * TPS65219_VOLT_STEP_100MV;
> +	else if (val >= TPS65219_BUCK_REG_0V6)
> +		return TPS65219_BUCK_0V6 + val * TPS65219_VOLT_STEP_25MV;
>  	else
>  		return -EINVAL;
>  }
> @@ -161,7 +161,7 @@ static int tps65219_ldo_volt2val(int idx, int uV)
>  	if (uV > max)
>  		return -EINVAL;
>  	else if (uV >= base)
> -		return (uV - TPS65219_LDO12_VOLT_MIN) / 50000;
> +		return (uV - TPS65219_LDO12_VOLT_MIN) / TPS65219_VOLT_STEP_50MV;
>  	else
>  		return -EINVAL;
>  }
> @@ -187,7 +187,7 @@ static int tps65219_ldo_val2volt(int idx, int val)
>  	else if (val <= reg_base)
>  		return base;
>  	else if (val >= 0)
> -		return TPS65219_LDO12_VOLT_MIN + (50000 * val);
> +		return TPS65219_LDO12_VOLT_MIN + (TPS65219_VOLT_STEP_50MV * val);
>  	else
>  		return -EINVAL;
>  }
> diff --git a/include/power/tps65219.h b/include/power/tps65219.h
> index aa81b92266fd..e8780af2d811 100644
> --- a/include/power/tps65219.h
> +++ b/include/power/tps65219.h
> @@ -17,10 +17,20 @@
>  #define TPS65219_BUCK_DRIVER		"tps65219_buck"
>
>  #define TPS65219_VOLT_MASK		0x3F
> -#define TPS65219_BUCK_VOLT_MAX		3400000
> -
>  #define TPS65219_ENABLE_CTRL_REG	0x2
>
> +#define TPS65219_VOLT_STEP_25MV		25000
> +#define TPS65219_VOLT_STEP_50MV		50000
> +#define TPS65219_VOLT_STEP_100MV	100000
> +
> +#define TPS65219_BUCK_0V6		600000
> +#define TPS65219_BUCK_1V4		1400000
> +#define TPS65219_BUCK_3V4		3400000
> +
> +#define TPS65219_BUCK_REG_0V6		0x00
> +#define TPS65219_BUCK_REG_1V4		0x20
> +#define TPS65219_BUCK_REG_3V4		0x34
> +
>  #define TPS65219_BUCK1_VOUT_REG		0xa
>  #define TPS65219_BUCK2_VOUT_REG		0x9
>  #define TPS65219_BUCK3_VOUT_REG		0x8
> --
> 2.34.1

Re: [PATCH v1 2/2] power: replace magic numbers with macros

[Shree Ramamoorthy]

Hi.


On 12/18/24 4:44 PM, Jaehoon Chung wrote:
> Hi,
>
>> -----Original Message-----
>> From: Shree Ramamoorthy <s-ramamoorthy@ti.com>
>> Sent: Thursday, December 19, 2024 2:13 AM
>>
>> Replace magic numbers in buckval2votl() & buckvolt2val() with macros to
>> help with clarity and correlate what the numbers correspond to in the
>> TPS65219 datasheet.
>>
>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
>> ---
>>   drivers/power/regulator/tps65219_regulator.c | 26 ++++++++++----------
>>   include/power/tps65219.h                     | 14 +++++++++--
>>   2 files changed, 25 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/power/regulator/tps65219_regulator.c
>> b/drivers/power/regulator/tps65219_regulator.c
>> index 4b0fb205909a..88abc896b3a2 100644
>> --- a/drivers/power/regulator/tps65219_regulator.c
>> +++ b/drivers/power/regulator/tps65219_regulator.c
>> @@ -72,12 +72,12 @@ static int tps65219_buck_enable(struct udevice *dev, int op, bool *enable)
>>
>>   static int tps65219_buck_volt2val(int uV)
>>   {
>> -	if (uV > TPS65219_BUCK_VOLT_MAX)
>> +	if (uV > TPS65219_BUCK_3V4)
>>   		return -EINVAL;
>> -	else if (uV >= 1400000)
>> -		return (uV - 1400000) / 100000 + 0x20;
>> -	else if (uV >= 600000)
>> -		return (uV - 600000) / 25000 + 0x00;
>> +	else if (uV >= TPS65219_BUCK_1V4)
>> +		return (uV - TPS65219_BUCK_1V4) / TPS65219_VOLT_STEP_100MV + TPS65219_BUCK_REG_1V4;
> Even though Not relevant to this subject. If uV is 340000, the return value is correct?
>
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>
> Best Regards,
> Jaehoon Chung

Thank you for reviewing!
The allowed max uV is 3.4V inclusive.
If 340000 uV is detected, the first 'else if' statement should be taken.

Best,
Shree

>> +	else if (uV >= TPS65219_BUCK_0V6)
>> +		return (uV - TPS65219_BUCK_0V6) / TPS65219_VOLT_STEP_25MV + TPS65219_BUCK_REG_0V6;
>>   	else
>>   		return -EINVAL;
>>   }
>> @@ -86,12 +86,12 @@ static int tps65219_buck_val2volt(int val)
>>   {
>>   	if (val > TPS65219_VOLT_MASK)
>>   		return -EINVAL;
>> -	else if (val > 0x34)
>> -		return TPS65219_BUCK_VOLT_MAX;
>> -	else if (val > 0x20)
>> -		return 1400000 + (val - 0x20) * 100000;
>> -	else if (val >= 0)
>> -		return 600000 + val * 25000;
>> +	else if (val > TPS65219_BUCK_REG_3V4)
>> +		return TPS65219_BUCK_3V4;
>> +	else if (val > TPS65219_BUCK_REG_1V4)
>> +		return TPS65219_BUCK_1V4 + (val - TPS65219_BUCK_REG_1V4) * TPS65219_VOLT_STEP_100MV;
>> +	else if (val >= TPS65219_BUCK_REG_0V6)
>> +		return TPS65219_BUCK_0V6 + val * TPS65219_VOLT_STEP_25MV;
>>   	else
>>   		return -EINVAL;
>>   }
>> @@ -161,7 +161,7 @@ static int tps65219_ldo_volt2val(int idx, int uV)
>>   	if (uV > max)
>>   		return -EINVAL;
>>   	else if (uV >= base)
>> -		return (uV - TPS65219_LDO12_VOLT_MIN) / 50000;
>> +		return (uV - TPS65219_LDO12_VOLT_MIN) / TPS65219_VOLT_STEP_50MV;
>>   	else
>>   		return -EINVAL;
>>   }
>> @@ -187,7 +187,7 @@ static int tps65219_ldo_val2volt(int idx, int val)
>>   	else if (val <= reg_base)
>>   		return base;
>>   	else if (val >= 0)
>> -		return TPS65219_LDO12_VOLT_MIN + (50000 * val);
>> +		return TPS65219_LDO12_VOLT_MIN + (TPS65219_VOLT_STEP_50MV * val);
>>   	else
>>   		return -EINVAL;
>>   }
>> diff --git a/include/power/tps65219.h b/include/power/tps65219.h
>> index aa81b92266fd..e8780af2d811 100644
>> --- a/include/power/tps65219.h
>> +++ b/include/power/tps65219.h
>> @@ -17,10 +17,20 @@
>>   #define TPS65219_BUCK_DRIVER		"tps65219_buck"
>>
>>   #define TPS65219_VOLT_MASK		0x3F
>> -#define TPS65219_BUCK_VOLT_MAX		3400000
>> -
>>   #define TPS65219_ENABLE_CTRL_REG	0x2
>>
>> +#define TPS65219_VOLT_STEP_25MV		25000
>> +#define TPS65219_VOLT_STEP_50MV		50000
>> +#define TPS65219_VOLT_STEP_100MV	100000
>> +
>> +#define TPS65219_BUCK_0V6		600000
>> +#define TPS65219_BUCK_1V4		1400000
>> +#define TPS65219_BUCK_3V4		3400000
>> +
>> +#define TPS65219_BUCK_REG_0V6		0x00
>> +#define TPS65219_BUCK_REG_1V4		0x20
>> +#define TPS65219_BUCK_REG_3V4		0x34
>> +
>>   #define TPS65219_BUCK1_VOUT_REG		0xa
>>   #define TPS65219_BUCK2_VOUT_REG		0x9
>>   #define TPS65219_BUCK3_VOUT_REG		0x8
>> --
>> 2.34.1
>
>

-- 
Best,
Shree Ramamoorthy
PMIC Software Engineer

RE: [PATCH v1 2/2] power: replace magic numbers with macros

[Jaehoon Chung]

> -----Original Message-----
> From: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> Sent: Thursday, December 19, 2024 7:55 AM
>
> Hi.
>
>
> On 12/18/24 4:44 PM, Jaehoon Chung wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> >> Sent: Thursday, December 19, 2024 2:13 AM
> >>
> >> Replace magic numbers in buckval2votl() & buckvolt2val() with macros to
> >> help with clarity and correlate what the numbers correspond to in the
> >> TPS65219 datasheet.
> >>
> >> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> >> ---
> >>   drivers/power/regulator/tps65219_regulator.c | 26 ++++++++++----------
> >>   include/power/tps65219.h                     | 14 +++++++++--
> >>   2 files changed, 25 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/power/regulator/tps65219_regulator.c
> >> b/drivers/power/regulator/tps65219_regulator.c
> >> index 4b0fb205909a..88abc896b3a2 100644
> >> --- a/drivers/power/regulator/tps65219_regulator.c
> >> +++ b/drivers/power/regulator/tps65219_regulator.c
> >> @@ -72,12 +72,12 @@ static int tps65219_buck_enable(struct udevice *dev, int op, bool *enable)
> >>
> >>   static int tps65219_buck_volt2val(int uV)
> >>   {
> >> -	if (uV > TPS65219_BUCK_VOLT_MAX)
> >> +	if (uV > TPS65219_BUCK_3V4)
> >>   		return -EINVAL;
> >> -	else if (uV >= 1400000)
> >> -		return (uV - 1400000) / 100000 + 0x20;
> >> -	else if (uV >= 600000)
> >> -		return (uV - 600000) / 25000 + 0x00;
> >> +	else if (uV >= TPS65219_BUCK_1V4)
> >> +		return (uV - TPS65219_BUCK_1V4) / TPS65219_VOLT_STEP_100MV + TPS65219_BUCK_REG_1V4;
> > Even though Not relevant to this subject. If uV is 340000, the return value is correct?
> >
> > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> >
> > Best Regards,
> > Jaehoon Chung
>
> Thank you for reviewing!
> The allowed max uV is 3.4V inclusive.
> If 340000 uV is detected, the first 'else if' statement should be taken.

Ah, I missed wrong read. Thanks for checking. If you can do, other tpsXXX can also be changed to readable macro.

Best Regards,
Jaehoon Chung

>
> Best,
> Shree
>
> >> +	else if (uV >= TPS65219_BUCK_0V6)
> >> +		return (uV - TPS65219_BUCK_0V6) / TPS65219_VOLT_STEP_25MV + TPS65219_BUCK_REG_0V6;
> >>   	else
> >>   		return -EINVAL;
> >>   }
> >> @@ -86,12 +86,12 @@ static int tps65219_buck_val2volt(int val)
> >>   {
> >>   	if (val > TPS65219_VOLT_MASK)
> >>   		return -EINVAL;
> >> -	else if (val > 0x34)
> >> -		return TPS65219_BUCK_VOLT_MAX;
> >> -	else if (val > 0x20)
> >> -		return 1400000 + (val - 0x20) * 100000;
> >> -	else if (val >= 0)
> >> -		return 600000 + val * 25000;
> >> +	else if (val > TPS65219_BUCK_REG_3V4)
> >> +		return TPS65219_BUCK_3V4;
> >> +	else if (val > TPS65219_BUCK_REG_1V4)
> >> +		return TPS65219_BUCK_1V4 + (val - TPS65219_BUCK_REG_1V4) * TPS65219_VOLT_STEP_100MV;
> >> +	else if (val >= TPS65219_BUCK_REG_0V6)
> >> +		return TPS65219_BUCK_0V6 + val * TPS65219_VOLT_STEP_25MV;
> >>   	else
> >>   		return -EINVAL;
> >>   }
> >> @@ -161,7 +161,7 @@ static int tps65219_ldo_volt2val(int idx, int uV)
> >>   	if (uV > max)
> >>   		return -EINVAL;
> >>   	else if (uV >= base)
> >> -		return (uV - TPS65219_LDO12_VOLT_MIN) / 50000;
> >> +		return (uV - TPS65219_LDO12_VOLT_MIN) / TPS65219_VOLT_STEP_50MV;
> >>   	else
> >>   		return -EINVAL;
> >>   }
> >> @@ -187,7 +187,7 @@ static int tps65219_ldo_val2volt(int idx, int val)
> >>   	else if (val <= reg_base)
> >>   		return base;
> >>   	else if (val >= 0)
> >> -		return TPS65219_LDO12_VOLT_MIN + (50000 * val);
> >> +		return TPS65219_LDO12_VOLT_MIN + (TPS65219_VOLT_STEP_50MV * val);
> >>   	else
> >>   		return -EINVAL;
> >>   }
> >> diff --git a/include/power/tps65219.h b/include/power/tps65219.h
> >> index aa81b92266fd..e8780af2d811 100644
> >> --- a/include/power/tps65219.h
> >> +++ b/include/power/tps65219.h
> >> @@ -17,10 +17,20 @@
> >>   #define TPS65219_BUCK_DRIVER		"tps65219_buck"
> >>
> >>   #define TPS65219_VOLT_MASK		0x3F
> >> -#define TPS65219_BUCK_VOLT_MAX		3400000
> >> -
> >>   #define TPS65219_ENABLE_CTRL_REG	0x2
> >>
> >> +#define TPS65219_VOLT_STEP_25MV		25000
> >> +#define TPS65219_VOLT_STEP_50MV		50000
> >> +#define TPS65219_VOLT_STEP_100MV	100000
> >> +
> >> +#define TPS65219_BUCK_0V6		600000
> >> +#define TPS65219_BUCK_1V4		1400000
> >> +#define TPS65219_BUCK_3V4		3400000
> >> +
> >> +#define TPS65219_BUCK_REG_0V6		0x00
> >> +#define TPS65219_BUCK_REG_1V4		0x20
> >> +#define TPS65219_BUCK_REG_3V4		0x34
> >> +
> >>   #define TPS65219_BUCK1_VOUT_REG		0xa
> >>   #define TPS65219_BUCK2_VOUT_REG		0x9
> >>   #define TPS65219_BUCK3_VOUT_REG		0x8
> >> --
> >> 2.34.1
> >
> >
>
> --
> Best,
> Shree Ramamoorthy
> PMIC Software Engineer

Re: [PATCH v1 2/2] power: replace magic numbers with macros

[Shree Ramamoorthy]

Hi,

On 12/18/24 5:26 PM, Jaehoon Chung wrote:
>
>> -----Original Message-----
>> From: Shree Ramamoorthy <s-ramamoorthy@ti.com>
>> Sent: Thursday, December 19, 2024 7:55 AM
>>
>> Hi.
>>
>>
>> On 12/18/24 4:44 PM, Jaehoon Chung wrote:
>>> Hi,
>>>
>>>> -----Original Message-----
>>>> From: Shree Ramamoorthy <s-ramamoorthy@ti.com>
>>>> Sent: Thursday, December 19, 2024 2:13 AM
>>>>
>>>> Replace magic numbers in buckval2votl() & buckvolt2val() with macros to
>>>> help with clarity and correlate what the numbers correspond to in the
>>>> TPS65219 datasheet.
>>>>
>>>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
>>>> ---
>>>>    drivers/power/regulator/tps65219_regulator.c | 26 ++++++++++----------
>>>>    include/power/tps65219.h                     | 14 +++++++++--
>>>>    2 files changed, 25 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/power/regulator/tps65219_regulator.c
>>>> b/drivers/power/regulator/tps65219_regulator.c
>>>> index 4b0fb205909a..88abc896b3a2 100644
>>>> --- a/drivers/power/regulator/tps65219_regulator.c
>>>> +++ b/drivers/power/regulator/tps65219_regulator.c
>>>> @@ -72,12 +72,12 @@ static int tps65219_buck_enable(struct udevice *dev, int op, bool *enable)
>>>>
>>>>    static int tps65219_buck_volt2val(int uV)
>>>>    {
>>>> -	if (uV > TPS65219_BUCK_VOLT_MAX)
>>>> +	if (uV > TPS65219_BUCK_3V4)
>>>>    		return -EINVAL;
>>>> -	else if (uV >= 1400000)
>>>> -		return (uV - 1400000) / 100000 + 0x20;
>>>> -	else if (uV >= 600000)
>>>> -		return (uV - 600000) / 25000 + 0x00;
>>>> +	else if (uV >= TPS65219_BUCK_1V4)
>>>> +		return (uV - TPS65219_BUCK_1V4) / TPS65219_VOLT_STEP_100MV + TPS65219_BUCK_REG_1V4;
>>> Even though Not relevant to this subject. If uV is 340000, the return value is correct?
>>>
>>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>>>
>>> Best Regards,
>>> Jaehoon Chung
>> Thank you for reviewing!
>> The allowed max uV is 3.4V inclusive.
>> If 340000 uV is detected, the first 'else if' statement should be taken.
> Ah, I missed wrong read. Thanks for checking. If you can do, other tpsXXX can also be changed to readable macro.
>
> Best Regards,
> Jaehoon Chung

Absolutely, I will make a note to work on this. Besides tps6594, are there any specific tpsXXX drivers you had in mind?
Or work on patches for any TI PMIC tpsXXX U-Boot drivers that include magic numbers?

>> Best,
>> Shree
>>
>>>> +	else if (uV >= TPS65219_BUCK_0V6)
>>>> +		return (uV - TPS65219_BUCK_0V6) / TPS65219_VOLT_STEP_25MV + TPS65219_BUCK_REG_0V6;
>>>>    	else
>>>>    		return -EINVAL;
>>>>    }
>>>> @@ -86,12 +86,12 @@ static int tps65219_buck_val2volt(int val)
>>>>    {
>>>>    	if (val > TPS65219_VOLT_MASK)
>>>>    		return -EINVAL;
>>>> -	else if (val > 0x34)
>>>> -		return TPS65219_BUCK_VOLT_MAX;
>>>> -	else if (val > 0x20)
>>>> -		return 1400000 + (val - 0x20) * 100000;
>>>> -	else if (val >= 0)
>>>> -		return 600000 + val * 25000;
>>>> +	else if (val > TPS65219_BUCK_REG_3V4)
>>>> +		return TPS65219_BUCK_3V4;
>>>> +	else if (val > TPS65219_BUCK_REG_1V4)
>>>> +		return TPS65219_BUCK_1V4 + (val - TPS65219_BUCK_REG_1V4) * TPS65219_VOLT_STEP_100MV;
>>>> +	else if (val >= TPS65219_BUCK_REG_0V6)
>>>> +		return TPS65219_BUCK_0V6 + val * TPS65219_VOLT_STEP_25MV;
>>>>    	else
>>>>    		return -EINVAL;
>>>>    }
>>>> @@ -161,7 +161,7 @@ static int tps65219_ldo_volt2val(int idx, int uV)
>>>>    	if (uV > max)
>>>>    		return -EINVAL;
>>>>    	else if (uV >= base)
>>>> -		return (uV - TPS65219_LDO12_VOLT_MIN) / 50000;
>>>> +		return (uV - TPS65219_LDO12_VOLT_MIN) / TPS65219_VOLT_STEP_50MV;
>>>>    	else
>>>>    		return -EINVAL;
>>>>    }
>>>> @@ -187,7 +187,7 @@ static int tps65219_ldo_val2volt(int idx, int val)
>>>>    	else if (val <= reg_base)
>>>>    		return base;
>>>>    	else if (val >= 0)
>>>> -		return TPS65219_LDO12_VOLT_MIN + (50000 * val);
>>>> +		return TPS65219_LDO12_VOLT_MIN + (TPS65219_VOLT_STEP_50MV * val);
>>>>    	else
>>>>    		return -EINVAL;
>>>>    }
>>>> diff --git a/include/power/tps65219.h b/include/power/tps65219.h
>>>> index aa81b92266fd..e8780af2d811 100644
>>>> --- a/include/power/tps65219.h
>>>> +++ b/include/power/tps65219.h
>>>> @@ -17,10 +17,20 @@
>>>>    #define TPS65219_BUCK_DRIVER		"tps65219_buck"
>>>>
>>>>    #define TPS65219_VOLT_MASK		0x3F
>>>> -#define TPS65219_BUCK_VOLT_MAX		3400000
>>>> -
>>>>    #define TPS65219_ENABLE_CTRL_REG	0x2
>>>>
>>>> +#define TPS65219_VOLT_STEP_25MV		25000
>>>> +#define TPS65219_VOLT_STEP_50MV		50000
>>>> +#define TPS65219_VOLT_STEP_100MV	100000
>>>> +
>>>> +#define TPS65219_BUCK_0V6		600000
>>>> +#define TPS65219_BUCK_1V4		1400000
>>>> +#define TPS65219_BUCK_3V4		3400000
>>>> +
>>>> +#define TPS65219_BUCK_REG_0V6		0x00
>>>> +#define TPS65219_BUCK_REG_1V4		0x20
>>>> +#define TPS65219_BUCK_REG_3V4		0x34
>>>> +
>>>>    #define TPS65219_BUCK1_VOUT_REG		0xa
>>>>    #define TPS65219_BUCK2_VOUT_REG		0x9
>>>>    #define TPS65219_BUCK3_VOUT_REG		0x8
>>>> --
>>>> 2.34.1
>>>
>> --
>> Best,
>> Shree Ramamoorthy
>> PMIC Software Engineer
>
>

RE: [PATCH v1 2/2] power: replace magic numbers with macros

[Jaehoon Chung]

> -----Original Message-----
> From: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> Sent: Friday, December 20, 2024 2:47 AM
>
> Hi,
>
> On 12/18/24 5:26 PM, Jaehoon Chung wrote:
> >
> >> -----Original Message-----
> >> From: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> >> Sent: Thursday, December 19, 2024 7:55 AM
> >>
> >> Hi.
> >>
> >>
> >> On 12/18/24 4:44 PM, Jaehoon Chung wrote:
> >>> Hi,
> >>>
> >>>> -----Original Message-----
> >>>> From: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> >>>> Sent: Thursday, December 19, 2024 2:13 AM
> >>>>
> >>>> Replace magic numbers in buckval2votl() & buckvolt2val() with macros to
> >>>> help with clarity and correlate what the numbers correspond to in the
> >>>> TPS65219 datasheet.
> >>>>
> >>>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> >>>> ---
> >>>>    drivers/power/regulator/tps65219_regulator.c | 26 ++++++++++----------
> >>>>    include/power/tps65219.h                     | 14 +++++++++--
> >>>>    2 files changed, 25 insertions(+), 15 deletions(-)
> >>>>
> >>>> diff --git a/drivers/power/regulator/tps65219_regulator.c
> >>>> b/drivers/power/regulator/tps65219_regulator.c
> >>>> index 4b0fb205909a..88abc896b3a2 100644
> >>>> --- a/drivers/power/regulator/tps65219_regulator.c
> >>>> +++ b/drivers/power/regulator/tps65219_regulator.c
> >>>> @@ -72,12 +72,12 @@ static int tps65219_buck_enable(struct udevice *dev, int op, bool *enable)
> >>>>
> >>>>    static int tps65219_buck_volt2val(int uV)
> >>>>    {
> >>>> -	if (uV > TPS65219_BUCK_VOLT_MAX)
> >>>> +	if (uV > TPS65219_BUCK_3V4)
> >>>>    		return -EINVAL;
> >>>> -	else if (uV >= 1400000)
> >>>> -		return (uV - 1400000) / 100000 + 0x20;
> >>>> -	else if (uV >= 600000)
> >>>> -		return (uV - 600000) / 25000 + 0x00;
> >>>> +	else if (uV >= TPS65219_BUCK_1V4)
> >>>> +		return (uV - TPS65219_BUCK_1V4) / TPS65219_VOLT_STEP_100MV + TPS65219_BUCK_REG_1V4;
> >>> Even though Not relevant to this subject. If uV is 340000, the return value is correct?
> >>>
> >>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> >>>
> >>> Best Regards,
> >>> Jaehoon Chung
> >> Thank you for reviewing!
> >> The allowed max uV is 3.4V inclusive.
> >> If 340000 uV is detected, the first 'else if' statement should be taken.
> > Ah, I missed wrong read. Thanks for checking. If you can do, other tpsXXX can also be changed to
> readable macro.
> >
> > Best Regards,
> > Jaehoon Chung
>
> Absolutely, I will make a note to work on this. Besides tps6594, are there any specific tpsXXX drivers
> you had in mind?
> Or work on patches for any TI PMIC tpsXXX U-Boot drivers that include magic numbers?

I didn't check the entire tpsXXX drivers in more detail. But I don't like using magic numbers. :)
It's a difficult to know what purpose its value is.

Best Regards,
Jaehoon Chung

>
> >> Best,
> >> Shree
> >>
> >>>> +	else if (uV >= TPS65219_BUCK_0V6)
> >>>> +		return (uV - TPS65219_BUCK_0V6) / TPS65219_VOLT_STEP_25MV + TPS65219_BUCK_REG_0V6;
> >>>>    	else
> >>>>    		return -EINVAL;
> >>>>    }
> >>>> @@ -86,12 +86,12 @@ static int tps65219_buck_val2volt(int val)
> >>>>    {
> >>>>    	if (val > TPS65219_VOLT_MASK)
> >>>>    		return -EINVAL;
> >>>> -	else if (val > 0x34)
> >>>> -		return TPS65219_BUCK_VOLT_MAX;
> >>>> -	else if (val > 0x20)
> >>>> -		return 1400000 + (val - 0x20) * 100000;
> >>>> -	else if (val >= 0)
> >>>> -		return 600000 + val * 25000;
> >>>> +	else if (val > TPS65219_BUCK_REG_3V4)
> >>>> +		return TPS65219_BUCK_3V4;
> >>>> +	else if (val > TPS65219_BUCK_REG_1V4)
> >>>> +		return TPS65219_BUCK_1V4 + (val - TPS65219_BUCK_REG_1V4) * TPS65219_VOLT_STEP_100MV;
> >>>> +	else if (val >= TPS65219_BUCK_REG_0V6)
> >>>> +		return TPS65219_BUCK_0V6 + val * TPS65219_VOLT_STEP_25MV;
> >>>>    	else
> >>>>    		return -EINVAL;
> >>>>    }
> >>>> @@ -161,7 +161,7 @@ static int tps65219_ldo_volt2val(int idx, int uV)
> >>>>    	if (uV > max)
> >>>>    		return -EINVAL;
> >>>>    	else if (uV >= base)
> >>>> -		return (uV - TPS65219_LDO12_VOLT_MIN) / 50000;
> >>>> +		return (uV - TPS65219_LDO12_VOLT_MIN) / TPS65219_VOLT_STEP_50MV;
> >>>>    	else
> >>>>    		return -EINVAL;
> >>>>    }
> >>>> @@ -187,7 +187,7 @@ static int tps65219_ldo_val2volt(int idx, int val)
> >>>>    	else if (val <= reg_base)
> >>>>    		return base;
> >>>>    	else if (val >= 0)
> >>>> -		return TPS65219_LDO12_VOLT_MIN + (50000 * val);
> >>>> +		return TPS65219_LDO12_VOLT_MIN + (TPS65219_VOLT_STEP_50MV * val);
> >>>>    	else
> >>>>    		return -EINVAL;
> >>>>    }
> >>>> diff --git a/include/power/tps65219.h b/include/power/tps65219.h
> >>>> index aa81b92266fd..e8780af2d811 100644
> >>>> --- a/include/power/tps65219.h
> >>>> +++ b/include/power/tps65219.h
> >>>> @@ -17,10 +17,20 @@
> >>>>    #define TPS65219_BUCK_DRIVER		"tps65219_buck"
> >>>>
> >>>>    #define TPS65219_VOLT_MASK		0x3F
> >>>> -#define TPS65219_BUCK_VOLT_MAX		3400000
> >>>> -
> >>>>    #define TPS65219_ENABLE_CTRL_REG	0x2
> >>>>
> >>>> +#define TPS65219_VOLT_STEP_25MV		25000
> >>>> +#define TPS65219_VOLT_STEP_50MV		50000
> >>>> +#define TPS65219_VOLT_STEP_100MV	100000
> >>>> +
> >>>> +#define TPS65219_BUCK_0V6		600000
> >>>> +#define TPS65219_BUCK_1V4		1400000
> >>>> +#define TPS65219_BUCK_3V4		3400000
> >>>> +
> >>>> +#define TPS65219_BUCK_REG_0V6		0x00
> >>>> +#define TPS65219_BUCK_REG_1V4		0x20
> >>>> +#define TPS65219_BUCK_REG_3V4		0x34
> >>>> +
> >>>>    #define TPS65219_BUCK1_VOUT_REG		0xa
> >>>>    #define TPS65219_BUCK2_VOUT_REG		0x9
> >>>>    #define TPS65219_BUCK3_VOUT_REG		0x8
> >>>> --
> >>>> 2.34.1
> >>>
> >> --
> >> Best,
> >> Shree Ramamoorthy
> >> PMIC Software Engineer
> >
> >