On Wed, Jul 24, 2024 at 10:31 AM Mateusz Majewski <m.majewski2@xxxxxxxxxxx> 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.
