On Thu, 2 Sep 2021 21:42:47 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> Hi Jonathan,
>
> Jonathan Cameron <jic23@xxxxxxxxxx> wrote on Mon, 30 Aug 2021 14:56:08
> +0100:
>
> > On Wed, 25 Aug 2021 17:24:58 +0200
> > Miquel Raynal <miquel.raynal@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > 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".
Fair enough. Perhaps it's worth throwing in a comment though if you happen
to be respining to say tcsadc->ctrl isn't actually the contents of
the register, but rather of 'most' of it.
Jonathan
>
> Thanks,
> Miquèl
