| Message ID | 1524743493-28113-2-git-send-email-b.zolnierkie@samsung.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Eduardo Valentin |
| Headers | show |
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?
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
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);
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(+)