Re: [PATCH 20/40] mfd: ti_am335x_tscadc: Gather the ctrl register logic at one place

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Jonathan,

Jonathan Cameron <jic23@kernel.org> wrote on Mon, 30 Aug 2021 14:56:08
+0100:

> On Wed, 25 Aug 2021 17:24:58 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Instead of deriving in the probe and in the resume path the value of the
> > ctrl register, let's do it only once in the probe, save the value of
> > this register in the driver's structure and use it from the resume
> > callback.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> A few minor things inline.
> 
> J
> 
> > ---
> >  drivers/mfd/ti_am335x_tscadc.c       | 31 ++++++++--------------------
> >  include/linux/mfd/ti_am335x_tscadc.h |  2 +-
> >  2 files changed, 10 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > index 7071344ad18e..d661e8ae66c9 100644
> > --- a/drivers/mfd/ti_am335x_tscadc.c
> > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > @@ -122,7 +122,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> >  	struct clk *clk;
> >  	u32 val;
> >  	int tsc_wires = 0, adc_channels = 0, readouts = 0, cell_idx = 0;
> > -	int total_channels, ctrl, err;
> > +	int total_channels, err;
> >  
> >  	/* Allocate memory for device */
> >  	tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL);
> > @@ -215,22 +215,21 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> >  	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
> >  
> >  	/* Set the control register bits */
> > -	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
> > -	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
> > +	tscadc->ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID;
> > +	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);
> >  
> >  	if (tsc_wires > 0) {
> > -		tscadc->tsc_wires = tsc_wires;
> > +		tscadc->ctrl |= CNTRLREG_TSCENB;
> >  		if (tsc_wires == 5)
> > -			ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB;
> > +			tscadc->ctrl |= CNTRLREG_5WIRE;
> >  		else
> > -			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
> > +			tscadc->ctrl |= CNTRLREG_4WIRE;
> >  	}
> >  
> >  	tscadc_idle_config(tscadc);
> >  
> >  	/* Enable the TSC module enable bit */
> > -	ctrl |= CNTRLREG_TSCSSENB;
> > -	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
> > +	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB);
> >  
> >  	/* TSC Cell */
> >  	if (tsc_wires > 0) {
> > @@ -305,25 +304,13 @@ static int __maybe_unused tscadc_suspend(struct device *dev)
> >  static int __maybe_unused tscadc_resume(struct device *dev)
> >  {
> >  	struct ti_tscadc_dev *tscadc = dev_get_drvdata(dev);
> > -	u32 ctrl;
> >  
> >  	pm_runtime_get_sync(dev);
> >  
> > -	/* context restore */
> > -	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
> > -	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
> > -
> > -	if (tscadc->tsc_wires > 0) {
> > -		if (tscadc->tsc_wires == 5)
> > -			ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB;
> > -		else
> > -			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
> > -	}
> > -	ctrl |= CNTRLREG_TSCSSENB;
> > -	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
> > -
> >  	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
> > +	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);  
> 
> Patch description should mention why this ordering change is here.

I actually moved the patch that reorders things earlier so that the
reviewer is not bothered by the order changes later on.

> 
> >  	tscadc_idle_config(tscadc);
> > +	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB);  
> 
> As the value of tscadc->ctrl is not the same as REG_CTRL this is a bit non obvious.
> 
> You might be better off keeping them in sync, but masking that bit out and then resetting
> it as appropriate.

I honestly find more readable doing:

ctrl = flags;
writel(ctrl);
writel(ctrl | en_bit);

than

ctrl = flags;
writel(ctrl & ~en_bit);
writel(ctrl);

because the second version emphasis the fact that we reset the en_bit
(which is wrong, the point of this first write is to actually write all
the configuration but not the en_bit yet) while the first version
clearly shows that the second write includes an additional "enable bit".

Thanks,
Miquèl