Hi Jonathan,

Jonathan Cameron [off-list ref] wrote on Mon, 30 Aug 2021 14:56:08
+0100:

On Wed, 25 Aug 2021 17:24:58 +0200
Miquel Raynal [off-list ref] wrote:

quoted

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

quoted

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

quoted

 	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