diff mbox series

[v2,21/46] mfd: ti_am335x_tscadc: Reorder the initialization steps

Message ID 20210902215144.507243-22-miquel.raynal@bootlin.com
State New
Headers show
Series TI AM437X ADC1 | expand

Commit Message

Miquel Raynal Sept. 2, 2021, 9:51 p.m. UTC
TI AM335X TRM [1] states that most of the configuration should be set in
the control register in the first place, before actually enabling the
hardware with the subsystem enable bit.

So far only half of the configuration was made in the first step (before
enabling the "subsystem"), which does not make really sense. Also, the
probe and the resume patch were acting differently. Let's harmonize all
this by following these steps:
1/ Configure the CLKDIV register
2/ Configure the CTRL register
3/ Configure the idle configuration
4/ Really enable the device by rewriting the CTRL register with the
   subsystem enable bit set.

[1] https://www.ti.com/lit/pdf/spruh73

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Jonathan Cameron Sept. 5, 2021, 12:18 p.m. UTC | #1
On Thu,  2 Sep 2021 23:51:19 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> TI AM335X TRM [1] states that most of the configuration should be set in
> the control register in the first place, before actually enabling the
> hardware with the subsystem enable bit.
> 
> So far only half of the configuration was made in the first step (before
> enabling the "subsystem"), which does not make really sense. Also, the
> probe and the resume patch were acting differently. Let's harmonize all
> this by following these steps:
> 1/ Configure the CLKDIV register
> 2/ Configure the CTRL register
> 3/ Configure the idle configuration
> 4/ Really enable the device by rewriting the CTRL register with the
>    subsystem enable bit set.
> 
> [1] https://www.ti.com/lit/pdf/spruh73
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
This tidies up the other patches which is good. Now I haven't checked the new
order against the datasheet but it now does things as you describe so I'm assuming
that's fine.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/mfd/ti_am335x_tscadc.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> index 5f189da99468..61f878c7593b 100644
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -221,8 +221,6 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  
>  	/* Set the control register bits */
>  	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
> -	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
> -
>  	if (tsc_wires > 0) {
>  		tscadc->tsc_wires = tsc_wires;
>  		if (tsc_wires == 5)
> @@ -230,6 +228,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  		else
>  			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
>  	}
> +	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
>  
>  	tscadc_idle_config(tscadc);
>  
> @@ -315,9 +314,9 @@ static int __maybe_unused tscadc_resume(struct device *dev)
>  	pm_runtime_get_sync(dev);
>  
>  	/* context restore */
> -	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
> -	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
> +	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
>  
> +	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
>  	if (tscadc->tsc_wires > 0) {
>  		if (tscadc->tsc_wires == 5)
>  			ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB;
> @@ -325,13 +324,13 @@ static int __maybe_unused tscadc_resume(struct device *dev)
>  			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
>  	}
>  
> +	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
> +
>  	tscadc_idle_config(tscadc);
>  
>  	ctrl |= CNTRLREG_TSCSSENB;
>  	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
>  
> -	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
> -
>  	return 0;
>  }
>
diff mbox series

Patch

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 5f189da99468..61f878c7593b 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -221,8 +221,6 @@  static	int ti_tscadc_probe(struct platform_device *pdev)
 
 	/* Set the control register bits */
 	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
-
 	if (tsc_wires > 0) {
 		tscadc->tsc_wires = tsc_wires;
 		if (tsc_wires == 5)
@@ -230,6 +228,7 @@  static	int ti_tscadc_probe(struct platform_device *pdev)
 		else
 			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
 	}
+	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
 
 	tscadc_idle_config(tscadc);
 
@@ -315,9 +314,9 @@  static int __maybe_unused tscadc_resume(struct device *dev)
 	pm_runtime_get_sync(dev);
 
 	/* context restore */
-	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
+	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
 
+	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
 	if (tscadc->tsc_wires > 0) {
 		if (tscadc->tsc_wires == 5)
 			ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB;
@@ -325,13 +324,13 @@  static int __maybe_unused tscadc_resume(struct device *dev)
 			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
 	}
 
+	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
+
 	tscadc_idle_config(tscadc);
 
 	ctrl |= CNTRLREG_TSCSSENB;
 	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
 
-	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
-
 	return 0;
 }