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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2704 bytes --]

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@nvidia.com>
> ---
>  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".

> 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.

> +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.

> +static struct tegra_tsensor tegra210_tsensors[] = {

"static 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.

> +struct tegra_soctherm_soc tegra210_soctherm = {

"const"?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]