[01/18] thermal: exynos: fix setting rising_threshold for Exynos5433
diff mbox

Message ID 1524743493-28113-2-git-send-email-b.zolnierkie@samsung.com
State Accepted
Delegated to: Eduardo Valentin
Headers show

Commit Message

Bartlomiej Zolnierkiewicz April 26, 2018, 11:51 a.m. UTC
Add missing clearing of the previous value when setting rising
temperature threshold.

Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Daniel Lezcano April 30, 2018, 2:36 p.m. UTC | #1
On Thu, Apr 26, 2018 at 01:51:16PM +0200, Bartlomiej Zolnierkiewicz wrote:
> Add missing clearing of the previous value when setting rising
> temperature threshold.
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index cda716c..523d26e 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -577,6 +577,7 @@ static int exynos5433_tmu_initialize(struct platform_device *pdev)
>  		threshold_code = temp_to_code(data, temp);
>  
>  		rising_threshold = readl(data->base + rising_reg_offset);
> +		rising_threshold &= ~(0xff << j * 8);
>  		rising_threshold |= (threshold_code << j * 8);

Bartlomiej,

I see this code is duplicated all around the driver, so I can't blame you to
fix it in the same way it is written today but this is not how to deal with
fields in a register mapping. Can you fix it by adding correct macros with
masks?
Bartlomiej Zolnierkiewicz April 30, 2018, 3:21 p.m. UTC | #2
On Monday, April 30, 2018 04:36:56 PM Daniel Lezcano wrote:
> On Thu, Apr 26, 2018 at 01:51:16PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > Add missing clearing of the previous value when setting rising
> > temperature threshold.
> > 
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > ---
> >  drivers/thermal/samsung/exynos_tmu.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> > index cda716c..523d26e 100644
> > --- a/drivers/thermal/samsung/exynos_tmu.c
> > +++ b/drivers/thermal/samsung/exynos_tmu.c
> > @@ -577,6 +577,7 @@ static int exynos5433_tmu_initialize(struct platform_device *pdev)
> >  		threshold_code = temp_to_code(data, temp);
> >  
> >  		rising_threshold = readl(data->base + rising_reg_offset);
> > +		rising_threshold &= ~(0xff << j * 8);
> >  		rising_threshold |= (threshold_code << j * 8);
> 
> Bartlomiej,
> 
> I see this code is duplicated all around the driver, so I can't blame you to
> fix it in the same way it is written today but this is not how to deal with

This patch is a bugfix (which may be backported later) and it shouldn't be
mixed with cleanups.

> fields in a register mapping. Can you fix it by adding correct macros with
> masks?

After my patch series we still have following occurrences of "RMW" code:

        th = readl(data->base + EXYNOS_THD_TEMP_RISE);
        th &= ~(0xff << 8 * trip);
        th |= temp_to_code(data, temp) << 8 * trip;
        writel(th, data->base + EXYNOS_THD_TEMP_RISE);

        th = readl(data->base + EXYNOS_THD_TEMP_FALL);
        th &= ~(0xff << 8 * trip);
        if (hyst)
                th |= temp_to_code(data, temp - hyst) << 8 * trip;
        writel(th, data->base + EXYNOS_THD_TEMP_FALL);

        th = readl(data->base + reg_off);
        th &= ~(0xff << j * 8);
        th |= (temp_to_code(data, temp) << j * 8);
        writel(th, data->base + reg_off);

        th = readl(data->base + reg_off);
        th &= ~(0xff << j * 8);
        th |= (temp_to_code(data, temp - hyst) << j * 8);
        writel(th, data->base + reg_off);

        th = readl(data->base + EXYNOS7_THD_TEMP_RISE7_6 + reg_off);
        th &= ~(EXYNOS7_TMU_TEMP_MASK << (16 * bit_off));
        th |= temp_to_code(data, temp) << (16 * bit_off);
        writel(th, data->base + EXYNOS7_THD_TEMP_RISE7_6 + reg_off);

        th = readl(data->base + EXYNOS7_THD_TEMP_FALL7_6 + reg_off);
        th &= ~(EXYNOS7_TMU_TEMP_MASK << (16 * bit_off));
        th |= temp_to_code(data, temp - hyst) << (16 * bit_off);
        writel(th, data->base + EXYNOS7_THD_TEMP_FALL7_6 + reg_off);

My personal opinion is that adding new macro for the code above will
only obfuscate it and make it harder to understand,

Anyway how would you like this new macro to look like (how generic or
specific it should be etc.)?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Patch
diff mbox

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index cda716c..523d26e 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -577,6 +577,7 @@  static int exynos5433_tmu_initialize(struct platform_device *pdev)
 		threshold_code = temp_to_code(data, temp);
 
 		rising_threshold = readl(data->base + rising_reg_offset);
+		rising_threshold &= ~(0xff << j * 8);
 		rising_threshold |= (threshold_code << j * 8);
 		writel(rising_threshold, data->base + rising_reg_offset);