[PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines

[PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines

[Axel Lin]

The AXP20X_xxx_START/END/STEPS defines make the code hard to read and
very hard to check the linear range settings because it needs to check
the defines one-by-one.
The original code without the defines is very good in readability
as the meaning of each field of REGULATOR_LINEAR_RANGE is clear.
So I suggest to get rid of AXP20X_xxx_START/END/STEPS defines.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/regulator/axp20x-regulator.c | 176 +++------------------------
 1 file changed, 19 insertions(+), 157 deletions(-)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index fba8f58ab769..46b5de53d1a3 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -64,26 +64,6 @@
 #define AXP20X_DCDC2_LDO3_V_RAMP_LDO3_EN_MASK		BIT_MASK(3)
 #define AXP20X_DCDC2_LDO3_V_RAMP_LDO3_EN		BIT(3)
 
-#define AXP20X_LDO4_V_OUT_1250mV_START	0x0
-#define AXP20X_LDO4_V_OUT_1250mV_STEPS	0
-#define AXP20X_LDO4_V_OUT_1250mV_END	\
-	(AXP20X_LDO4_V_OUT_1250mV_START + AXP20X_LDO4_V_OUT_1250mV_STEPS)
-#define AXP20X_LDO4_V_OUT_1300mV_START	0x1
-#define AXP20X_LDO4_V_OUT_1300mV_STEPS	7
-#define AXP20X_LDO4_V_OUT_1300mV_END	\
-	(AXP20X_LDO4_V_OUT_1300mV_START + AXP20X_LDO4_V_OUT_1300mV_STEPS)
-#define AXP20X_LDO4_V_OUT_2500mV_START	0x9
-#define AXP20X_LDO4_V_OUT_2500mV_STEPS	0
-#define AXP20X_LDO4_V_OUT_2500mV_END	\
-	(AXP20X_LDO4_V_OUT_2500mV_START + AXP20X_LDO4_V_OUT_2500mV_STEPS)
-#define AXP20X_LDO4_V_OUT_2700mV_START	0xa
-#define AXP20X_LDO4_V_OUT_2700mV_STEPS	1
-#define AXP20X_LDO4_V_OUT_2700mV_END	\
-	(AXP20X_LDO4_V_OUT_2700mV_START + AXP20X_LDO4_V_OUT_2700mV_STEPS)
-#define AXP20X_LDO4_V_OUT_3000mV_START	0xc
-#define AXP20X_LDO4_V_OUT_3000mV_STEPS	3
-#define AXP20X_LDO4_V_OUT_3000mV_END	\
-	(AXP20X_LDO4_V_OUT_3000mV_START + AXP20X_LDO4_V_OUT_3000mV_STEPS)
 #define AXP20X_LDO4_V_OUT_NUM_VOLTAGES	16
 
 #define AXP22X_IO_ENABLED		0x03
@@ -156,44 +136,9 @@
 #define AXP803_DCDC23_POLYPHASE_DUAL	BIT(6)
 #define AXP803_DCDC56_POLYPHASE_DUAL	BIT(5)
 
-#define AXP803_DCDC234_500mV_START	0x00
-#define AXP803_DCDC234_500mV_STEPS	70
-#define AXP803_DCDC234_500mV_END	\
-	(AXP803_DCDC234_500mV_START + AXP803_DCDC234_500mV_STEPS)
-#define AXP803_DCDC234_1220mV_START	0x47
-#define AXP803_DCDC234_1220mV_STEPS	4
-#define AXP803_DCDC234_1220mV_END	\
-	(AXP803_DCDC234_1220mV_START + AXP803_DCDC234_1220mV_STEPS)
 #define AXP803_DCDC234_NUM_VOLTAGES	76
-
-#define AXP803_DCDC5_800mV_START	0x00
-#define AXP803_DCDC5_800mV_STEPS	32
-#define AXP803_DCDC5_800mV_END		\
-	(AXP803_DCDC5_800mV_START + AXP803_DCDC5_800mV_STEPS)
-#define AXP803_DCDC5_1140mV_START	0x21
-#define AXP803_DCDC5_1140mV_STEPS	35
-#define AXP803_DCDC5_1140mV_END		\
-	(AXP803_DCDC5_1140mV_START + AXP803_DCDC5_1140mV_STEPS)
 #define AXP803_DCDC5_NUM_VOLTAGES	68
-
-#define AXP803_DCDC6_600mV_START	0x00
-#define AXP803_DCDC6_600mV_STEPS	50
-#define AXP803_DCDC6_600mV_END		\
-	(AXP803_DCDC6_600mV_START + AXP803_DCDC6_600mV_STEPS)
-#define AXP803_DCDC6_1120mV_START	0x33
-#define AXP803_DCDC6_1120mV_STEPS	14
-#define AXP803_DCDC6_1120mV_END		\
-	(AXP803_DCDC6_1120mV_START + AXP803_DCDC6_1120mV_STEPS)
 #define AXP803_DCDC6_NUM_VOLTAGES	72
-
-#define AXP803_DLDO2_700mV_START	0x00
-#define AXP803_DLDO2_700mV_STEPS	26
-#define AXP803_DLDO2_700mV_END		\
-	(AXP803_DLDO2_700mV_START + AXP803_DLDO2_700mV_STEPS)
-#define AXP803_DLDO2_3400mV_START	0x1b
-#define AXP803_DLDO2_3400mV_STEPS	4
-#define AXP803_DLDO2_3400mV_END		\
-	(AXP803_DLDO2_3400mV_START + AXP803_DLDO2_3400mV_STEPS)
 #define AXP803_DLDO2_NUM_VOLTAGES	32
 
 #define AXP806_DCDCA_V_CTRL_MASK	GENMASK(6, 0)
@@ -235,34 +180,8 @@
 
 #define AXP806_DCDCDE_POLYPHASE_DUAL	BIT(5)
 
-#define AXP806_DCDCA_600mV_START	0x00
-#define AXP806_DCDCA_600mV_STEPS	50
-#define AXP806_DCDCA_600mV_END		\
-	(AXP806_DCDCA_600mV_START + AXP806_DCDCA_600mV_STEPS)
-#define AXP806_DCDCA_1120mV_START	0x33
-#define AXP806_DCDCA_1120mV_STEPS	14
-#define AXP806_DCDCA_1120mV_END		\
-	(AXP806_DCDCA_1120mV_START + AXP806_DCDCA_1120mV_STEPS)
 #define AXP806_DCDCA_NUM_VOLTAGES	72
-
-#define AXP806_DCDCD_600mV_START	0x00
-#define AXP806_DCDCD_600mV_STEPS	45
-#define AXP806_DCDCD_600mV_END		\
-	(AXP806_DCDCD_600mV_START + AXP806_DCDCD_600mV_STEPS)
-#define AXP806_DCDCD_1600mV_START	0x2e
-#define AXP806_DCDCD_1600mV_STEPS	17
-#define AXP806_DCDCD_1600mV_END		\
-	(AXP806_DCDCD_1600mV_START + AXP806_DCDCD_1600mV_STEPS)
 #define AXP806_DCDCD_NUM_VOLTAGES	64
-
-#define AXP809_DCDC4_600mV_START	0x00
-#define AXP809_DCDC4_600mV_STEPS	47
-#define AXP809_DCDC4_600mV_END		\
-	(AXP809_DCDC4_600mV_START + AXP809_DCDC4_600mV_STEPS)
-#define AXP809_DCDC4_1800mV_START	0x30
-#define AXP809_DCDC4_1800mV_STEPS	8
-#define AXP809_DCDC4_1800mV_END		\
-	(AXP809_DCDC4_1800mV_START + AXP809_DCDC4_1800mV_STEPS)
 #define AXP809_DCDC4_NUM_VOLTAGES	57
 
 #define AXP813_DCDC7_V_OUT_MASK		GENMASK(6, 0)
@@ -517,26 +436,11 @@ static const struct regulator_ops axp20x_ops_sw = {
 };
 
 static const struct regulator_linear_range axp20x_ldo4_ranges[] = {
-	REGULATOR_LINEAR_RANGE(1250000,
-			       AXP20X_LDO4_V_OUT_1250mV_START,
-			       AXP20X_LDO4_V_OUT_1250mV_END,
-			       0),
-	REGULATOR_LINEAR_RANGE(1300000,
-			       AXP20X_LDO4_V_OUT_1300mV_START,
-			       AXP20X_LDO4_V_OUT_1300mV_END,
-			       100000),
-	REGULATOR_LINEAR_RANGE(2500000,
-			       AXP20X_LDO4_V_OUT_2500mV_START,
-			       AXP20X_LDO4_V_OUT_2500mV_END,
-			       0),
-	REGULATOR_LINEAR_RANGE(2700000,
-			       AXP20X_LDO4_V_OUT_2700mV_START,
-			       AXP20X_LDO4_V_OUT_2700mV_END,
-			       100000),
-	REGULATOR_LINEAR_RANGE(3000000,
-			       AXP20X_LDO4_V_OUT_3000mV_START,
-			       AXP20X_LDO4_V_OUT_3000mV_END,
-			       100000),
+	REGULATOR_LINEAR_RANGE(1250000, 0x0, 0x0, 0),
+	REGULATOR_LINEAR_RANGE(1300000, 0x1, 0x8, 100000),
+	REGULATOR_LINEAR_RANGE(2500000, 0x9, 0x9, 0),
+	REGULATOR_LINEAR_RANGE(2700000, 0xa, 0xb, 100000),
+	REGULATOR_LINEAR_RANGE(3000000, 0xc, 0xf, 100000),
 };
 
 static const struct regulator_desc axp20x_regulators[] = {
@@ -645,48 +549,24 @@ static const struct regulator_desc axp22x_drivevbus_regulator = {
 
 /* DCDC ranges shared with AXP813 */
 static const struct regulator_linear_range axp803_dcdc234_ranges[] = {
-	REGULATOR_LINEAR_RANGE(500000,
-			       AXP803_DCDC234_500mV_START,
-			       AXP803_DCDC234_500mV_END,
-			       10000),
-	REGULATOR_LINEAR_RANGE(1220000,
-			       AXP803_DCDC234_1220mV_START,
-			       AXP803_DCDC234_1220mV_END,
-			       20000),
+	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
+	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x4b, 20000),
 };
 
 static const struct regulator_linear_range axp803_dcdc5_ranges[] = {
-	REGULATOR_LINEAR_RANGE(800000,
-			       AXP803_DCDC5_800mV_START,
-			       AXP803_DCDC5_800mV_END,
-			       10000),
-	REGULATOR_LINEAR_RANGE(1140000,
-			       AXP803_DCDC5_1140mV_START,
-			       AXP803_DCDC5_1140mV_END,
-			       20000),
+	REGULATOR_LINEAR_RANGE(800000, 0x0, 0x20, 10000),
+	REGULATOR_LINEAR_RANGE(1140000, 0x21, 0x44, 20000),
 };
 
 static const struct regulator_linear_range axp803_dcdc6_ranges[] = {
-	REGULATOR_LINEAR_RANGE(600000,
-			       AXP803_DCDC6_600mV_START,
-			       AXP803_DCDC6_600mV_END,
-			       10000),
-	REGULATOR_LINEAR_RANGE(1120000,
-			       AXP803_DCDC6_1120mV_START,
-			       AXP803_DCDC6_1120mV_END,
-			       20000),
+	REGULATOR_LINEAR_RANGE(600000, 0x0, 0x32, 10000),
+	REGULATOR_LINEAR_RANGE(1120000, 0x33, 0x47, 20000),
 };
 
 /* AXP806's CLDO2 and AXP809's DLDO1 share the same range */
 static const struct regulator_linear_range axp803_dldo2_ranges[] = {
-	REGULATOR_LINEAR_RANGE(700000,
-			       AXP803_DLDO2_700mV_START,
-			       AXP803_DLDO2_700mV_END,
-			       100000),
-	REGULATOR_LINEAR_RANGE(3400000,
-			       AXP803_DLDO2_3400mV_START,
-			       AXP803_DLDO2_3400mV_END,
-			       200000),
+	REGULATOR_LINEAR_RANGE(700000, 0x0, 0x1a, 100000),
+	REGULATOR_LINEAR_RANGE(3400000, 0x1b, 0x1f, 200000),
 };
 
 static const struct regulator_desc axp803_regulators[] = {
@@ -765,25 +645,13 @@ static const struct regulator_desc axp803_regulators[] = {
 };
 
 static const struct regulator_linear_range axp806_dcdca_ranges[] = {
-	REGULATOR_LINEAR_RANGE(600000,
-			       AXP806_DCDCA_600mV_START,
-			       AXP806_DCDCA_600mV_END,
-			       10000),
-	REGULATOR_LINEAR_RANGE(1120000,
-			       AXP806_DCDCA_1120mV_START,
-			       AXP806_DCDCA_1120mV_END,
-			       20000),
+	REGULATOR_LINEAR_RANGE(600000, 0x0, 0x32, 10000),
+	REGULATOR_LINEAR_RANGE(1120000, 0x33, 0x47, 20000),
 };
 
 static const struct regulator_linear_range axp806_dcdcd_ranges[] = {
-	REGULATOR_LINEAR_RANGE(600000,
-			       AXP806_DCDCD_600mV_START,
-			       AXP806_DCDCD_600mV_END,
-			       20000),
-	REGULATOR_LINEAR_RANGE(1600000,
-			       AXP806_DCDCD_600mV_START,
-			       AXP806_DCDCD_600mV_END,
-			       100000),
+	REGULATOR_LINEAR_RANGE(600000, 0x0, 0x2d, 20000),
+	REGULATOR_LINEAR_RANGE(1600000, 0x2e, 0x3f, 100000),
 };
 
 static const struct regulator_desc axp806_regulators[] = {
@@ -841,14 +709,8 @@ static const struct regulator_desc axp806_regulators[] = {
 };
 
 static const struct regulator_linear_range axp809_dcdc4_ranges[] = {
-	REGULATOR_LINEAR_RANGE(600000,
-			       AXP809_DCDC4_600mV_START,
-			       AXP809_DCDC4_600mV_END,
-			       20000),
-	REGULATOR_LINEAR_RANGE(1800000,
-			       AXP809_DCDC4_1800mV_START,
-			       AXP809_DCDC4_1800mV_END,
-			       100000),
+	REGULATOR_LINEAR_RANGE(600000, 0x0, 0x2f, 20000),
+	REGULATOR_LINEAR_RANGE(1800000, 0x30, 0x38, 100000),
 };
 
 static const struct regulator_desc axp809_regulators[] = {
-- 
2.17.1

Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines

[Olliver Schinagl]

Hey Axel,

On February 20, 2019 5:50:13 PM GMT+01:00, Axel Lin <axel.lin@ingics.com> wrote:
>The AXP20X_xxx_START/END/STEPS defines make the code hard to read and
>very hard to check the linear range settings because it needs to check
>the defines one-by-one.
>The original code without the defines is very good in readability
>as the meaning of each field of REGULATOR_LINEAR_RANGE is clear.
>So I suggest to get rid of AXP20X_xxx_START/END/STEPS defines.
Are you suggesting that magic values and hex numbers are more readable?

Maybe you can explain what your problem is, as it appears you are trying to debug something?

>
>Signed-off-by: Axel Lin <axel.lin@ingics.com>
>---
> drivers/regulator/axp20x-regulator.c | 176 +++------------------------
> 1 file changed, 19 insertions(+), 157 deletions(-)
>
>diff --git a/drivers/regulator/axp20x-regulator.c
>b/drivers/regulator/axp20x-regulator.c
>index fba8f58ab769..46b5de53d1a3 100644
>--- a/drivers/regulator/axp20x-regulator.c
>+++ b/drivers/regulator/axp20x-regulator.c
>@@ -64,26 +64,6 @@
> #define AXP20X_DCDC2_LDO3_V_RAMP_LDO3_EN_MASK		BIT_MASK(3)
> #define AXP20X_DCDC2_LDO3_V_RAMP_LDO3_EN		BIT(3)
> 
>-#define AXP20X_LDO4_V_OUT_1250mV_START	0x0
>-#define AXP20X_LDO4_V_OUT_1250mV_STEPS	0
>-#define AXP20X_LDO4_V_OUT_1250mV_END	\
>-	(AXP20X_LDO4_V_OUT_1250mV_START + AXP20X_LDO4_V_OUT_1250mV_STEPS)
>-#define AXP20X_LDO4_V_OUT_1300mV_START	0x1
>-#define AXP20X_LDO4_V_OUT_1300mV_STEPS	7
>-#define AXP20X_LDO4_V_OUT_1300mV_END	\
>-	(AXP20X_LDO4_V_OUT_1300mV_START + AXP20X_LDO4_V_OUT_1300mV_STEPS)
>-#define AXP20X_LDO4_V_OUT_2500mV_START	0x9
>-#define AXP20X_LDO4_V_OUT_2500mV_STEPS	0
>-#define AXP20X_LDO4_V_OUT_2500mV_END	\
>-	(AXP20X_LDO4_V_OUT_2500mV_START + AXP20X_LDO4_V_OUT_2500mV_STEPS)
>-#define AXP20X_LDO4_V_OUT_2700mV_START	0xa
>-#define AXP20X_LDO4_V_OUT_2700mV_STEPS	1
>-#define AXP20X_LDO4_V_OUT_2700mV_END	\
>-	(AXP20X_LDO4_V_OUT_2700mV_START + AXP20X_LDO4_V_OUT_2700mV_STEPS)
>-#define AXP20X_LDO4_V_OUT_3000mV_START	0xc
>-#define AXP20X_LDO4_V_OUT_3000mV_STEPS	3
>-#define AXP20X_LDO4_V_OUT_3000mV_END	\
>-	(AXP20X_LDO4_V_OUT_3000mV_START + AXP20X_LDO4_V_OUT_3000mV_STEPS)
> #define AXP20X_LDO4_V_OUT_NUM_VOLTAGES	16
> 
> #define AXP22X_IO_ENABLED		0x03
>@@ -156,44 +136,9 @@
> #define AXP803_DCDC23_POLYPHASE_DUAL	BIT(6)
> #define AXP803_DCDC56_POLYPHASE_DUAL	BIT(5)
> 
>-#define AXP803_DCDC234_500mV_START	0x00
>-#define AXP803_DCDC234_500mV_STEPS	70
>-#define AXP803_DCDC234_500mV_END	\
>-	(AXP803_DCDC234_500mV_START + AXP803_DCDC234_500mV_STEPS)
>-#define AXP803_DCDC234_1220mV_START	0x47
>-#define AXP803_DCDC234_1220mV_STEPS	4
>-#define AXP803_DCDC234_1220mV_END	\
>-	(AXP803_DCDC234_1220mV_START + AXP803_DCDC234_1220mV_STEPS)
> #define AXP803_DCDC234_NUM_VOLTAGES	76
>-
>-#define AXP803_DCDC5_800mV_START	0x00
>-#define AXP803_DCDC5_800mV_STEPS	32
>-#define AXP803_DCDC5_800mV_END		\
>-	(AXP803_DCDC5_800mV_START + AXP803_DCDC5_800mV_STEPS)
>-#define AXP803_DCDC5_1140mV_START	0x21
>-#define AXP803_DCDC5_1140mV_STEPS	35
>-#define AXP803_DCDC5_1140mV_END		\
>-	(AXP803_DCDC5_1140mV_START + AXP803_DCDC5_1140mV_STEPS)
> #define AXP803_DCDC5_NUM_VOLTAGES	68
>-
>-#define AXP803_DCDC6_600mV_START	0x00
>-#define AXP803_DCDC6_600mV_STEPS	50
>-#define AXP803_DCDC6_600mV_END		\
>-	(AXP803_DCDC6_600mV_START + AXP803_DCDC6_600mV_STEPS)
>-#define AXP803_DCDC6_1120mV_START	0x33
>-#define AXP803_DCDC6_1120mV_STEPS	14
>-#define AXP803_DCDC6_1120mV_END		\
>-	(AXP803_DCDC6_1120mV_START + AXP803_DCDC6_1120mV_STEPS)
> #define AXP803_DCDC6_NUM_VOLTAGES	72
>-
>-#define AXP803_DLDO2_700mV_START	0x00
>-#define AXP803_DLDO2_700mV_STEPS	26
>-#define AXP803_DLDO2_700mV_END		\
>-	(AXP803_DLDO2_700mV_START + AXP803_DLDO2_700mV_STEPS)
>-#define AXP803_DLDO2_3400mV_START	0x1b
>-#define AXP803_DLDO2_3400mV_STEPS	4
>-#define AXP803_DLDO2_3400mV_END		\
>-	(AXP803_DLDO2_3400mV_START + AXP803_DLDO2_3400mV_STEPS)
> #define AXP803_DLDO2_NUM_VOLTAGES	32
> 
> #define AXP806_DCDCA_V_CTRL_MASK	GENMASK(6, 0)
>@@ -235,34 +180,8 @@
> 
> #define AXP806_DCDCDE_POLYPHASE_DUAL	BIT(5)
> 
>-#define AXP806_DCDCA_600mV_START	0x00
>-#define AXP806_DCDCA_600mV_STEPS	50
>-#define AXP806_DCDCA_600mV_END		\
>-	(AXP806_DCDCA_600mV_START + AXP806_DCDCA_600mV_STEPS)
>-#define AXP806_DCDCA_1120mV_START	0x33
>-#define AXP806_DCDCA_1120mV_STEPS	14
>-#define AXP806_DCDCA_1120mV_END		\
>-	(AXP806_DCDCA_1120mV_START + AXP806_DCDCA_1120mV_STEPS)
> #define AXP806_DCDCA_NUM_VOLTAGES	72
>-
>-#define AXP806_DCDCD_600mV_START	0x00
>-#define AXP806_DCDCD_600mV_STEPS	45
>-#define AXP806_DCDCD_600mV_END		\
>-	(AXP806_DCDCD_600mV_START + AXP806_DCDCD_600mV_STEPS)
>-#define AXP806_DCDCD_1600mV_START	0x2e
>-#define AXP806_DCDCD_1600mV_STEPS	17
>-#define AXP806_DCDCD_1600mV_END		\
>-	(AXP806_DCDCD_1600mV_START + AXP806_DCDCD_1600mV_STEPS)
> #define AXP806_DCDCD_NUM_VOLTAGES	64
>-
>-#define AXP809_DCDC4_600mV_START	0x00
>-#define AXP809_DCDC4_600mV_STEPS	47
>-#define AXP809_DCDC4_600mV_END		\
>-	(AXP809_DCDC4_600mV_START + AXP809_DCDC4_600mV_STEPS)
>-#define AXP809_DCDC4_1800mV_START	0x30
>-#define AXP809_DCDC4_1800mV_STEPS	8
>-#define AXP809_DCDC4_1800mV_END		\
>-	(AXP809_DCDC4_1800mV_START + AXP809_DCDC4_1800mV_STEPS)
> #define AXP809_DCDC4_NUM_VOLTAGES	57
> 
> #define AXP813_DCDC7_V_OUT_MASK		GENMASK(6, 0)
>@@ -517,26 +436,11 @@ static const struct regulator_ops axp20x_ops_sw =
>{
> };
> 
> static const struct regulator_linear_range axp20x_ldo4_ranges[] = {
>-	REGULATOR_LINEAR_RANGE(1250000,
>-			       AXP20X_LDO4_V_OUT_1250mV_START,
>-			       AXP20X_LDO4_V_OUT_1250mV_END,
>-			       0),
>-	REGULATOR_LINEAR_RANGE(1300000,
>-			       AXP20X_LDO4_V_OUT_1300mV_START,
>-			       AXP20X_LDO4_V_OUT_1300mV_END,
>-			       100000),
>-	REGULATOR_LINEAR_RANGE(2500000,
>-			       AXP20X_LDO4_V_OUT_2500mV_START,
>-			       AXP20X_LDO4_V_OUT_2500mV_END,
>-			       0),
>-	REGULATOR_LINEAR_RANGE(2700000,
>-			       AXP20X_LDO4_V_OUT_2700mV_START,
>-			       AXP20X_LDO4_V_OUT_2700mV_END,
>-			       100000),
>-	REGULATOR_LINEAR_RANGE(3000000,
>-			       AXP20X_LDO4_V_OUT_3000mV_START,
>-			       AXP20X_LDO4_V_OUT_3000mV_END,
>-			       100000),
>+	REGULATOR_LINEAR_RANGE(1250000, 0x0, 0x0, 0),
>+	REGULATOR_LINEAR_RANGE(1300000, 0x1, 0x8, 100000),
>+	REGULATOR_LINEAR_RANGE(2500000, 0x9, 0x9, 0),
>+	REGULATOR_LINEAR_RANGE(2700000, 0xa, 0xb, 100000),
>+	REGULATOR_LINEAR_RANGE(3000000, 0xc, 0xf, 100000),
> };
> 
> static const struct regulator_desc axp20x_regulators[] = {
>@@ -645,48 +549,24 @@ static const struct regulator_desc
>axp22x_drivevbus_regulator = {
> 
> /* DCDC ranges shared with AXP813 */
> static const struct regulator_linear_range axp803_dcdc234_ranges[] = {
>-	REGULATOR_LINEAR_RANGE(500000,
>-			       AXP803_DCDC234_500mV_START,
>-			       AXP803_DCDC234_500mV_END,
>-			       10000),
>-	REGULATOR_LINEAR_RANGE(1220000,
>-			       AXP803_DCDC234_1220mV_START,
>-			       AXP803_DCDC234_1220mV_END,
>-			       20000),
>+	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
>+	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x4b, 20000),
> };
> 
> static const struct regulator_linear_range axp803_dcdc5_ranges[] = {
>-	REGULATOR_LINEAR_RANGE(800000,
>-			       AXP803_DCDC5_800mV_START,
>-			       AXP803_DCDC5_800mV_END,
>-			       10000),
>-	REGULATOR_LINEAR_RANGE(1140000,
>-			       AXP803_DCDC5_1140mV_START,
>-			       AXP803_DCDC5_1140mV_END,
>-			       20000),
>+	REGULATOR_LINEAR_RANGE(800000, 0x0, 0x20, 10000),
>+	REGULATOR_LINEAR_RANGE(1140000, 0x21, 0x44, 20000),
> };
> 
> static const struct regulator_linear_range axp803_dcdc6_ranges[] = {
>-	REGULATOR_LINEAR_RANGE(600000,
>-			       AXP803_DCDC6_600mV_START,
>-			       AXP803_DCDC6_600mV_END,
>-			       10000),
>-	REGULATOR_LINEAR_RANGE(1120000,
>-			       AXP803_DCDC6_1120mV_START,
>-			       AXP803_DCDC6_1120mV_END,
>-			       20000),
>+	REGULATOR_LINEAR_RANGE(600000, 0x0, 0x32, 10000),
>+	REGULATOR_LINEAR_RANGE(1120000, 0x33, 0x47, 20000),
> };
> 
> /* AXP806's CLDO2 and AXP809's DLDO1 share the same range */
> static const struct regulator_linear_range axp803_dldo2_ranges[] = {
>-	REGULATOR_LINEAR_RANGE(700000,
>-			       AXP803_DLDO2_700mV_START,
>-			       AXP803_DLDO2_700mV_END,
>-			       100000),
>-	REGULATOR_LINEAR_RANGE(3400000,
>-			       AXP803_DLDO2_3400mV_START,
>-			       AXP803_DLDO2_3400mV_END,
>-			       200000),
>+	REGULATOR_LINEAR_RANGE(700000, 0x0, 0x1a, 100000),
>+	REGULATOR_LINEAR_RANGE(3400000, 0x1b, 0x1f, 200000),
> };
> 
> static const struct regulator_desc axp803_regulators[] = {
>@@ -765,25 +645,13 @@ static const struct regulator_desc
>axp803_regulators[] = {
> };
> 
> static const struct regulator_linear_range axp806_dcdca_ranges[] = {
>-	REGULATOR_LINEAR_RANGE(600000,
>-			       AXP806_DCDCA_600mV_START,
>-			       AXP806_DCDCA_600mV_END,
>-			       10000),
>-	REGULATOR_LINEAR_RANGE(1120000,
>-			       AXP806_DCDCA_1120mV_START,
>-			       AXP806_DCDCA_1120mV_END,
>-			       20000),
>+	REGULATOR_LINEAR_RANGE(600000, 0x0, 0x32, 10000),
>+	REGULATOR_LINEAR_RANGE(1120000, 0x33, 0x47, 20000),
> };
> 
> static const struct regulator_linear_range axp806_dcdcd_ranges[] = {
>-	REGULATOR_LINEAR_RANGE(600000,
>-			       AXP806_DCDCD_600mV_START,
>-			       AXP806_DCDCD_600mV_END,
>-			       20000),
>-	REGULATOR_LINEAR_RANGE(1600000,
>-			       AXP806_DCDCD_600mV_START,
>-			       AXP806_DCDCD_600mV_END,
>-			       100000),
>+	REGULATOR_LINEAR_RANGE(600000, 0x0, 0x2d, 20000),
>+	REGULATOR_LINEAR_RANGE(1600000, 0x2e, 0x3f, 100000),
> };
> 
> static const struct regulator_desc axp806_regulators[] = {
>@@ -841,14 +709,8 @@ static const struct regulator_desc
>axp806_regulators[] = {
> };
> 
> static const struct regulator_linear_range axp809_dcdc4_ranges[] = {
>-	REGULATOR_LINEAR_RANGE(600000,
>-			       AXP809_DCDC4_600mV_START,
>-			       AXP809_DCDC4_600mV_END,
>-			       20000),
>-	REGULATOR_LINEAR_RANGE(1800000,
>-			       AXP809_DCDC4_1800mV_START,
>-			       AXP809_DCDC4_1800mV_END,
>-			       100000),
>+	REGULATOR_LINEAR_RANGE(600000, 0x0, 0x2f, 20000),
>+	REGULATOR_LINEAR_RANGE(1800000, 0x30, 0x38, 100000),
> };
> 
> static const struct regulator_desc axp809_regulators[] = {

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines

[Axel Lin]

Olliver Schinagl <oliver@schinagl.nl> 於 2019年2月21日 週四 上午6:57寫道：
>
> Hey Axel,
>
> On February 20, 2019 5:50:13 PM GMT+01:00, Axel Lin <axel.lin@ingics.com> wrote:
> >The AXP20X_xxx_START/END/STEPS defines make the code hard to read and
> >very hard to check the linear range settings because it needs to check
> >the defines one-by-one.
> >The original code without the defines is very good in readability
> >as the meaning of each field of REGULATOR_LINEAR_RANGE is clear.
> >So I suggest to get rid of AXP20X_xxx_START/END/STEPS defines.
> Are you suggesting that magic values and hex numbers are more readable?

For example:
static const struct regulator_linear_range axp803_dcdc234_ranges[] = {
       REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
       REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x4b, 20000),
};
Above looks very clear to me as it describes the linear ranges:
1st linear range: min_uV is 500000, from selector 0 ~ 0x46, the uV_step is 10000
2nd linear range: min_uV is 1220000, from selector 0x47 ~ 0x4b, the
uV_step is 20000
And it's easy to check the min_sel and max_sel in each linear range.

Now, take a look at below code:
static const struct regulator_linear_range axp803_dcdc234_ranges[] = {
       REGULATOR_LINEAR_RANGE(500000,
                              AXP803_DCDC234_500mV_START,
                              AXP803_DCDC234_500mV_END,
                              10000),
       REGULATOR_LINEAR_RANGE(1220000,
                              AXP803_DCDC234_1220mV_START,
                              AXP803_DCDC234_1220mV_END,
                              20000),
};
1st linear range:
I have to check what is AXP803_DCDC234_500mV_START for min_sel,
then check what is AXP803_DCDC234_500mV_END for max_sel,
and AXP803_DCDC234_500mV_END is AXP803_DCDC234_500mV_START +
AXP803_DCDC234_500mV_STEPS
so I have to check if AXP803_DCDC234_500mV_STEPS is correct or not.
The same for 2nd linear range.
It's unnecessary complexity.
And it's just for 2 linear ranges, for axp20x_ldo4_ranges with 5 linear range,
it's really hard to check the settings.

IMHO, In such case, the defines does not help.
It's a rare case that I suggest to remove the defines.

>
> Maybe you can explain what your problem is, as it appears you are trying to debug something?
well, I just read the code.
I just found it's very hard to read the linear range setting for this driver.

Regards,
Axel

Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

On Thu, Feb 21, 2019 at 08:22:53AM +0800, Axel Lin wrote:
> Olliver Schinagl <oliver@schinagl.nl> 於 2019年2月21日 週四 上午6:57寫道：
> > On February 20, 2019 5:50:13 PM GMT+01:00, Axel Lin <axel.lin@ingics.com> wrote:

> > >The AXP20X_xxx_START/END/STEPS defines make the code hard to read and
> > >very hard to check the linear range settings because it needs to check
> > >the defines one-by-one.
> > >The original code without the defines is very good in readability
> > >as the meaning of each field of REGULATOR_LINEAR_RANGE is clear.
> > >So I suggest to get rid of AXP20X_xxx_START/END/STEPS defines.

> > Are you suggesting that magic values and hex numbers are more readable?

> For example:
> static const struct regulator_linear_range axp803_dcdc234_ranges[] = {
>        REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
>        REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x4b, 20000),
> };
> Above looks very clear to me as it describes the linear ranges:
> 1st linear range: min_uV is 500000, from selector 0 ~ 0x46, the uV_step is 10000
> 2nd linear range: min_uV is 1220000, from selector 0x47 ~ 0x4b, the
> uV_step is 20000
> And it's easy to check the min_sel and max_sel in each linear range.

Frankly I tend to agree with Axel here - the defines are used in exactly
one place so their main impact is that you can't read the ranges
directly in the array but have to jump around to read through the
defines.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines

[Olliver Schinagl]

On 21-02-2019 10:42, Mark Brown wrote:
> On Thu, Feb 21, 2019 at 08:22:53AM +0800, Axel Lin wrote:
>> Olliver Schinagl <oliver@schinagl.nl> 於 2019年2月21日 週四 上午6:57寫道：
>>> On February 20, 2019 5:50:13 PM GMT+01:00, Axel Lin <axel.lin@ingics.com> wrote:
>>>> The AXP20X_xxx_START/END/STEPS defines make the code hard to read and
>>>> very hard to check the linear range settings because it needs to check
>>>> the defines one-by-one.
>>>> The original code without the defines is very good in readability
>>>> as the meaning of each field of REGULATOR_LINEAR_RANGE is clear.
>>>> So I suggest to get rid of AXP20X_xxx_START/END/STEPS defines.
>>> Are you suggesting that magic values and hex numbers are more readable?
>> For example:
>> static const struct regulator_linear_range axp803_dcdc234_ranges[] = {
>>        REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
>>        REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x4b, 20000),
>> };
>> Above looks very clear to me as it describes the linear ranges:
>> 1st linear range: min_uV is 500000, from selector 0 ~ 0x46, the uV_step is 10000
>> 2nd linear range: min_uV is 1220000, from selector 0x47 ~ 0x4b, the
>> uV_step is 20000
>> And it's easy to check the min_sel and max_sel in each linear range.
> Frankly I tend to agree with Axel here - the defines are used in exactly
> one place so their main impact is that you can't read the ranges
> directly in the array but have to jump around to read through the
> defines.

The number of times the defines are used, are of course no reason for it
to be a bad thing. Defines should certainly not be thought of as a tool
to only reduce duplicity of course. Also being consistent where others
do have multiple invocations makes sense.

I will not disagree that it may be extra work to look up the define
(especially if there is no tool tip or split view in the editor) but
reading the whole lot of code, with only the magic values, you still
have to look up the meaning of each magic value, have to guess which one
has the same meaning etc.

Further more, I do believe far more people reading will find an define
to be more descriptive to read. Whoever needs to actually go in and
fix/change things.

Also, how many steps are in the first or second regulator ranges? which
bits are these exactly? Etc etc, you still have to use your mind to
figure things out.

Finally, while some people live and breath hexadecimals and bitmasks,
others just see magic values. So having something readable there, may
make it harder for the one that needs to work on it (which hopefully is
almost never, as these things are not very dynamic/commonly changing)
but passers by see readable text and understand what is happening at a
glance. Which is why we where taught not to use magic values in the
first place :)

Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines

[Axel Lin]

> I will not disagree that it may be extra work to look up the define
> (especially if there is no tool tip or split view in the editor) but
> reading the whole lot of code, with only the magic values, you still
> have to look up the meaning of each magic value, have to guess which one
> has the same meaning etc.
>
> Further more, I do believe far more people reading will find an define
> to be more descriptive to read. Whoever needs to actually go in and
> fix/change things.

I disagree.
The reason I sent this patch is because these defines reduce readability.
I do care about readability, but in this case these defines make
readability worsen.

At the context of REGULATOR_LINEAR_RANGE, each fields has well known meaning.
When I look at the table with values (I don't care if it's hex or decimal),
it tells everything I need to know.
I can easily check if any linear ranger cover other ranges.
I can easily check if any gap between linar ranges, (probably due to
reserved bits).
I can easily count the number of entries in each range.
I can easily calculate the min/max voltage of each range and double
check with datasheet.
i.e. If there are something wrong, it's eash to detect it.

When you change the values to DEFINES, I have to check the value of
each define *one-by-one*.
There is no benefit in this case.

I don't mean adding DEFINES is wrong. Just in this case it does not
help and actually has downside.
I only remove AXP20X_xxx_START/END/STEPS defines, not all defines.

BTW, just show you an example (from drivers/regulator/88pm8607.c)
I don't think change all below values to DEFINES help in readability.
static const unsigned int BUCK1_table[] = {
         725000,  750000,  775000,  800000,  825000,  850000,  875000,  900000,
         925000,  950000,  975000, 1000000, 1025000, 1050000, 1075000, 1100000,
        1125000, 1150000, 1175000, 1200000, 1225000, 1250000, 1275000, 1300000,
        1325000, 1350000, 1375000, 1400000, 1425000, 1450000, 1475000, 1500000,
              0,   25000,   50000,   75000,  100000,  125000,  150000,  175000,
         200000,  225000,  250000,  275000,  300000,  325000,  350000,  375000,
         400000,  425000,  450000,  475000,  500000,  525000,  550000,  575000,
         600000,  625000,  650000,  675000,  700000,  725000,  750000,  775000,
};


Regards,
Axel

Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines

[Olliver Schinagl]

On 23-02-2019 13:54, Axel Lin wrote:
>> I will not disagree that it may be extra work to look up the define
>> (especially if there is no tool tip or split view in the editor) but
>> reading the whole lot of code, with only the magic values, you still
>> have to look up the meaning of each magic value, have to guess which one
>> has the same meaning etc.
>>
>> Further more, I do believe far more people reading will find an define
>> to be more descriptive to read. Whoever needs to actually go in and
>> fix/change things.
> I disagree.
> The reason I sent this patch is because these defines reduce readability.
> I do care about readability, but in this case these defines make
> readability worsen.
Well this really is up to personal preference isn't it? As personally
find it much nicer to read without the magics :) If I actually have to
modify or go into the actual meaning, then yes, I will have to dig into
it a little deeper. But the overal code to a passer by, is still in my
opinion much more readable.
>
> At the context of REGULATOR_LINEAR_RANGE, each fields has well known meaning.
> When I look at the table with values (I don't care if it's hex or decimal),
> it tells everything I need to know.
> I can easily check if any linear ranger cover other ranges.
> I can easily check if any gap between linar ranges, (probably due to
> reserved bits).
> I can easily count the number of entries in each range.
> I can easily calculate the min/max voltage of each range and double
> check with datasheet.
> i.e. If there are something wrong, it's eash to detect it.

In any case, you seem like a smart person that reads and writes hex and
bits often enough. This is not true for everyone. I can just as easily
reverse your arguments of course, for example, 'each field has a well
known meaning' ... to whom? People that use these things daily, sure.
People who just need to double check something or modify something, not
so much. They have to look up the MACRO, the struct its in, compare it
to others, so as you can see, what is natural for you, is not true for
everyone. :)

Also, the general consensus is still to avoid magic values, and to stay
consistent with the rest and not make expceptions, it makes sense to
have defines instead of magic values.

>
> When you change the values to DEFINES, I have to check the value of
> each define *one-by-one*.
> There is no benefit in this case.
>
> I don't mean adding DEFINES is wrong. Just in this case it does not
> help and actually has downside.
> I only remove AXP20X_xxx_START/END/STEPS defines, not all defines.
>
> BTW, just show you an example (from drivers/regulator/88pm8607.c)
> I don't think change all below values to DEFINES help in readability.
> static const unsigned int BUCK1_table[] = {
>          725000,  750000,  775000,  800000,  825000,  850000,  875000,  900000,
>          925000,  950000,  975000, 1000000, 1025000, 1050000, 1075000, 1100000,
>         1125000, 1150000, 1175000, 1200000, 1225000, 1250000, 1275000, 1300000,
>         1325000, 1350000, 1375000, 1400000, 1425000, 1450000, 1475000, 1500000,
>               0,   25000,   50000,   75000,  100000,  125000,  150000,  175000,
>          200000,  225000,  250000,  275000,  300000,  325000,  350000,  375000,
>          400000,  425000,  450000,  475000,  500000,  525000,  550000,  575000,
>          600000,  625000,  650000,  675000,  700000,  725000,  750000,  775000,
> };

Personally, I think this is a horrible table :p sure, I can guess that
these are voltages (based on the fact that it's a regulator table and I
am a little familiar here), but without knowing the context, I see a
bunch of voltages, from 0,725 to 1,5 appearantly in equal steps, but the
first question I ask, is the step always .25? I can't see, i'd have to
go over each value and compare them all. Quite cumbersome ;)

And then, there is a nother row, which starts after the 1.5 but at 0 and
goes to 7.75. Are these two the same regulator? Why the overlap?

The only way to save this, would be with 2 linear range define/macro's,
with a start voltage, and end voltage and steps; right? So yes, macro's
and defines would help immensly here :)

>
>
> Regards,
> Axel

Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

On Sat, Feb 23, 2019 at 09:37:01PM +0100, Olliver Schinagl wrote:

> In any case, you seem like a smart person that reads and writes hex and
> bits often enough. This is not true for everyone. I can just as easily
> reverse your arguments of course, for example, 'each field has a well
> known meaning' ... to whom? People that use these things daily, sure.
> People who just need to double check something or modify something, not
> so much. They have to look up the MACRO, the struct its in, compare it
> to others, so as you can see, what is natural for you, is not true for
> everyone. :)

> Also, the general consensus is still to avoid magic values, and to stay
> consistent with the rest and not make expceptions, it makes sense to
> have defines instead of magic values.

If you find you need to describe what the fields are it would be much
more constructive to add a comment at the top of the table saying what
they are.  As things are this isn't helping anyone - as a big pile of
defines it's hard to read the values without context for how they're
used and if you're looking at the table you can't tell what the
regulator actually supports without going and decoding the defines.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines

[Olliver Schinagl]

On 25-02-2019 18:25, Mark Brown wrote:
> On Sat, Feb 23, 2019 at 09:37:01PM +0100, Olliver Schinagl wrote:
>
>> In any case, you seem like a smart person that reads and writes hex and
>> bits often enough. This is not true for everyone. I can just as easily
>> reverse your arguments of course, for example, 'each field has a well
>> known meaning' ... to whom? People that use these things daily, sure.
>> People who just need to double check something or modify something, not
>> so much. They have to look up the MACRO, the struct its in, compare it
>> to others, so as you can see, what is natural for you, is not true for
>> everyone. :)
>> Also, the general consensus is still to avoid magic values, and to stay
>> consistent with the rest and not make expceptions, it makes sense to
>> have defines instead of magic values.
> If you find you need to describe what the fields are it would be much
> more constructive to add a comment at the top of the table saying what
> they are.  As things are this isn't helping anyone - as a big pile of
> defines it's hard to read the values without context for how they're
> used and if you're looking at the table you can't tell what the
> regulator actually supports without going and decoding the defines.
Then the name of the define should be more constructive, which imo they
are reasonably? But as everything with programming, naming things is the
he hardest part, right?

Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1112 bytes --]

