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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150330163933.GA5442@opensource.wolfsonmicro.com>
Date:	Mon, 30 Mar 2015 17:39:33 +0100
From:	Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	sameo@...ux.intel.com, broonie@...nel.org, lgirdwood@...il.com,
	patches@...nsource.wolfsonmicro.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/4] mfd: arizona: Factor out SYSCLK enable from
	wm5102 hardware patch

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@...nsource.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.

> 
> > +	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.

> 
> > +	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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/