Date:	Wed, 13 Jan 2016 16:48:18 +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 07/10] thermal: tegra: add thermtrip function

On Wed, Jan 13, 2016 at 04:00:33PM +0800, Wei Ni wrote:
[...]
> diff --git a/drivers/thermal/tegra/tegra_soctherm.c b/drivers/thermal/tegra/tegra_soctherm.c
[...]
> +/**
> + * enforce_temp_range() - check and enforce temperature range [min, max]
> + * @trip_temp: the trip temperature to check
> + *
> + * Checks and enforces the permitted temperature range that SOC_THERM
> + * HW can support This is
> + * done while taking care of precision.
> + *
> + * Return: The precision adjusted capped temperature in millicelsius.
> + */
> +static int enforce_temp_range(struct device *dev, int trip_temp)
> +{
> +	int temp;
> +
> +	temp = clamp_val(trip_temp, min_low_temp, max_high_temp);
> +	if (temp != trip_temp)
> +		dev_info(dev, "soctherm: trip temp %d forced to %d\n",
> +			 trip_temp, temp);

I prefer unabbreviated text in log messages, especially non-debug
messages, so something like this would be more appropriate in my
opinion:

	"soctherm: trip temperature %d forced to %d\n"

> +/**
> + * tegra_soctherm_thermtrip() - configure thermal shutdown from DT data
> + * @dev: struct device * of the SOC_THERM instance
> + *
> + * Configure the SOC_THERM "THERMTRIP" feature, using data from DT.
> + * After it's been configured, THERMTRIP will take action when the
> + * configured SoC thermal sensor group reaches a certain temperature.
> + *
> + * SOC_THERM registers are in the VDD_SOC voltage domain.  This means
> + * that SOC_THERM THERMTRIP programming does not survive an LP0/SC7
> + * transition, unless this driver has been modified to save those
> + * registers before entering SC7 and restore them upon exiting SC7.
> + *
> + * Return: 0 upon success, or a negative error code on failure.
> + * "Success" does not mean that thermtrip was enabled; it could also
> + * mean that no "thermtrip" node was found in DT.  THERMTRIP has been
> + * enabled successfully when a message similar to this one appears on
> + * the serial console: "thermtrip: will shut down when sensor group
> + * XXX reaches YYYYYY millidegrees C"
> + */
> +static int tegra_soctherm_thermtrip(struct device *dev)
> +{
> +	struct tegra_soctherm *ts = dev_get_drvdata(dev);
> +	const struct tegra_tsensor_group **ttgs =  ts->sensor_groups;

Extra space after '='.

> +	struct device_node *dn;

np would be a more idiomatic variable name for struct device_node *.

> +	int i;

This can be unsigned int.

> +
> +	dn = of_find_node_by_name(dev->of_node, "hw-trips");
> +	if (!dn) {
> +		dev_info(dev, "thermtrip: no DT node - not enabling\n");
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i < TEGRA124_SOCTHERM_SENSOR_NUM; ++i) {

Should this not be parameterized based on the number of groups an SoC
generation actually supports? It might be the same on Tegra210 and
Tegra124, but might just as well change in the next generation, so this
seems odd.

> +		const struct tegra_tsensor_group *sg = ttgs[i];
> +		struct device_node *sgdn;
> +		u32 temperature;
> +		int r;
> +
> +		sgdn = of_find_node_by_name(dn, sg->name);
> +		if (!sgdn) {
> +			dev_info(dev,
> +				 "thermtrip: %s: skip due to no configuration\n",
> +				 sg->name);

Perhaps: "thermtrip: %s: no configuration found, skipping\n"?

> +			continue;
> +		}
> +
> +		r = of_property_read_u32(sgdn, "therm-temp", &temperature);
> +		if (r) {
> +			dev_err(dev,
> +				"thermtrip: %s: missing temperature property\n",

"missing shutdown temperature"? Or "shutdown-temperature property not
found"?

>  #ifdef CONFIG_DEBUG_FS
> +static const struct tegra_tsensor_group *find_sensor_group_by_id(
> +						struct tegra_soctherm *ts,
> +						int id)

That's an odd way to wrap lines. A more idiomatic way would be:

	static const struct tegra_tsensor_group *
	find_sensor_group_by_id(struct tegra_soctherm *ts, int id)

Also struct tegra_tsensor.id is u8, perhaps id should be u8 here as
well.

> +{
> +	int i;

Can be unsigned int.

> +
> +	if ((id < 0) || (id > TEGRA124_SOCTHERM_SENSOR_NUM))

If you make id u8, there's no need for the first check.

> +static int thermtrip_read(struct platform_device *pdev,
> +			  int id, u32 *temp)

Same comment about the id parameter.

> +{
> +	struct tegra_soctherm *ts = platform_get_drvdata(pdev);
> +	const struct tegra_tsensor_group *sg;
> +	u32 state;
> +	int r;

r is used to store the return value of readl(), so it should be u32.

> +
> +	sg = find_sensor_group_by_id(ts, id);
> +	if (IS_ERR(sg)) {
> +		dev_err(&pdev->dev, "Read thermtrip failed\n");
> +		return -EINVAL;
> +	}
> +
> +	r = readl(ts->regs + THERMCTL_THERMTRIP_CTL);
> +	state = REG_GET_MASK(r, sg->thermtrip_threshold_mask);
> +	state *= sg->thresh_grain;
> +	*temp = state;
> +
> +	return 0;
> +}
> +
> +static int thermtrip_write(struct platform_device *pdev,
> +			   int id, int temp)

u8 id?

> +{
> +	struct tegra_soctherm *ts = platform_get_drvdata(pdev);
> +	const struct tegra_tsensor_group *sg;
> +	u32 state;
> +	int r;

I'd lean towards adding another variable, say "err" to store the return
value from functions and make that int. Then you can make r a u32 since
it stores the result of a 32-bit register read.

[...]
>  static int regs_show(struct seq_file *s, void *data)
>  {
>  	struct platform_device *pdev = s->private;
>  	struct tegra_soctherm *ts = platform_get_drvdata(pdev);
>  	struct tegra_tsensor *tsensors = ts->tsensors;
> +	const struct tegra_tsensor_group **ttgs = ts->sensor_groups;
>  	u32 r, state;
>  	int i;
>  
> @@ -229,6 +466,17 @@ static int regs_show(struct seq_file *s, void *data)
>  	state = REG_GET_MASK(r, SENSOR_TEMP2_MEM_TEMP_MASK);
>  	seq_printf(s, " MEM(%d)\n", translate_temp(state));
>  
> +	r = readl(ts->regs + THERMCTL_THERMTRIP_CTL);
> +	state = REG_GET_MASK(r, ttgs[0]->thermtrip_any_en_mask);
> +	seq_printf(s, "ThermTRIP ANY En(%d)\n", state);

The spelling here is inconsistent with the other text in this file.
Could this be rewritten to use a more consistent style that might at the
same time be easier to parse? I'm thinking something like a list of
"key: value" strings. Or, like I said earlier, maybe split this up into
more files to make it less complicated to read.

> +	for (i = 0; i < TEGRA124_SOCTHERM_SENSOR_NUM; i++) {
> +		state = REG_GET_MASK(r, ttgs[i]->thermtrip_enable_mask);
> +		seq_printf(s, "     %s En(%d) ", ttgs[i]->name, state);
> +		state = REG_GET_MASK(r, ttgs[i]->thermtrip_threshold_mask);
> +		state *= ttgs[i]->thresh_grain;
> +		seq_printf(s, "Thresh(%d)\n", state);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -251,6 +499,12 @@ static int soctherm_debug_init(struct platform_device *pdev)
>  	tegra_soctherm_root = debugfs_create_dir("tegra_soctherm", NULL);
>  	debugfs_create_file("regs", 0644, tegra_soctherm_root,
>  			    pdev, &regs_fops);
> +	debugfs_create_file("cpu_thermtrip", S_IRUGO | S_IWUSR,
> +			    tegra_soctherm_root, pdev, &cpu_thermtrip_fops);
> +	debugfs_create_file("gpu_thermtrip", S_IRUGO | S_IWUSR,
> +			    tegra_soctherm_root, pdev, &gpu_thermtrip_fops);
> +	debugfs_create_file("pll_thermtrip", S_IRUGO | S_IWUSR,
> +			    tegra_soctherm_root, pdev, &pll_thermtrip_fops);

Perhaps create a subdirectory named "thermtrip" and add "cpu", "gpu" and
"pll" files in it for better grouping?

Thierry

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

Powered by blists - more mailing lists