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 for Android: free password hash cracker in your pocket

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

Message-ID: <20240724153058.914693-1-m.majewski2@samsung.com>
Date: Wed, 24 Jul 2024 17:30:56 +0200
From: Mateusz Majewski <m.majewski2@...sung.com>
To: Sam Protsenko <semen.protsenko@...aro.org>
Cc: Mateusz Majewski <m.majewski2@...sung.com>, 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

> 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

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?