Re: [PATCH v3 1/4] mfd: arizona: Factor out SYSCLK enable from wm5102 hardware patch

[Lee Jones <lee.jones@linaro.org>]

On Mon, 30 Mar 2015, Charles Keepax wrote:
> On Mon, Mar 30, 2015 at 12:22:55PM +0100, Lee Jones wrote:
> > On Mon, 23 Mar 2015, Charles Keepax wrote:
> > 
> > > wm5102 applies a custom hardware boot sequence, for this the SYSCLK
> > > needs to be enabled. This patch factors out the code that enables
> > > SYSCLK for this sequence such that it can be used for other boot time
> > > operations that require SYSCLK.
> > > 
> > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > ---
> > > 
> > > Changes since v3:
> > >  - Split out enable and disable for the freerunning SYSCLK instead
> > >    of having a single function that takes a function pointer.
> > >  - Improve some naming for the sake of clarity
> > >  - Update error handling to use if (err) rather than if (err != 0)
> > >  - Added comment on register patch
> > > 
> > > Thanks,
> > > Charles
> > > 
> > >  drivers/mfd/arizona-core.c |  103 ++++++++++++++++++++++++++++++-------------
> > >  1 files changed, 72 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> > > index 6ca6dfa..695c68e 100644
> > > --- a/drivers/mfd/arizona-core.c
> > > +++ b/drivers/mfd/arizona-core.c
> > > @@ -250,20 +250,26 @@ static int arizona_wait_for_boot(struct arizona *arizona)
> > >  	return ret;
> > >  }
> > >  
> > > -static int arizona_apply_hardware_patch(struct arizona* arizona)
> > > +struct arizona_sysclk_state {
> > > +	unsigned int fll;
> > > +	unsigned int sysclk;
> > > +};
> > > +
> > > +
> <snip>
> > > +err_fll:
> > > +	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, state->fll);
> > > +	if (err)
> > > +		dev_err(arizona->dev,
> > > +			"Failed to re-apply old FLL settings: %d\n",
> > > +			err);
> > 
> > Nit: How is it that the regmap_write() line fit on 80 chars and the
> > "err);" bit can't?
> 
> It can, I just thought it looked a bit nicer this way. But happy
> to change.

I don't usually like unnecessary line wraps, but it's only a nit, so
you decide.

> > > +	return ret;
> > > +}
> > > +
> > > +static int arizona_disable_freerun_sysclk(struct arizona *arizona,
> > > +					  struct arizona_sysclk_state *state)
> > > +{
> > > +	int ret = 0;
> > > +	int err;
> > > +
> > > +	err = regmap_write(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1,
> > > +			   state->sysclk);
> > > +	if (err) {
> > > +		dev_err(arizona->dev,
> > > +			"Failed to re-apply old SYSCLK settings: %d\n",
> > > +			err);
> > 
> > Same there, this extra linewrap seems unnecessary.
> 
> Ditto. Can change.
> 
> > 
> > > +		ret = err;
> > > +	}
> > 
> > Can you explain the resson for using 'err' and and not 'ret'?
> 
> It is to return an error if either write failed but still execute
> both writes. Although looking at things again with fresh eyes, it
> is pretty epically catastrophic if either one fails so I am not
> so sure we care in the case one has failed if we do the other.  I
> will fix this up to.

I can see why you used 'err' below, but this one can be 'ret'.

But if it's a proper failure then this point is moot and you can use
conventional error handling instead.

> > > +	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, state->fll);
> > > +	if (err) {
> > > +		dev_err(arizona->dev,
> > > +			"Failed to re-apply old FLL settings: %d\n",
> > > +			err);
> > > +		ret = err;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> <snip>
> > > -err_fll:
> > > -	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, fll);
> > > -	if (err != 0) {
> > > -		dev_err(arizona->dev,
> > > -			"Failed to re-apply old FLL settings: %d\n",
> > > -			err);
> > > -	}
> > > +err:
> > > +	err = arizona_disable_freerun_sysclk(arizona, &state);
> > >  
> > > -	if (ret != 0)
> > > +	if (ret)
> > >  		return ret;
> > >  	else
> > >  		return err;
> > 
> > return ret ?: err;
> 
> Hard to keep tabs on people's preferences around these ternary
> operators. I am happy to update this too.
> 
> Will fire out a new spin tomorrow.
> 
> Thanks,
> Charles

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog