Message-ID: <56977057.2010703@nvidia.com>
Date:	Thu, 14 Jan 2016 17:54:31 +0800
From:	Wei Ni <wni@...dia.com>
To:	Thierry Reding <thierry.reding@...il.com>
CC:	<rui.zhang@...el.com>, <mikko.perttunen@...si.fi>,
	<swarren@...dotorg.org>, <linux-tegra@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V1 03/10] thermal: tegra: split tegra_soctherm driver

Thierry,

On 2016年01月13日 23:04, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Jan 13, 2016 at 03:58:42PM +0800, Wei Ni wrote:
>> Split most of the T124 data and code into a T124-specific driver.
>> Split most of the fuse-related code into a fuse-related source file.
>> Now drivers/thermal/tegra_soctherm.c contains common SOC_THERM library
> 
> That path no longer exists since patch 01/10.

Yes, will remove it.

> 
>> diff --git a/drivers/thermal/tegra/Kconfig b/drivers/thermal/tegra/Kconfig
>> index a6e6cd4528dc..ae7e5e93dab9 100644
>> --- a/drivers/thermal/tegra/Kconfig
>> +++ b/drivers/thermal/tegra/Kconfig
>> @@ -1,6 +1,10 @@
>>  config TEGRA_SOCTHERM
>> -	tristate "Tegra SOCTHERM thermal management"
>> -	depends on ARCH_TEGRA
>> +	bool
>> +
>> +config TEGRA124_SOCTHERM
>> +	bool "Tegra124 SOCTHERM thermal management"
>> +	depends on ARCH_TEGRA_124_SOC
>> +	select TEGRA_SOCTHERM
>>  	help
>>  	  Enable this option for integrated thermal management support on NVIDIA
>>  	  Tegra124 systems-on-chip. The driver supports four thermal zones
> 
> I'd like to do this differently to reduce the number of Kconfig symbols.
> The alternate proposal would be for the TEGRA_SOCTHERM symbol to remain
> as it is and then build in driver support depending on the selected
> ARCH_TEGRA_*_SOC options.

Yes, it will make the kconfig more clear. Will change it.

> 
>> diff --git a/drivers/thermal/tegra/Makefile b/drivers/thermal/tegra/Makefile
>> index 8c51076e4b1e..7a864ec07a25 100644
>> --- a/drivers/thermal/tegra/Makefile
>> +++ b/drivers/thermal/tegra/Makefile
>> @@ -3,4 +3,5 @@
>>  #
>>  
>>  # Tegra soc thermal drivers
>> -obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
>> +obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o tegra_soctherm_fuse.o
>> +obj-$(CONFIG_TEGRA124_SOCTHERM)	+= tegra124_soctherm.o
> 
> So this would look roughly like:
> 
> 	obj-$(CONFIG_TEGRA_SOCTHERM) += tegra-soctherm.o
> 
> 	tegra-soctherm-y := soctherm.c
> 	tegra-soctherm-$(CONFIG_ARCH_TEGRA_124_SOC) += tegra124.o
> 	tegra-soctherm-$(CONFIG_ARCH_TEGRA_210_SOC) += tegra210.o
> 
> Two things to note here: I've replaced the underscore in the filename
> with a dash, because that's more commonly used in filenames for other
> drivers on Tegra. This could be done in patch 01/10 since the file is

Yes, will change to use dash.

> already moved anyway. The second thing to note here is that the SoC-
> generation specific drivers don't contain the redundant _soctherm
> suffix. I suppose that if this directory will ever contain anything
> other than soctherm drivers it might be useful to have the suffix to
> differentiate between different drivers, so feel free to ignore that
> suggestion if you have plans to add other thermal-related drivers to
> this directory.

Yes, I plan to add thermal-throttle driver in this directory, so will keep the
name tegraxxx_soctherm.c

