linux-kernel - Re: [PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850 support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPLW+4ktoygdOcy1_isdhsWvfnziR+wzhs8OtUwsn7E2y7HFzQ@mail.gmail.com>
Date: Wed, 24 Jul 2024 19:56:12 -0500
From: Sam Protsenko <semen.protsenko@...aro.org>
To: Mateusz Majewski <m.majewski2@...sung.com>
Cc: linux-pm@...r.kernel.org, linux-samsung-soc@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, Bartlomiej Zolnierkiewicz <bzolnier@...il.com>, 
	Krzysztof Kozlowski <krzk@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, Zhang Rui <rui.zhang@...el.com>, 
	Lukasz Luba <lukasz.luba@....com>, Rob Herring <robh@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Alim Akhtar <alim.akhtar@...sung.com>
Subject: Re: [PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850 support

On Wed, Jul 24, 2024 at 10:31 AM Mateusz Majewski
<m.majewski2@...sung.com> wrote:
>
> > I'd suggest to group all the definitions here as such:
> >
> > #define REG1_OFFSET
> > #define REG1_FIELD1_OFFSET
> > #define REG1_FIELD2_OFFSET
> > ...empty line...
> > #define REG2_OFFSET
> > #define REG2_FIELD1_OFFSET
> > #define REG2_FIELD2_OFFSET
> > ...etc...
> >
> > Or otherwise each shift/mask constant should contain its register name
> > as a prefix, to avoid confusion. But right now it's kinda hard to
> > understand what belongs to what :) But that's just a nitpick.
>
> I came up with this:
>
> /* Exynos850 specific registers */
> #define EXYNOS850_TMU_REG_CURRENT_TEMP0_1       0x40
> #define EXYNOS850_TMU_REG_THD_TEMP0_RISE        0x50
> #define EXYNOS850_TMU_REG_THD_TEMP0_FALL        0x60
> #define EXYNOS850_TMU_TEMP_SHIFT                9
>
> #define EXYNOS850_TMU_TRIMINFO_SHIFT            4
> #define EXYNOS850_TMU_TRIMINFO_OFFSET(n) \
>         (EXYNOS_TMU_REG_TRIMINFO + (n) * EXYNOS850_TMU_TRIMINFO_SHIFT)
> #define EXYNOS850_TMU_T_TRIM0_SHIFT             18
>
> #define EXYNOS850_TMU_REG_CONTROL1              0x24
> #define EXYNOS850_TMU_LPI_MODE_MASK             1
> #define EXYNOS850_TMU_LPI_MODE_SHIFT            10
>
> #define EXYNOS850_TMU_REG_COUNTER_VALUE0        0x30
> #define EXYNOS850_TMU_EN_TEMP_SEN_OFF_MASK      0xffff
> #define EXYNOS850_TMU_EN_TEMP_SEN_OFF_SHIFT     0
>
> #define EXYNOS850_TMU_REG_COUNTER_VALUE1        0x34
> #define EXYNOS850_TMU_CLK_SENSE_ON_MASK         0xffff
> #define EXYNOS850_TMU_CLK_SENSE_ON_SHIFT        16
>
> #define EXYNOS850_TMU_REG_AVG_CON               0x38
> #define EXYNOS850_TMU_AVG_MODE_MASK             0x7
> #define EXYNOS850_TMU_DEM_ENABLE                BIT(4)
>
> #define EXYNOS850_TMU_REG_TRIM0                 0x3c
> #define EXYNOS850_TMU_TRIM0_MASK                0xf
> #define EXYNOS850_TMU_VBEI_TRIM_SHIFT           8
> #define EXYNOS850_TMU_VREF_TRIM_SHIFT           12
> #define EXYNOS850_TMU_BGRI_TRIM_SHIFT           20
>
> #define EXYNOS850_TMU_TEM1051X_SENSE_VALUE      0x028a
> #define EXYNOS850_TMU_TEM1456X_SENSE_VALUE      0x0a28
>

Looks better, thanks!

> This also omits some definitions that were in v1, as they had the same
> value and they were the same thing anyway. For instance, I dropped
> EXYNOS850_TMU_T_BUF_VREF_SEL_MASK in favor of
> EXYNOS_TMU_REF_VOLTAGE_MASK, and have a single EXYNOS850_TMU_TRIM0_MASK
> instead of EXYNOS850_TMU_BGRI_TRIM_MASK, EXYNOS850_TMU_VREF_TRIM_MASK,
> EXYNOS850_TMU_VBEI_TRIM_MASK and EXYNOS850_TMU_T_TRIM0_MASK. Also,
>
> > Suggest using GENMASK() macro whenever possible.
>
> This would make me have a separate mask for each of these again. Maybe
> if this driver gets refactored in the future to use u32_get_bits() and
> so on this would make more sense?

Sure, that was just a suggestion, don't have a strong opinion on that
one. If you don't like it, feel free to skip it for now.