From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753176AbcAYFrs (ORCPT ); Mon, 25 Jan 2016 00:47:48 -0500 Received: from hqemgate16.nvidia.com ([216.228.121.65]:18433 "EHLO hqemgate16.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751087AbcAYFrp convert rfc822-to-8bit (ORCPT ); Mon, 25 Jan 2016 00:47:45 -0500 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Sun, 24 Jan 2016 21:48:40 -0800 Message-ID: <56A5B73B.6040706@nvidia.com> Date: Mon, 25 Jan 2016 13:48:43 +0800 From: Wei Ni User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Thierry Reding CC: , , , , , Subject: Re: [PATCH V3 05/11] thermal: tegra: add T210-specific SOC_THERM driver References: <1453111426-12356-1-git-send-email-wni@nvidia.com> <20160121142546.GA32301@ulmo> In-Reply-To: <20160121142546.GA32301@ulmo> X-Originating-IP: [10.19.224.146] X-ClientProxiedBy: DRBGMAIL104.nvidia.com (10.18.16.23) To HKMAIL101.nvidia.com (10.18.16.10) Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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 >