Message-ID: <20160113150433.GC2588@ulmo>
Date:	Wed, 13 Jan 2016 16:04:33 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Wei Ni <wni@...dia.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

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.

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

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

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.

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

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

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

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists