| Message ID | 20231120145049.310509-9-m.majewski2@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve Exynos thermal driver | expand |
Hi Mateusz, On 11/20/23 14:50, Mateusz Majewski wrote: > The original driver did not use that macro and it allows us to make our > intentions slightly clearer. > > Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com> > --- > drivers/thermal/samsung/exynos_tmu.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > [snip] > @@ -590,15 +590,15 @@ static void exynos5433_tmu_control(struct platform_device *pdev, bool on) > continue; > > interrupt_en |= > - (1 << (EXYNOS7_TMU_INTEN_RISE0_SHIFT + i)); > + BIT(EXYNOS7_TMU_INTEN_RISE0_SHIFT + i); > } > > interrupt_en |= > interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT; > > - con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT); > + con |= BIT(EXYNOS_TMU_CORE_EN_SHIFT); > } else Minor issue: the if-else segment here. When the 'if' has the brackets, then the 'else' should have them as well, even if there is only a single line under 'else'. It's not strictly to this patch, but you can address that later somewhere (just saw it here). > - con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT); > + con &= ~BIT(EXYNOS_TMU_CORE_EN_SHIFT); > > pd_det_en = on ? EXYNOS5433_PD_DET_EN : 0; > > @@ -622,17 +622,17 @@ static void exynos7_tmu_control(struct platform_device *pdev, bool on) > continue; > > interrupt_en |= > - (1 << (EXYNOS7_TMU_INTEN_RISE0_SHIFT + i)); > + BIT(EXYNOS7_TMU_INTEN_RISE0_SHIFT + i); > } > > interrupt_en |= > interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT; > > - con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT); > - con |= (1 << EXYNOS7_PD_DET_EN_SHIFT); > + con |= BIT(EXYNOS_TMU_CORE_EN_SHIFT); > + con |= BIT(EXYNOS7_PD_DET_EN_SHIFT); > } else { > - con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT); > - con &= ~(1 << EXYNOS7_PD_DET_EN_SHIFT); > + con &= ~BIT(EXYNOS_TMU_CORE_EN_SHIFT); > + con &= ~BIT(EXYNOS7_PD_DET_EN_SHIFT); > } > > writel(interrupt_en, data->base + EXYNOS7_TMU_REG_INTEN); The patch LGTM, Reviewed-by Lukasz Luba <lukasz.luba@arm.com> Regards, Lukasz
Hi, > > @@ -590,15 +590,15 @@ static void exynos5433_tmu_control(struct platform_device *pdev, bool on) > > continue; > > > > interrupt_en |= > > - (1 << (EXYNOS7_TMU_INTEN_RISE0_SHIFT + i)); > > + BIT(EXYNOS7_TMU_INTEN_RISE0_SHIFT + i); > > } > > > > interrupt_en |= > > interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT; > > > > - con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT); > > + con |= BIT(EXYNOS_TMU_CORE_EN_SHIFT); > > } else > > Minor issue: the if-else segment here. When the 'if' has the > brackets, then the 'else' should have them as well, > even if there is only a single line under 'else'. > It's not strictly to this patch, but you can address that > later somewhere (just saw it here). For what it's worth, this issue disappears after the final patch of this series, because the other branch reduces to a single line too (all the interrupt_en operations are done in the tmu_set_*_temp functions). Thank you :) Mateusz
On 11/21/23 12:54, Mateusz Majewski wrote: > Hi, > >>> @@ -590,15 +590,15 @@ static void exynos5433_tmu_control(struct platform_device *pdev, bool on) >>> continue; >>> >>> interrupt_en |= >>> - (1 << (EXYNOS7_TMU_INTEN_RISE0_SHIFT + i)); >>> + BIT(EXYNOS7_TMU_INTEN_RISE0_SHIFT + i); >>> } >>> >>> interrupt_en |= >>> interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT; >>> >>> - con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT); >>> + con |= BIT(EXYNOS_TMU_CORE_EN_SHIFT); >>> } else >> >> Minor issue: the if-else segment here. When the 'if' has the >> brackets, then the 'else' should have them as well, >> even if there is only a single line under 'else'. >> It's not strictly to this patch, but you can address that >> later somewhere (just saw it here). > > For what it's worth, this issue disappears after the final patch of this series, > because the other branch reduces to a single line too (all the interrupt_en > operations are done in the tmu_set_*_temp functions). That sounds perfect. I'm planning to build&run the patch set today evening, so I will finish the review of the patch 9/9.
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c index 6b3a7dd05c68..40e250c815f8 100644 --- a/drivers/thermal/samsung/exynos_tmu.c +++ b/drivers/thermal/samsung/exynos_tmu.c @@ -388,7 +388,7 @@ static void exynos4412_tmu_set_trip_temp(struct exynos_tmu_data *data, if (trip == 3) { con = readl(data->base + EXYNOS_TMU_REG_CONTROL); - con |= (1 << EXYNOS_TMU_THERM_TRIP_EN_SHIFT); + con |= BIT(EXYNOS_TMU_THERM_TRIP_EN_SHIFT); writel(con, data->base + EXYNOS_TMU_REG_CONTROL); } } @@ -559,16 +559,16 @@ static void exynos4210_tmu_control(struct platform_device *pdev, bool on) continue; interrupt_en |= - (1 << (EXYNOS_TMU_INTEN_RISE0_SHIFT + i * 4)); + BIT(EXYNOS_TMU_INTEN_RISE0_SHIFT + i * 4); } if (data->soc != SOC_ARCH_EXYNOS4210) interrupt_en |= interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT; - con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT); + con |= BIT(EXYNOS_TMU_CORE_EN_SHIFT); } else { - con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT); + con &= ~BIT(EXYNOS_TMU_CORE_EN_SHIFT); } writel(interrupt_en, data->base + EXYNOS_TMU_REG_INTEN); @@ -590,15 +590,15 @@ static void exynos5433_tmu_control(struct platform_device *pdev, bool on) continue; interrupt_en |= - (1 << (EXYNOS7_TMU_INTEN_RISE0_SHIFT + i)); + BIT(EXYNOS7_TMU_INTEN_RISE0_SHIFT + i); } interrupt_en |= interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT; - con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT); + con |= BIT(EXYNOS_TMU_CORE_EN_SHIFT); } else - con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT); + con &= ~BIT(EXYNOS_TMU_CORE_EN_SHIFT); pd_det_en = on ? EXYNOS5433_PD_DET_EN : 0; @@ -622,17 +622,17 @@ static void exynos7_tmu_control(struct platform_device *pdev, bool on) continue; interrupt_en |= - (1 << (EXYNOS7_TMU_INTEN_RISE0_SHIFT + i)); + BIT(EXYNOS7_TMU_INTEN_RISE0_SHIFT + i); } interrupt_en |= interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT; - con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT); - con |= (1 << EXYNOS7_PD_DET_EN_SHIFT); + con |= BIT(EXYNOS_TMU_CORE_EN_SHIFT); + con |= BIT(EXYNOS7_PD_DET_EN_SHIFT); } else { - con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT); - con &= ~(1 << EXYNOS7_PD_DET_EN_SHIFT); + con &= ~BIT(EXYNOS_TMU_CORE_EN_SHIFT); + con &= ~BIT(EXYNOS7_PD_DET_EN_SHIFT); } writel(interrupt_en, data->base + EXYNOS7_TMU_REG_INTEN);
The original driver did not use that macro and it allows us to make our intentions slightly clearer. Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com> --- drivers/thermal/samsung/exynos_tmu.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)