From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752573AbbCaIB6 (ORCPT ); Tue, 31 Mar 2015 04:01:58 -0400 Received: from mail-wg0-f53.google.com ([74.125.82.53]:33785 "EHLO mail-wg0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751008AbbCaIB5 (ORCPT ); Tue, 31 Mar 2015 04:01:57 -0400 Date: Tue, 31 Mar 2015 09:01:52 +0100 From: Lee Jones To: Charles Keepax Cc: sameo@linux.intel.com, broonie@kernel.org, lgirdwood@gmail.com, patches@opensource.wolfsonmicro.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 1/4] mfd: arizona: Factor out SYSCLK enable from wm5102 hardware patch Message-ID: <20150331080152.GL9447@x1> References: <1427127589-18297-1-git-send-email-ckeepax@opensource.wolfsonmicro.com> <20150330112255.GB9447@x1> <20150330163933.GA5442@opensource.wolfsonmicro.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150330163933.GA5442@opensource.wolfsonmicro.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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 > > > --- > > > > > > 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; > > > +}; > > > + > > > + > > > > +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; > > > +} > > > + > > > > -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