On Wed, Feb 27, 2019 at 08:41:46PM +0100, Olliver Schinagl wrote:
> On 25-02-2019 18:25, Mark Brown wrote:

> > If you find you need to describe what the fields are it would be much
> > more constructive to add a comment at the top of the table saying what
> > they are.  As things are this isn't helping anyone - as a big pile of
> > defines it's hard to read the values without context for how they're
> > used and if you're looking at the table you can't tell what the
> > regulator actually supports without going and decoding the defines.

> Then the name of the define should be more constructive, which imo they
> are reasonably? But as everything with programming, naming things is the
> he hardest part, right?

I really don't think that's it - I think that sometimes a data table is
just a data table.  There are some coding styles that work to avoid
having raw numbers anywhere in code outside of defines at all costs but
I do think that goes too far in cases like this where the name of the
define is at some level just going to summarize what should go in a
given slot in a table which adds little.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines

[Olliver Schinagl]

On 27-02-2019 21:05, Mark Brown wrote:
> On Wed, Feb 27, 2019 at 08:41:46PM +0100, Olliver Schinagl wrote:
>> On 25-02-2019 18:25, Mark Brown wrote:
>>> If you find you need to describe what the fields are it would be much
>>> more constructive to add a comment at the top of the table saying what
>>> they are.  As things are this isn't helping anyone - as a big pile of
>>> defines it's hard to read the values without context for how they're
>>> used and if you're looking at the table you can't tell what the
>>> regulator actually supports without going and decoding the defines.
>> Then the name of the define should be more constructive, which imo they
>> are reasonably? But as everything with programming, naming things is the
>> he hardest part, right?
> I really don't think that's it - I think that sometimes a data table is
> just a data table.  There are some coding styles that work to avoid
> having raw numbers anywhere in code outside of defines at all costs but
> I do think that goes too far in cases like this where the name of the
> define is at some level just going to summarize what should go in a
> given slot in a table which adds little.

