diff mbox series

[v5,8/9] thermal: exynos: use BIT wherever possible

Message ID 20231120145049.310509-9-m.majewski2@samsung.com (mailing list archive)
State New, archived
Delegated to: Daniel Lezcano
Headers show
Series [v5,1/9] thermal: exynos: remove an unnecessary field description | expand

Commit Message

Mateusz Majewski Nov. 20, 2023, 2:50 p.m. UTC 
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(-)

Comments

Lukasz Luba Nov. 20, 2023, 3:24 p.m. UTC | #1 
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
Mateusz Majewski Nov. 21, 2023, 12:54 p.m. UTC | #2 
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
Lukasz Luba Nov. 21, 2023, 1:07 p.m. UTC | #3 
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 mbox series

Patch

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);