On 2016年01月13日 23:48, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> 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"
Sure, will change it.
>
>> +/**
>> + * 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 '='.
Will fix it.
>
>> + struct device_node *dn;
>
> np would be a more idiomatic variable name for struct device_node *.
>
>> + int i;
>
> This can be unsigned int.
Yes, will fix it and other same items.
>
>> +
>> + 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.
Yes, I will use num_sensor_groups to handle it.
>
>> + 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"?
Got it.
>
>> + 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"?
Will change it.
>
>> #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)
Hmm, yes, I didn't notice it, will fix it.
>
> 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.
Yes, you are right, will fix it.
>
> [...]
>> 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.
Sorry, what's the consistent type you mean?
The "ThermTRIP ANY En" is the bit THERMTRIP_ANY_EN_MASK of register
THERMCTL_THERMTRIP_CTL. How about "Thermtrip Any En"?
Since there have many registers, it's not good to split them into more files. As
I showed it in previous email, this file is decode form, it's easy to read and
check the HW status/registers.
>
>> + 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, ®s_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?
Ok, will do it.
>
> 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