I'm not sure if we're still talking about the same thing or same table;
In any case, this is something up to personal taste in the end, and I am
in the camp that favors (readable, which of course could always be
improved) defines over magic values; especially when it comes to these
bit-selectors. And even if a little less here, to keep things consistent
with all defines is why at least I prefer the one approach. You guys
prefer the raw values. Two flavors of two opinions I suppose :)

Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines

[Axel Lin]

Olliver Schinagl <oliver@schinagl.nl> 於 2019年2月24日 週日 上午4:37寫道：
>
> On 23-02-2019 13:54, Axel Lin wrote:
> >> I will not disagree that it may be extra work to look up the define
> >> (especially if there is no tool tip or split view in the editor) but
> >> reading the whole lot of code, with only the magic values, you still
> >> have to look up the meaning of each magic value, have to guess which one
> >> has the same meaning etc.
> >>
> >> Further more, I do believe far more people reading will find an define
> >> to be more descriptive to read. Whoever needs to actually go in and
> >> fix/change things.
> > I disagree.
> > The reason I sent this patch is because these defines reduce readability.
> > I do care about readability, but in this case these defines make
> > readability worsen.
> Well this really is up to personal preference isn't it? As personally
> find it much nicer to read without the magics :) If I actually have to
> modify or go into the actual meaning, then yes, I will have to dig into
> it a little deeper. But the overal code to a passer by, is still in my
> opinion much more readable.

