| Message ID | 20240807084829.1037303-1-m.majewski2@samsung.com |
|---|---|
| Headers | show |
| Series | Add initial Exynos850 support to the thermal driver | expand |
On 07/08/2024 10:48, Mateusz Majewski wrote: > There are two minor issues regarding this function. > > One is that it attempts to calculate the second calibration value even > if 1-point trimming is being used; in this case, the calculated value is > probably not useful and is never used anyway. Changing this also > requires a minor reordering in Exynos5433 initialization function, so > that we know which type of trimming is used before we call > sanitize_temp_error. > > The second issue is that the function is not very consistent when it > comes to the use of Exynos7-specific parameters. This seems to not be an > issue in practice, in part because some of these issues are related to > the mentioned calculation of the second calibration value. However, > fixing this makes the code a bit less confusing, and will be required > for Exynos850 which has 9-bit temperature values and uses 2-point > trimming. May I suggest to convert this function to a specific soc ops to be put in the struct exynos_tmu_data ? > Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com> > --- > v1 -> v2: reworked to change shift instead of only mask and to also fix > the 2-point trimming issue. > > drivers/thermal/samsung/exynos_tmu.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c > index b68e9755c933..087a09628e23 100644 > --- a/drivers/thermal/samsung/exynos_tmu.c > +++ b/drivers/thermal/samsung/exynos_tmu.c > @@ -111,6 +111,7 @@ > #define EXYNOS7_TMU_REG_EMUL_CON 0x160 > > #define EXYNOS7_TMU_TEMP_MASK 0x1ff > +#define EXYNOS7_TMU_TEMP_SHIFT 9 > #define EXYNOS7_PD_DET_EN_SHIFT 23 > #define EXYNOS7_TMU_INTEN_RISE0_SHIFT 0 > #define EXYNOS7_EMUL_DATA_SHIFT 7 > @@ -234,20 +235,23 @@ static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info) > u16 tmu_temp_mask = > (data->soc == SOC_ARCH_EXYNOS7) ? EXYNOS7_TMU_TEMP_MASK > : EXYNOS_TMU_TEMP_MASK; > + int tmu_85_shift = > + (data->soc == SOC_ARCH_EXYNOS7) ? EXYNOS7_TMU_TEMP_SHIFT > + : EXYNOS_TRIMINFO_85_SHIFT; > > data->temp_error1 = trim_info & tmu_temp_mask; > - data->temp_error2 = ((trim_info >> EXYNOS_TRIMINFO_85_SHIFT) & > - EXYNOS_TMU_TEMP_MASK); > - > if (!data->temp_error1 || > (data->min_efuse_value > data->temp_error1) || > (data->temp_error1 > data->max_efuse_value)) > - data->temp_error1 = data->efuse_value & EXYNOS_TMU_TEMP_MASK; > + data->temp_error1 = data->efuse_value & tmu_temp_mask; > > - if (!data->temp_error2) > - data->temp_error2 = > - (data->efuse_value >> EXYNOS_TRIMINFO_85_SHIFT) & > - EXYNOS_TMU_TEMP_MASK; > + if (data->cal_type == TYPE_TWO_POINT_TRIMMING) { > + data->temp_error2 = (trim_info >> tmu_85_shift) & tmu_temp_mask; > + if (!data->temp_error2) > + data->temp_error2 = > + (data->efuse_value >> tmu_85_shift) & > + tmu_temp_mask; > + } > } > > static int exynos_tmu_initialize(struct platform_device *pdev) > @@ -510,7 +514,6 @@ static void exynos5433_tmu_initialize(struct platform_device *pdev) > int sensor_id, cal_type; > > trim_info = readl(data->base + EXYNOS_TMU_REG_TRIMINFO); > - sanitize_temp_error(data, trim_info); > > /* Read the temperature sensor id */ > sensor_id = (trim_info & EXYNOS5433_TRIMINFO_SENSOR_ID_MASK) > @@ -532,6 +535,8 @@ static void exynos5433_tmu_initialize(struct platform_device *pdev) > break; > } > > + sanitize_temp_error(data, trim_info); > + > dev_info(&pdev->dev, "Calibration type is %d-point calibration\n", > cal_type ? 2 : 1); > }
Hello :) > May I suggest to convert this function to a specific soc ops to be put > in the struct exynos_tmu_data ? Like this? static void exynos4210_sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info) { data->temp_error1 = trim_info & EXYNOS_TMU_TEMP_MASK; if (!data->temp_error1 || (data->min_efuse_value > data->temp_error1) || (data->temp_error1 > data->max_efuse_value)) data->temp_error1 = data->efuse_value & EXYNOS_TMU_TEMP_MASK; WARN_ON_ONCE(data->cal_type == TYPE_TWO_POINT_TRIMMING); } static void exynos5433_sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info) { data->temp_error1 = trim_info & EXYNOS_TMU_TEMP_MASK; if (!data->temp_error1 || (data->min_efuse_value > data->temp_error1) || (data->temp_error1 > data->max_efuse_value)) data->temp_error1 = data->efuse_value & EXYNOS_TMU_TEMP_MASK; if (data->cal_type == TYPE_TWO_POINT_TRIMMING) { data->temp_error2 = (trim_info >> EXYNOS_TRIMINFO_85_SHIFT) & EXYNOS_TMU_TEMP_MASK; if (!data->temp_error2) data->temp_error2 = (data->efuse_value >> EXYNOS_TRIMINFO_85_SHIFT) & EXYNOS_TMU_TEMP_MASK; } } static void exynos7_sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info) { data->temp_error1 = trim_info & EXYNOS7_TMU_TEMP_MASK; if (!data->temp_error1 || (data->min_efuse_value > data->temp_error1) || (data->temp_error1 > data->max_efuse_value)) data->temp_error1 = data->efuse_value & EXYNOS7_TMU_TEMP_MASK; WARN_ON_ONCE(data->cal_type == TYPE_TWO_POINT_TRIMMING); } static void exynos850_sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info) { data->temp_error1 = trim_info & EXYNOS7_TMU_TEMP_MASK; if (!data->temp_error1 || (data->min_efuse_value > data->temp_error1) || (data->temp_error1 > data->max_efuse_value)) data->temp_error1 = data->efuse_value & EXYNOS7_TMU_TEMP_MASK; if (data->cal_type == TYPE_TWO_POINT_TRIMMING) { data->temp_error2 = (trim_info >> EXYNOS7_TMU_TEMP_SHIFT) & EXYNOS7_TMU_TEMP_MASK; if (!data->temp_error2) data->temp_error2 = (data->efuse_value >> EXYNOS7_TMU_TEMP_SHIFT) & EXYNOS_TMU_TEMP_MASK; } } Or maybe we could put tmu_temp_mask and tmu_85_shift in data instead, and have one function like this: static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info) { data->temp_error1 = trim_info & data->tmu_temp_mask; if (!data->temp_error1 || (data->min_efuse_value > data->temp_error1) || (data->temp_error1 > data->max_efuse_value)) data->temp_error1 = data->efuse_value & data->tmu_temp_mask; if (data->cal_type == TYPE_TWO_POINT_TRIMMING) { data->temp_error2 = (trim_info >> data->tmu_85_shift) & data->tmu_temp_mask; if (!data->temp_error2) data->temp_error2 = (data->efuse_value >> data->tmu_85_shift) & data->tmu_temp_mask; } } Thank you, Mateusz
On 03/09/2024 09:19, Mateusz Majewski wrote: > Hello :) > >> May I suggest to convert this function to a specific soc ops to be put >> in the struct exynos_tmu_data ? > > Like this? > > static void exynos4210_sanitize_temp_error(struct exynos_tmu_data *data, > u32 trim_info) > { > data->temp_error1 = trim_info & EXYNOS_TMU_TEMP_MASK; > if (!data->temp_error1 || > (data->min_efuse_value > data->temp_error1) || > (data->temp_error1 > data->max_efuse_value)) > data->temp_error1 = data->efuse_value & EXYNOS_TMU_TEMP_MASK; > WARN_ON_ONCE(data->cal_type == TYPE_TWO_POINT_TRIMMING); > } > > static void exynos5433_sanitize_temp_error(struct exynos_tmu_data *data, > u32 trim_info) > { > data->temp_error1 = trim_info & EXYNOS_TMU_TEMP_MASK; > if (!data->temp_error1 || > (data->min_efuse_value > data->temp_error1) || > (data->temp_error1 > data->max_efuse_value)) > data->temp_error1 = data->efuse_value & EXYNOS_TMU_TEMP_MASK; > > if (data->cal_type == TYPE_TWO_POINT_TRIMMING) { > data->temp_error2 = (trim_info >> EXYNOS_TRIMINFO_85_SHIFT) & > EXYNOS_TMU_TEMP_MASK; > if (!data->temp_error2) > data->temp_error2 = (data->efuse_value >> > EXYNOS_TRIMINFO_85_SHIFT) & > EXYNOS_TMU_TEMP_MASK; > } > } > > static void exynos7_sanitize_temp_error(struct exynos_tmu_data *data, > u32 trim_info) > { > data->temp_error1 = trim_info & EXYNOS7_TMU_TEMP_MASK; > if (!data->temp_error1 || > (data->min_efuse_value > data->temp_error1) || > (data->temp_error1 > data->max_efuse_value)) > data->temp_error1 = data->efuse_value & EXYNOS7_TMU_TEMP_MASK; > WARN_ON_ONCE(data->cal_type == TYPE_TWO_POINT_TRIMMING); > } > > static void exynos850_sanitize_temp_error(struct exynos_tmu_data *data, > u32 trim_info) > { > data->temp_error1 = trim_info & EXYNOS7_TMU_TEMP_MASK; > if (!data->temp_error1 || > (data->min_efuse_value > data->temp_error1) || > (data->temp_error1 > data->max_efuse_value)) > data->temp_error1 = data->efuse_value & EXYNOS7_TMU_TEMP_MASK; > > if (data->cal_type == TYPE_TWO_POINT_TRIMMING) { > data->temp_error2 = (trim_info >> EXYNOS7_TMU_TEMP_SHIFT) & > EXYNOS7_TMU_TEMP_MASK; > if (!data->temp_error2) > data->temp_error2 = (data->efuse_value >> > EXYNOS7_TMU_TEMP_SHIFT) & > EXYNOS_TMU_TEMP_MASK; > } > } Yes, something like that but may be with more code factoring, like a common routine to pass the temp_mask and the specific chunk of code. > Or maybe we could put tmu_temp_mask and tmu_85_shift in data instead, > and have one function like this: Either way It would be nice if the code can be commented to explain how the sensor works in order to understand what means "sanitize the temp error" > static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info) > { > data->temp_error1 = trim_info & data->tmu_temp_mask; > if (!data->temp_error1 || (data->min_efuse_value > data->temp_error1) || > (data->temp_error1 > data->max_efuse_value)) > data->temp_error1 = data->efuse_value & data->tmu_temp_mask; > > if (data->cal_type == TYPE_TWO_POINT_TRIMMING) { > data->temp_error2 = (trim_info >> data->tmu_85_shift) & > data->tmu_temp_mask; > if (!data->temp_error2) > data->temp_error2 = > (data->efuse_value >> data->tmu_85_shift) & > data->tmu_temp_mask; > } > } > > Thank you, > Mateusz
This series adds initial Exynos850 support to the thermal driver together with its requirements (sanitize_temp_error fix, adding the new string to dt-bindings), while also cleaning up a bit (improving power management support and removing some outdated information from dt-bindings). Changelog: v3: - Reworded the commit message of the dt-binding information removal change v2: - Reimplemented to use the Exynos850 TMU clock: removed the patch to make the clock optional and changed dt-bindings change accordingly - Improved the Exynos850 implementation itself (style and one correct register offset) - Removed conditional compilation in favor of pm_sleep_ptr - Shortened dt-bindings description Mateusz Majewski (6): drivers/thermal/exynos: use DEFINE_SIMPLE_DEV_PM_OPS drivers/thermal/exynos: use pm_sleep_ptr instead of conditional compilation drivers/thermal/exynos: improve sanitize_temp_error dt-bindings: thermal: samsung,exynos: add exynos850-tmu string drivers/thermal/exynos: add initial Exynos850 support dt-bindings: thermal: samsung,exynos: remove driver-specific information .../thermal/samsung,exynos-thermal.yaml | 8 +- drivers/thermal/samsung/exynos_tmu.c | 219 ++++++++++++++++-- 2 files changed, 199 insertions(+), 28 deletions(-)