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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