It's not about personal preference.
The thing is your changes added unnecessary complexity as I pointed out.

> >
> > At the context of REGULATOR_LINEAR_RANGE, each fields has well known meaning.
> > When I look at the table with values (I don't care if it's hex or decimal),
> > it tells everything I need to know.
> > I can easily check if any linear ranger cover other ranges.
> > I can easily check if any gap between linar ranges, (probably due to
> > reserved bits).
> > I can easily count the number of entries in each range.
> > I can easily calculate the min/max voltage of each range and double
> > check with datasheet.
> > i.e. If there are something wrong, it's eash to detect it.
>
> In any case, you seem like a smart person that reads and writes hex and
> bits often enough. This is not true for everyone. I can just as easily
> reverse your arguments of course, for example, 'each field has a well
> known meaning' ... to whom? People that use these things daily, sure.
> People who just need to double check something or modify something, not
> so much. They have to look up the MACRO, the struct its in, compare it
> to others, so as you can see, what is natural for you, is not true for
> everyone. :)

To judge the readability you still have to understand the meaning of
fields of a REGULATOR_LINEAR_RANGE no matter using DEFINES or constant values.
Once you understand the fields of REGULATOR_LINEAR_RANGE, you will know
there is no readability issue with constant values in the table.

>
> Also, the general consensus is still to avoid magic values, and to stay
> consistent with the rest and not make expceptions, it makes sense to
> have defines instead of magic values.
>
> >
> > When you change the values to DEFINES, I have to check the value of
> > each define *one-by-one*.
> > There is no benefit in this case.
> >
> > I don't mean adding DEFINES is wrong. Just in this case it does not
> > help and actually has downside.
> > I only remove AXP20X_xxx_START/END/STEPS defines, not all defines.
> >
> > BTW, just show you an example (from drivers/regulator/88pm8607.c)
> > I don't think change all below values to DEFINES help in readability.
> > static const unsigned int BUCK1_table[] = {
> >          725000,  750000,  775000,  800000,  825000,  850000,  875000,  900000,
> >          925000,  950000,  975000, 1000000, 1025000, 1050000, 1075000, 1100000,
> >         1125000, 1150000, 1175000, 1200000, 1225000, 1250000, 1275000, 1300000,
> >         1325000, 1350000, 1375000, 1400000, 1425000, 1450000, 1475000, 1500000,
> >               0,   25000,   50000,   75000,  100000,  125000,  150000,  175000,
> >          200000,  225000,  250000,  275000,  300000,  325000,  350000,  375000,
> >          400000,  425000,  450000,  475000,  500000,  525000,  550000,  575000,
> >          600000,  625000,  650000,  675000,  700000,  725000,  750000,  775000,
> > };
>
> Personally, I think this is a horrible table :p sure, I can guess that
> these are voltages (based on the fact that it's a regulator table and I
> am a little familiar here), but without knowing the context, I see a
> bunch of voltages, from 0,725 to 1,5 appearantly in equal steps, but the
> first question I ask, is the step always .25? I can't see, i'd have to
> go over each value and compare them all. Quite cumbersome ;)
>
> And then, there is a nother row, which starts after the 1.5 but at 0 and
> goes to 7.75. Are these two the same regulator? Why the overlap?

Your guessing is wrong. This is *not* a linear range table.
To judge the readability you had better really read the code first.
Macro's and defines cannot help you regarding this.

Axel