Re: [PATCH V3 04/11] thermal: tegra: split tegra_soctherm driver

[Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 2016年01月21日 22:46, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Jan 18, 2016 at 06:02:29PM +0800, Wei Ni wrote:
> [...]
>> +int tegra_soctherm_calculate_tsensor_calibration(
>> +				struct tegra_tsensor *sensor,
>> +				const struct tsensor_shared_calibration *shared)
> 
> The need to ident weirdly here should be an indication that the function
> name is too long, how about:

Hmm, yes, it's too long.

> 
> int tegra_tsensor_calc_calib(struct tegra_tsensor *sensor,
> 			     const struct tsensor_shared_calibration *shared)
> 
> ?

There have two functions about calibration, I prefer to name them as:
tegra_calc_tsensor_calib() and tegra_calc_shared_calib().

> 
>> +{
>> +	const struct tegra_tsensor_group *sensor_group;
>> +	u32 val, calib;
>> +	s32 actual_tsensor_ft, actual_tsensor_cp;
>> +	s32 delta_sens, delta_temp;
>> +	s32 mult, div;
>> +	s16 therma, thermb;
>> +	int err;
>> +
>> +	sensor_group = sensor->group;
>> +
>> +	err = tegra_fuse_readl(sensor->calib_fuse_offset, &val);
>> +	if (err)
>> +		return err;
>> +
>> +	actual_tsensor_cp = (shared->base_cp * 64) + sign_extend32(val, 12);
>> +	val = (val & FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK)
>> +		>> FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT;
> 
> I think it's more canonical to put the >> on the first line line.

Ok, will fix it.

> 
>> +	actual_tsensor_ft = (shared->base_ft * 32) + sign_extend32(val, 12);
>> +
>> +	delta_sens = actual_tsensor_ft - actual_tsensor_cp;
>> +	delta_temp = shared->actual_temp_ft - shared->actual_temp_cp;
>> +
>> +	mult = sensor_group->pdiv * sensor->config->tsample_ate;
>> +	div = sensor->config->tsample * sensor_group->pdiv_ate;
>> +
>> +	therma = div64_s64_precise((s64)delta_temp * (1LL << 13) * mult,
>> +			(s64)delta_sens * div);
> 
> Are the explicit casts necessary? Shouldn't an s32 be automatically
> promoted to s64? Also arguments on subsequent lines should be aligned
> with the first argument on the first line.

I made a mistake, the div64_s64_precise(s64 a, s64 b) should be
div64_s64_precise(s64 a, s32b), so to make the value more precise, I added (s64)
cast in here.
And I will use temporary variable, and align the arguments.

> 
>> +	thermb = div64_s64_precise(
>> +			((s64)actual_tsensor_ft * shared->actual_temp_cp) -
>> +			((s64)actual_tsensor_cp * shared->actual_temp_ft),
>> +			(s64)delta_sens);
> 
> Perhaps add a temporary variable for the first parameter here for
> readability?

Yes, will use temporary variable, and remove the cast for delta_sens.

> 
>> +
>> +	therma = div64_s64_precise((s64)therma * sensor->fuse_corr_alpha,
>> +			(s64)1000000LL);
>> +	thermb = div64_s64_precise((s64)thermb * sensor->fuse_corr_alpha +
>> +			sensor->fuse_corr_beta,
>> +			(s64)1000000LL);
> 
> What are the 1000000LL? Does it perhaps make sense to have a macro for
> it, or perhaps a comment would help.

It's the coefficient data for the therma and thermb. I will use macro for it.
#define CALIB_COEFFICIENT 1000000LL

> 
>> +	calib = ((u16)therma << SENSOR_CONFIG2_THERMA_SHIFT) |
>> +		 ((u16)thermb << SENSOR_CONFIG2_THERMB_SHIFT);
> 
> Alignment here isn't right.

will do it.

> 
>> diff --git a/drivers/thermal/tegra/soctherm.h b/drivers/thermal/tegra/soctherm.h
> [...]
>> +struct tegra_soctherm_soc {
>> +	struct tegra_tsensor *tsensors;
> 
> Can't these be const? Do they ever need to be modified? If so, they
> should probably not be part of this structure. Or at least only part of
> them should be. The invariant part.
> 
> The reason is that if ever a second instance of this device was present
> both instances would share this same data. I know it's unlikely to
> happen, but setting a bad example would be... well... bad.
> 
> Instead I think if you need to have non-const fields you could separate
> this further into struct tegra_tsensor_soc, with only the static
> information about the sensor, and make struct tegra_tsensor contain a
> pointer to that SoC structure and provide the variable fields in
> addition. That way you can create a struct tegra_tsensor for each struct
> tegra_tsensor_soc and store those per-instance.

There has a member "calib" in the tsensors which will be written in the driver,
so I didn't make it as const.
But you are right, I need to consider the risk that the system have two more SOCs.
I will remove the "calib" to struct tegra_soctherm, then can fix this issue, and
can make all data to "const" in the chip-specific file.

> 
>> diff --git a/drivers/thermal/tegra/tegra124-soctherm.c b/drivers/thermal/tegra/tegra124-soctherm.c
> [...]
>> +static struct tegra_tsensor tegra124_tsensors[] = {
> 
> Can this be "static const" instead?

Will fix this one.

> 
>> +	{
>> +		.name = "cpu0",
>> +		.base = 0xc0,
>> +		.config = &t124_tsensor_config,
>> +		.calib_fuse_offset = 0x098,
>> +		.fuse_corr_alpha = 1135400,
>> +		.fuse_corr_beta = -6266900,
>> +		.group = &tegra124_tsensor_group_cpu,
>> +	},
>> +	{
> 
> "}," and "{" can go on the same line.

Got it, will change it.

> 
>> +struct tegra_soctherm_soc tegra124_soctherm = {
> 
> "const"?

Will fix it.

> 
> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1
>