Messages in this thread | | | From | Axel Lin <> | Date | Thu, 21 Feb 2019 08:22:53 +0800 | Subject | Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines |
| |
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
| |