Hi,
On 11-01-15 13:05, Siarhei Siamashka wrote:
On Fri, 9 Jan 2015 18:05:35 +0100 Anatolij Gustschin agust@denx.de wrote:
On Fri, 9 Jan 2015 12:01:09 +0200 Siarhei Siamashka siarhei.siamashka@gmail.com wrote: ...
+int axp221_set_eldo2(unsigned int mvolt) +{
- int ret;
- u8 cfg = axp221_mvolt_to_cfg(mvolt, 700, 3300, 100);
- if (mvolt == 0)
return axp221_clrbits(AXP221_OUTPUT_CTRL2,AXP221_OUTPUT_CTRL2_ELDO2_EN);- ret = pmic_bus_write(AXP221_ELDO2_CTRL, cfg);
- if (ret)
return ret;- return axp221_setbits(AXP221_OUTPUT_CTRL2,
AXP221_OUTPUT_CTRL2_ELDO2_EN);+}
+int axp221_set_eldo3(unsigned int mvolt) +{
- int ret;
- u8 cfg = axp221_mvolt_to_cfg(mvolt, 700, 3300, 100);
- if (mvolt == 0)
return axp221_clrbits(AXP221_OUTPUT_CTRL2,AXP221_OUTPUT_CTRL2_ELDO3_EN);- ret = pmic_bus_write(AXP221_ELDO3_CTRL, cfg);
- if (ret)
return ret;- return axp221_setbits(AXP221_OUTPUT_CTRL2,
AXP221_OUTPUT_CTRL2_ELDO3_EN);+}
Can we avoid code duplication here? I.e. only one function should be sufficient here:
int axp221_set_eldo(int eldo_num, unsigned int mvolt) { int ret; u8 cfg = axp221_mvolt_to_cfg(mvolt, 700, 3300, 100); u8 addr, bits;
switch (eldo_num) { case 3: addr = AXP221_ELDO3_CTRL; bits = AXP221_OUTPUT_CTRL2_ELDO3_EN; break; case 2: addr = AXP221_ELDO2_CTRL; bits = AXP221_OUTPUT_CTRL2_ELDO2_EN; break; case 1: default: addr = AXP221_ELDO1_CTRL; bits = AXP221_OUTPUT_CTRL2_ELDO1_EN; break; } if (mvolt == 0) return axp221_clrbits(AXP221_OUTPUT_CTRL2, bits); ret = pmic_bus_write(addr, cfg); if (ret) return ret; return axp221_setbits(AXP221_OUTPUT_CTRL2, bits);}
Thanks for the review.
Yes, I am not very happy about this code duplication either. But I just made it so that it follows the same pattern as the rest of the code in the axp221 source file for the other voltage regulators. Does it mean that we want to change it all to have a consistent style?
This is a purely sunxi code and not directly related to video. And I would like to know what sunxi custodians think about it.
I think that Anatolij's suggestion is a good one, you're right that it introduces some inconsistency wrt the existing code, but I think it would be better to eventually also move the existing code over to the model suggested by Anatolij. So lets make the eldo support follow this from day one.
Then one day someone should do a follow up patch separate from this patch-set to make the rest also follow the eldo model, with one function per type of regulator, so we end up with axp221_set_dcdc axp221_set_dldo and axp221_set_eldo.
Regards,
Hans