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 <[email protected]>
>> ---
>> 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
>
