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".
Thanks,
Miquèl
