Re: [PATCH V3 05/11] thermal: tegra: add T210-specific SOC_THERM driver

[Wei Ni]

On 2016å01æ21æ 22:25, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Jan 18, 2016 at 06:03:46PM +0800, Wei Ni wrote:
>> Add Tegra210 specific SOC_THERM driver.
>>
>> Signed-off-by: Wei Ni <wni@xxxxxxxxxx>
>> ---
>>  drivers/thermal/tegra/Makefile            |   1 +
>>  drivers/thermal/tegra/soctherm-fuse.c     |  11 ++
>>  drivers/thermal/tegra/soctherm.c          |   6 +
>>  drivers/thermal/tegra/soctherm.h          |   4 +
>>  drivers/thermal/tegra/tegra210-soctherm.c | 181 ++++++++++++++++++++++++++++++
>>  5 files changed, 203 insertions(+)
>>  create mode 100644 drivers/thermal/tegra/tegra210-soctherm.c
> 
> This looks pretty good, just a few minor nits...
> 
>> diff --git a/drivers/thermal/tegra/soctherm.h b/drivers/thermal/tegra/soctherm.h
> [...]
>> index 1ac66cafb392..bd0f03bc9d80 100644
>> --- a/drivers/thermal/tegra/soctherm.h
>> +++ b/drivers/thermal/tegra/soctherm.h
>> @@ -108,5 +108,9 @@ int tegra_soctherm_calculate_tsensor_calibration(
>>  extern struct tegra_soctherm_soc tegra124_soctherm;
>>  #endif
>>  
>> +#ifdef CONFIG_ARCH_TEGRA_210_SOC
>> +extern struct tegra_soctherm_soc tegra210_soctherm;
> 
> I would've expected this to be "const".

Yes, will change it.

> 
>> diff --git a/drivers/thermal/tegra/tegra210-soctherm.c b/drivers/thermal/tegra/tegra210-soctherm.c
> [...]
>> +static const struct tegra_tsensor_group tegra210_tsensor_group_cpu = {
>> +	.id				= TEGRA124_SOCTHERM_SENSOR_CPU,
>> +	.name				= "cpu",
>> +	.sensor_temp_offset		= SENSOR_TEMP1,
>> +	.sensor_temp_mask		= SENSOR_TEMP1_CPU_TEMP_MASK,
>> +	.pdiv				= 8,
>> +	.pdiv_ate			= 8,
>> +	.pdiv_mask			= SENSOR_PDIV_CPU_MASK,
>> +	.pllx_hotspot_diff		= 10,
>> +	.pllx_hotspot_mask		= SENSOR_HOTSPOT_CPU_MASK,
>> +};
> 
> I prefer single spaces for padding because I find it easier to read that
> way. Also using tabs to make this look like a table has the drawback
> that you run the risk of having to adjust padding for all fields when a
> new field is added whose name is longer than any existing ones. Or you
> have to rely on excessive padding (such as in this case) to make that
> unlikely. So I don't see any advantage in this, but I don't have very
> strong objections either.

Ok, will change it.

> 
>> +static const struct tegra_tsensor_group *
>> +tegra210_tsensor_groups[] = {
> 
> Why wrap these two lines? They seem to fit on one line and within 78
> columns.

Hmm, will fix it.

> 
>> +static struct tegra_tsensor tegra210_tsensors[] = {
> 
> "static const"?

Will change to "const".

> 
>> +	{
>> +		.name = "cpu0",
>> +		.base = 0xc0,
>> +		.config = &tegra210_tsensor_config,
>> +		.calib_fuse_offset = 0x098,
>> +		.fuse_corr_alpha = 1085000,
>> +		.fuse_corr_beta = 3244200,
>> +		.group = &tegra210_tsensor_group_cpu,
>> +	},
>> +	{
> 
> "}," and "{" can go on the same line.

Will change it.

> 
>> +struct tegra_soctherm_soc tegra210_soctherm = {
> 
> "const"?
> 
> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1
>