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

["Jaehoon Chung" <jh80.chung@samsung.com>]

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