Re: [PATCH v7 RESEND 2/3] thermal: exynos_tmu: Support new hardware and update TMU interface

[Daniel Lezcano]

Hi Tudor,

On 11/24/25 11:43, Tudor Ambarus wrote:
> Hi, Shin,
>
> On 11/24/25 12:06 PM, 손신 wrote:
> > > > +static void update_con_reg(struct exynos_tmu_data *data) {
> > > > +	u32 val, t_buf_vref_sel, t_buf_slope_sel;
> > > > +
> > > > +	val = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
> > > > +	t_buf_vref_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_SHIFT)
> > > > +				& EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_MASK;
> > > > +	t_buf_slope_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_SHIFT)
> > > > +				& EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_MASK;
> > > > +
> > > > +	val = readl(data->base +  EXYNOSAUTOV920_TMU_REG_CONTROL);
> > > > +	val &= ~(EXYNOS_TMU_REF_VOLTAGE_MASK <<
> > > EXYNOS_TMU_REF_VOLTAGE_SHIFT);
> > > > +	val |= (t_buf_vref_sel << EXYNOS_TMU_REF_VOLTAGE_SHIFT);
> > > > +	val &= ~(EXYNOS_TMU_BUF_SLOPE_SEL_MASK <<
> > > EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
> > > > +	val |= (t_buf_slope_sel << EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
> > > > +	writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
> > > > +
> > > > +	val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
> > > > +	val &= ~(EXYNOSAUTOV920_TMU_NUM_PROBE_MASK <<
> > > EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
> > > > +	val &= ~(EXYNOSAUTOV920_TMU_LPI_MODE_MASK <<
> > > EXYNOSAUTOV920_TMU_LPI_MODE_SHIFT);
> > > > +	val |= (data->sensor_count << EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
> > > > +	writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
> > > > +
> > > > +	writel(1, data->base + EXYNOSAUTOV920_TMU_SAMPLING_INTERVAL);
> > > > +	writel(EXYNOSAUTOV920_TMU_AVG_CON_UPDATE, data->base +
> > > EXYNOSAUTOV920_TMU_REG_AVG_CONTROL);
> > > > +	writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE0_UPDATE,
> > > > +	       data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE0);
> > > > +	writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE1_UPDATE,
> > > > +	       data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE1);
> > > > +}
> > > > +
> > > This is unreadable; please make it understandable for those who don’t have
> > > the documentation (explicit static inline functions, comments, etc ...).
> > I'll restructure this code by introducing explicit static inline helper functions and proper comments to improve readability.
>
> We likely shouldn't use inlines here, see Linus's reply from 2006:
> https://lore.kernel.org/all/Pine.LNX.4.64.0601021105000.3668@xxxxxxxxxxx/T/#u

We should not use inline functions when they can be called from 
different places and if they contain a significant amount of code inside.

But for one line functions, inside a driver it may help for the 
readability when the function name is self-explanatory.


> I guess you can make this easier to read if you use FIELD_GET/SET from
> bitfield.h. Other improvement would be using the regmap api.

regmap would be too overkill to write unshared registers.


--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog