From mboxrd@z Thu Jan  1 00:00:00 1970
From: Siarhei Siamashka <siarhei.siamashka@gmail.com>
Date: Sun, 11 Jan 2015 14:05:08 +0200
Subject: [U-Boot] [PATCH 1/8] sunxi: axp221: Add ELDO[1-3] support
In-Reply-To: <20150109180535.41f14077@crub>
References: <1420797676-22515-1-git-send-email-siarhei.siamashka@gmail.com>
	<1420797676-22515-2-git-send-email-siarhei.siamashka@gmail.com>
	<20150109180535.41f14077@crub>
Message-ID: <20150111140508.62cea298@i7>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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.

-- 
Best regards,
Siarhei Siamashka