[U-Boot] [PATCH 1/8] sunxi: axp221: Add ELDO[1-3] support

[Hans de Goede <hdegoede@redhat.com>]

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