> 
> The advantage of the above is that we'll have a single Kconfig option
> to worry about and also everything will be included in a single driver
> rather than per-SoC "drivers" that merely provide the SoC-specific
> data. This will require some slight changes to the driver code, see
> below.
> 
>> diff --git a/drivers/thermal/tegra/tegra124_soctherm.c b/drivers/thermal/tegra/tegra124_soctherm.c
> [...]
>> +static const struct tegra_tsensor_group *
>> +tegra124_tsensor_groups[TEGRA124_SOCTHERM_SENSOR_NUM] = {
>> +	&tegra124_tsensor_group_cpu,
>> +	&tegra124_tsensor_group_gpu,
>> +	&tegra124_tsensor_group_pll,
>> +	&tegra124_tsensor_group_mem,
>> +};
>> +
>> +static struct tegra_tsensor tegra124_tsensors[] = {
> [...]
>> +	{ .name = NULL },
>> +};
> 
> I think it'd be good not to rely on this sentinel entry being there.
> It's more error-prone than simply storing the number of entries in a
> separate location. See below.

Ok, will do it.

> 
>> +static const struct of_device_id tegra124_soctherm_of_match[] = {
>> +	{ .compatible = "nvidia,tegra124-soctherm" },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, tegra_soctherm_of_match);
> 
> The general idea would be to keep the of_device_id table in the
> tegra_soctherm.c file and have it include entries depending on which SoC
> generation is being supported. See for example the memory controller
> driver in drivers/memory/tegra for a reference that uses this style of
> multi-SoC support.

Got it, will do it.

> 
>> +
>> +static int tegra124_soctherm_probe(struct platform_device *pdev)
>> +{
>> +	return tegra_soctherm_probe(pdev,
>> +				    tegra124_tsensors,
>> +				    tegra124_tsensor_groups,
>> +				    &tegra124_soctherm_fuse);
>> +}
>> +
>> +static struct platform_driver tegra124_soctherm_driver = {
>> +	.probe = tegra124_soctherm_probe,
>> +	.remove = tegra_soctherm_remove,
>> +	.driver = {
>> +		.name = "tegra124_soctherm",
>> +		.of_match_table = tegra124_soctherm_of_match,
>> +	},
>> +};
>> +module_platform_driver(tegra124_soctherm_driver);
>> +
>> +MODULE_AUTHOR("NVIDIA");
>> +MODULE_DESCRIPTION("Tegra124 SOCTHERM thermal management driver");
>> +MODULE_LICENSE("GPL v2");
> 
> With the alternate proposal you can get rid of all of this, which has a
> number of other advantages like not having to export all of the library
> functions that these subdrivers rely on.
> 
> What you'd do instead is add a new structure, along the lines of this:
> 
> 	struct tegra_soctherm_soc {
> 		const struct tegra_tsensor_groups *groups;
> 		unsigned int num_groups;
> 		const struct tegra_tsensor *sensors;
> 		unsigned int num_sensors;
> 		const struct tegra_soctherm_fuse *fuse;
> 	};
> 
> and create one of these with the SoC-specific groups, sensors and FUSE
> parameters. Then you pass these structures to the generic driver as the
> struct of_device_id's .data field:
> 
> 	static const struct of_device_id tegra_soctherm_of_match[] = {
> 		{ .compatible = "nvidia,tegra124-soctherm", .data = &tegra124_soctherm },
> 		{ .compatible = "nvidia,tegra210-soctherm", .data = &tegra210_soctherm },
> 		{ }
> 	};
> 
> Also, please use your name and email address if you're the author.
> Companies don't write code, people do.

Will do it.

> 
>> -static int tegra_soctherm_probe(struct platform_device *pdev)
>> +int tegra_soctherm_probe(struct platform_device *pdev,
>> +			 struct tegra_tsensor *tsensors,
>> +			 const struct tegra_tsensor_group **ttgs,
>> +			 const struct tegra_soctherm_fuse *tfuse)
>>  {
> [...]
>>  }
>>  
>> -static int tegra_soctherm_remove(struct platform_device *pdev)
>> +int tegra_soctherm_remove(struct platform_device *pdev)
>>  {
>>  	struct tegra_soctherm *tegra = platform_get_drvdata(pdev);
>>  	unsigned int i;
>> @@ -564,16 +273,6 @@ static int tegra_soctherm_remove(struct platform_device *pdev)
>>  	return 0;
>>  }
> 
> Both the tegra_soctherm_probe() and tegra_soctherm_remove() functions
> would need to be exported with this type of driver design, otherwise
> building everything as modules the per-SoC drivers couldn't access the
> functions.
> 
> If you create a single driver there is no need to export any of the
> symbols because they are never required by another module.

I will use the new type, so will not export them.

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

Powered by blists - more mailing lists