diff mbox

[v3,02/23] thermal: armada: remove useless register accesses

Message ID 20180716144206.30985-3-miquel.raynal@bootlin.com (mailing list archive)
State New, archived
Headers show

Commit Message

Miquel Raynal July 16, 2018, 2:41 p.m. UTC
Prepare the migration to use regmaps by first simplifying the
initialization functions: avoid unnecessary write/read cycles on
configuration registers.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/thermal/armada_thermal.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Daniel Lezcano July 27, 2018, 3:56 p.m. UTC | #1
On 16/07/2018 16:41, Miquel Raynal wrote:
> Prepare the migration to use regmaps by first simplifying the
> initialization functions: avoid unnecessary write/read cycles on
> configuration registers.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Good catch. I'm wondering why it was done this way ...

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---
>  drivers/thermal/armada_thermal.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index 077e8e562306..6fdb90b3c001 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -103,16 +103,13 @@ static void armadaxp_init_sensor(struct platform_device *pdev,
>  
>  	reg = readl_relaxed(priv->control1);
>  	reg |= PMU_TDC0_OTF_CAL_MASK;
> -	writel(reg, priv->control1);
>  
>  	/* Reference calibration value */
>  	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
>  	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> -	writel(reg, priv->control1);
>  
>  	/* Reset the sensor */
> -	reg = readl_relaxed(priv->control1);
> -	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control1);
> +	reg |= PMU_TDC0_SW_RST_MASK;
>  
>  	writel(reg, priv->control1);
>  
> @@ -129,14 +126,13 @@ static void armada370_init_sensor(struct platform_device *pdev,
>  
>  	reg = readl_relaxed(priv->control1);
>  	reg |= PMU_TDC0_OTF_CAL_MASK;
> -	writel(reg, priv->control1);
>  
>  	/* Reference calibration value */
>  	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
>  	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> -	writel(reg, priv->control1);
>  
>  	reg &= ~PMU_TDC0_START_CAL_MASK;
> +
>  	writel(reg, priv->control1);
>  
>  	msleep(10);
>
Ezequiel Garcia July 27, 2018, 4:13 p.m. UTC | #2
Hi Miquel,

You know, it is usually useful to Cc the author of the lines a patch touches :-)

On 16 July 2018 at 11:41, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> Prepare the migration to use regmaps by first simplifying the
> initialization functions: avoid unnecessary write/read cycles on
> configuration registers.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/thermal/armada_thermal.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index 077e8e562306..6fdb90b3c001 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -103,16 +103,13 @@ static void armadaxp_init_sensor(struct platform_device *pdev,
>
>         reg = readl_relaxed(priv->control1);
>         reg |= PMU_TDC0_OTF_CAL_MASK;
> -       writel(reg, priv->control1);
>
>         /* Reference calibration value */
>         reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
>         reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> -       writel(reg, priv->control1);
>
>         /* Reset the sensor */
> -       reg = readl_relaxed(priv->control1);
> -       writel((reg | PMU_TDC0_SW_RST_MASK), priv->control1);
> +       reg |= PMU_TDC0_SW_RST_MASK;
>
>         writel(reg, priv->control1);
>
> @@ -129,14 +126,13 @@ static void armada370_init_sensor(struct platform_device *pdev,
>
>         reg = readl_relaxed(priv->control1);
>         reg |= PMU_TDC0_OTF_CAL_MASK;
> -       writel(reg, priv->control1);
>
>         /* Reference calibration value */
>         reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
>         reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> -       writel(reg, priv->control1);
>
>         reg &= ~PMU_TDC0_START_CAL_MASK;
> +
>         writel(reg, priv->control1);
>

IIRC, the documentation for this hardware block was a meager.

Are you sure there isn't any requirement for the calibration
start to be issued separately or something like that?
Miquel Raynal July 29, 2018, 7:23 p.m. UTC | #3
Hi Ezequiel,

Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote on Fri, 27 Jul
2018 13:13:20 -0300:

> Hi Miquel,
> 
> You know, it is usually useful to Cc the author of the lines a patch touches :-)

I have to admit I limited the CC list to the get_maintainers.pl output.
But for sure I can add you for further changes :)

> 
> On 16 July 2018 at 11:41, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > Prepare the migration to use regmaps by first simplifying the
> > initialization functions: avoid unnecessary write/read cycles on
> > configuration registers.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/thermal/armada_thermal.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> > index 077e8e562306..6fdb90b3c001 100644
> > --- a/drivers/thermal/armada_thermal.c
> > +++ b/drivers/thermal/armada_thermal.c
> > @@ -103,16 +103,13 @@ static void armadaxp_init_sensor(struct platform_device *pdev,
> >
> >         reg = readl_relaxed(priv->control1);
> >         reg |= PMU_TDC0_OTF_CAL_MASK;
> > -       writel(reg, priv->control1);
> >
> >         /* Reference calibration value */
> >         reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> >         reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > -       writel(reg, priv->control1);
> >
> >         /* Reset the sensor */
> > -       reg = readl_relaxed(priv->control1);
> > -       writel((reg | PMU_TDC0_SW_RST_MASK), priv->control1);
> > +       reg |= PMU_TDC0_SW_RST_MASK;
> >
> >         writel(reg, priv->control1);
> >
> > @@ -129,14 +126,13 @@ static void armada370_init_sensor(struct platform_device *pdev,
> >
> >         reg = readl_relaxed(priv->control1);
> >         reg |= PMU_TDC0_OTF_CAL_MASK;
> > -       writel(reg, priv->control1);
> >
> >         /* Reference calibration value */
> >         reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> >         reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > -       writel(reg, priv->control1);
> >
> >         reg &= ~PMU_TDC0_START_CAL_MASK;
> > +
> >         writel(reg, priv->control1);
> >  
> 
> IIRC, the documentation for this hardware block was a meager.
> 
> Are you sure there isn't any requirement for the calibration
> start to be issued separately or something like that?

I didn't find any info about this, I don't think it will be a problem
if my understanding of this IP is correct (had a lot of interactions
with Marvell engineers) but cannot be 100% sure, if someone has a
problem we'll know where to look at.

Thanks,
Miquèl
diff mbox

Patch

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 077e8e562306..6fdb90b3c001 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -103,16 +103,13 @@  static void armadaxp_init_sensor(struct platform_device *pdev,
 
 	reg = readl_relaxed(priv->control1);
 	reg |= PMU_TDC0_OTF_CAL_MASK;
-	writel(reg, priv->control1);
 
 	/* Reference calibration value */
 	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
 	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
-	writel(reg, priv->control1);
 
 	/* Reset the sensor */
-	reg = readl_relaxed(priv->control1);
-	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control1);
+	reg |= PMU_TDC0_SW_RST_MASK;
 
 	writel(reg, priv->control1);
 
@@ -129,14 +126,13 @@  static void armada370_init_sensor(struct platform_device *pdev,
 
 	reg = readl_relaxed(priv->control1);
 	reg |= PMU_TDC0_OTF_CAL_MASK;
-	writel(reg, priv->control1);
 
 	/* Reference calibration value */
 	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
 	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
-	writel(reg, priv->control1);
 
 	reg &= ~PMU_TDC0_START_CAL_MASK;
+
 	writel(reg, priv->control1);
 
 	msleep(10);