* [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register
@ 2018-11-28 5:44 ` Wei Ni
0 siblings, 0 replies; 19+ messages in thread
From: Wei Ni @ 2018-11-28 5:44 UTC (permalink / raw)
To: thierry.reding, daniel.lezcano, linux-tegra
Cc: rui.zhang, edubezval, srikars, linux-kernel, Wei Ni
Since different platforms may not support all 4
sensors, so the sensor registration may be failed.
Add codes to parse dt to find sensor id which
need to be registered. So that the registration
can be successful on all platform.
Signed-off-by: Wei Ni <wni@nvidia.com>
---
drivers/thermal/tegra/soctherm.c | 46 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 44 insertions(+), 2 deletions(-)
diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
index 375cadbc24cd..79e4628224d7 100644
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -1224,6 +1224,44 @@ static void soctherm_init(struct platform_device *pdev)
tegra_soctherm_throttle(&pdev->dev);
}
+static bool tegra_soctherm_find_sensor_id(int sensor_id)
+{
+ int id;
+ bool ret = false;
+ struct of_phandle_args sensor_specs;
+ struct device_node *np, *sensor_np;
+
+ np = of_find_node_by_name(NULL, "thermal-zones");
+ if (!np)
+ return ret;
+
+ sensor_np = of_get_next_child(np, NULL);
+ for_each_available_child_of_node(np, sensor_np) {
+ if (of_parse_phandle_with_args(sensor_np, "thermal-sensors",
+ "#thermal-sensor-cells",
+ 0, &sensor_specs))
+ continue;
+
+ if (sensor_specs.args_count != 1) {
+ WARN(sensor_specs.args_count > 1,
+ "%s: wrong cells in sensor specifier %d\n",
+ sensor_specs.np->name, sensor_specs.args_count);
+ continue;
+ } else {
+ id = sensor_specs.args[0];
+ if (sensor_id == id) {
+ ret = true;
+ break;
+ }
+ }
+ }
+
+ of_node_put(np);
+ of_node_put(sensor_np);
+
+ return ret;
+}
+
static const struct of_device_id tegra_soctherm_of_match[] = {
#ifdef CONFIG_ARCH_TEGRA_124_SOC
{
@@ -1365,13 +1403,15 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
zone->sg = soc->ttgs[i];
zone->ts = tegra;
+ if (!tegra_soctherm_find_sensor_id(soc->ttgs[i]->id))
+ continue;
z = devm_thermal_zone_of_sensor_register(&pdev->dev,
soc->ttgs[i]->id, zone,
&tegra_of_thermal_ops);
if (IS_ERR(z)) {
err = PTR_ERR(z);
- dev_err(&pdev->dev, "failed to register sensor: %d\n",
- err);
+ dev_err(&pdev->dev, "failed to register sensor %s: %d\n",
+ soc->ttgs[i]->name, err);
goto disable_clocks;
}
@@ -1434,6 +1474,8 @@ static int __maybe_unused soctherm_resume(struct device *dev)
struct thermal_zone_device *tz;
tz = tegra->thermctl_tzs[soc->ttgs[i]->id];
+ if (!tz)
+ continue;
err = tegra_soctherm_set_hwtrips(dev, soc->ttgs[i], tz);
if (err) {
dev_err(&pdev->dev,
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register
2018-11-28 5:44 ` Wei Ni
@ 2018-11-28 5:55 ` Wei Ni
-1 siblings, 0 replies; 19+ messages in thread
From: Wei Ni @ 2018-11-28 5:55 UTC (permalink / raw)
To: thierry.reding, daniel.lezcano, linux-tegra
Cc: rui.zhang, edubezval, srikars, linux-kernel
Hi Daniel,
I updated my patch to parse the sensor id, please take a look.
Wei.
On 28/11/2018 1:44 PM, Wei Ni wrote:
> Since different platforms may not support all 4
> sensors, so the sensor registration may be failed.
> Add codes to parse dt to find sensor id which
> need to be registered. So that the registration
> can be successful on all platform.
>
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
> drivers/thermal/tegra/soctherm.c | 46 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
> index 375cadbc24cd..79e4628224d7 100644
> --- a/drivers/thermal/tegra/soctherm.c
> +++ b/drivers/thermal/tegra/soctherm.c
> @@ -1224,6 +1224,44 @@ static void soctherm_init(struct platform_device *pdev)
> tegra_soctherm_throttle(&pdev->dev);
> }
>
> +static bool tegra_soctherm_find_sensor_id(int sensor_id)
> +{
> + int id;
> + bool ret = false;
> + struct of_phandle_args sensor_specs;
> + struct device_node *np, *sensor_np;
> +
> + np = of_find_node_by_name(NULL, "thermal-zones");
> + if (!np)
> + return ret;
> +
> + sensor_np = of_get_next_child(np, NULL);
> + for_each_available_child_of_node(np, sensor_np) {
> + if (of_parse_phandle_with_args(sensor_np, "thermal-sensors",
> + "#thermal-sensor-cells",
> + 0, &sensor_specs))
> + continue;
> +
> + if (sensor_specs.args_count != 1) {
> + WARN(sensor_specs.args_count > 1,
> + "%s: wrong cells in sensor specifier %d\n",
> + sensor_specs.np->name, sensor_specs.args_count);
> + continue;
> + } else {
> + id = sensor_specs.args[0];
> + if (sensor_id == id) {
> + ret = true;
> + break;
> + }
> + }
> + }
> +
> + of_node_put(np);
> + of_node_put(sensor_np);
> +
> + return ret;
> +}
> +
> static const struct of_device_id tegra_soctherm_of_match[] = {
> #ifdef CONFIG_ARCH_TEGRA_124_SOC
> {
> @@ -1365,13 +1403,15 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
> zone->sg = soc->ttgs[i];
> zone->ts = tegra;
>
> + if (!tegra_soctherm_find_sensor_id(soc->ttgs[i]->id))
> + continue;
> z = devm_thermal_zone_of_sensor_register(&pdev->dev,
> soc->ttgs[i]->id, zone,
> &tegra_of_thermal_ops);
> if (IS_ERR(z)) {
> err = PTR_ERR(z);
> - dev_err(&pdev->dev, "failed to register sensor: %d\n",
> - err);
> + dev_err(&pdev->dev, "failed to register sensor %s: %d\n",
> + soc->ttgs[i]->name, err);
> goto disable_clocks;
> }
>
> @@ -1434,6 +1474,8 @@ static int __maybe_unused soctherm_resume(struct device *dev)
> struct thermal_zone_device *tz;
>
> tz = tegra->thermctl_tzs[soc->ttgs[i]->id];
> + if (!tz)
> + continue;
> err = tegra_soctherm_set_hwtrips(dev, soc->ttgs[i], tz);
> if (err) {
> dev_err(&pdev->dev,
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register
@ 2018-11-28 5:55 ` Wei Ni
0 siblings, 0 replies; 19+ messages in thread
From: Wei Ni @ 2018-11-28 5:55 UTC (permalink / raw)
To: thierry.reding, daniel.lezcano, linux-tegra
Cc: rui.zhang, edubezval, srikars, linux-kernel
Hi Daniel,
I updated my patch to parse the sensor id, please take a look.
Wei.
On 28/11/2018 1:44 PM, Wei Ni wrote:
> Since different platforms may not support all 4
> sensors, so the sensor registration may be failed.
> Add codes to parse dt to find sensor id which
> need to be registered. So that the registration
> can be successful on all platform.
>
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
> drivers/thermal/tegra/soctherm.c | 46 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
> index 375cadbc24cd..79e4628224d7 100644
> --- a/drivers/thermal/tegra/soctherm.c
> +++ b/drivers/thermal/tegra/soctherm.c
> @@ -1224,6 +1224,44 @@ static void soctherm_init(struct platform_device *pdev)
> tegra_soctherm_throttle(&pdev->dev);
> }
>
> +static bool tegra_soctherm_find_sensor_id(int sensor_id)
> +{
> + int id;
> + bool ret = false;
> + struct of_phandle_args sensor_specs;
> + struct device_node *np, *sensor_np;
> +
> + np = of_find_node_by_name(NULL, "thermal-zones");
> + if (!np)
> + return ret;
> +
> + sensor_np = of_get_next_child(np, NULL);
> + for_each_available_child_of_node(np, sensor_np) {
> + if (of_parse_phandle_with_args(sensor_np, "thermal-sensors",
> + "#thermal-sensor-cells",
> + 0, &sensor_specs))
> + continue;
> +
> + if (sensor_specs.args_count != 1) {
> + WARN(sensor_specs.args_count > 1,
> + "%s: wrong cells in sensor specifier %d\n",
> + sensor_specs.np->name, sensor_specs.args_count);
> + continue;
> + } else {
> + id = sensor_specs.args[0];
> + if (sensor_id == id) {
> + ret = true;
> + break;
> + }
> + }
> + }
> +
> + of_node_put(np);
> + of_node_put(sensor_np);
> +
> + return ret;
> +}
> +
> static const struct of_device_id tegra_soctherm_of_match[] = {
> #ifdef CONFIG_ARCH_TEGRA_124_SOC
> {
> @@ -1365,13 +1403,15 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
> zone->sg = soc->ttgs[i];
> zone->ts = tegra;
>
> + if (!tegra_soctherm_find_sensor_id(soc->ttgs[i]->id))
> + continue;
> z = devm_thermal_zone_of_sensor_register(&pdev->dev,
> soc->ttgs[i]->id, zone,
> &tegra_of_thermal_ops);
> if (IS_ERR(z)) {
> err = PTR_ERR(z);
> - dev_err(&pdev->dev, "failed to register sensor: %d\n",
> - err);
> + dev_err(&pdev->dev, "failed to register sensor %s: %d\n",
> + soc->ttgs[i]->name, err);
> goto disable_clocks;
> }
>
> @@ -1434,6 +1474,8 @@ static int __maybe_unused soctherm_resume(struct device *dev)
> struct thermal_zone_device *tz;
>
> tz = tegra->thermctl_tzs[soc->ttgs[i]->id];
> + if (!tz)
> + continue;
> err = tegra_soctherm_set_hwtrips(dev, soc->ttgs[i], tz);
> if (err) {
> dev_err(&pdev->dev,
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register
2018-11-28 5:44 ` Wei Ni
(?)
(?)
@ 2018-11-28 10:25 ` Thierry Reding
2018-11-29 5:55 ` Wei Ni
-1 siblings, 1 reply; 19+ messages in thread
From: Thierry Reding @ 2018-11-28 10:25 UTC (permalink / raw)
To: Wei Ni
Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, srikars,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3656 bytes --]
On Wed, Nov 28, 2018 at 01:44:37PM +0800, Wei Ni wrote:
> Since different platforms may not support all 4
> sensors, so the sensor registration may be failed.
> Add codes to parse dt to find sensor id which
> need to be registered. So that the registration
> can be successful on all platform.
>
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
> drivers/thermal/tegra/soctherm.c | 46 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
> index 375cadbc24cd..79e4628224d7 100644
> --- a/drivers/thermal/tegra/soctherm.c
> +++ b/drivers/thermal/tegra/soctherm.c
> @@ -1224,6 +1224,44 @@ static void soctherm_init(struct platform_device *pdev)
> tegra_soctherm_throttle(&pdev->dev);
> }
>
> +static bool tegra_soctherm_find_sensor_id(int sensor_id)
> +{
> + int id;
You might want to make this and the sensor_id parameter unsigned int to
match the signedness of the ID in the specifier arguments and the sensor
groups.
Thierry
> + bool ret = false;
> + struct of_phandle_args sensor_specs;
> + struct device_node *np, *sensor_np;
> +
> + np = of_find_node_by_name(NULL, "thermal-zones");
> + if (!np)
> + return ret;
> +
> + sensor_np = of_get_next_child(np, NULL);
> + for_each_available_child_of_node(np, sensor_np) {
Aren't we leaking np here? I think we need of_node_put() after
of_get_next_child() to make sure the reference to the "thermal-zones"
node is properly released.
> + if (of_parse_phandle_with_args(sensor_np, "thermal-sensors",
> + "#thermal-sensor-cells",
> + 0, &sensor_specs))
> + continue;
> +
> + if (sensor_specs.args_count != 1) {
> + WARN(sensor_specs.args_count > 1,
> + "%s: wrong cells in sensor specifier %d\n",
> + sensor_specs.np->name, sensor_specs.args_count);
> + continue;
This is odd. You check for args_count != 1 but then WARN on args_count >
1. Shouldn't both of these conditions be the same?
> + } else {
Also, since the above has "continue;", we don't really need the else
block.
> + id = sensor_specs.args[0];
> + if (sensor_id == id) {
It might not be worth to store the ID in a separate variable, we could
just do:
if (sensor_specs.args[0] == sensor_id)
Thierry
> + ret = true;
> + break;
> + }
> + }
> + }
> +
> + of_node_put(np);
> + of_node_put(sensor_np);
> +
> + return ret;
> +}
> +
> static const struct of_device_id tegra_soctherm_of_match[] = {
> #ifdef CONFIG_ARCH_TEGRA_124_SOC
> {
> @@ -1365,13 +1403,15 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
> zone->sg = soc->ttgs[i];
> zone->ts = tegra;
>
> + if (!tegra_soctherm_find_sensor_id(soc->ttgs[i]->id))
> + continue;
> z = devm_thermal_zone_of_sensor_register(&pdev->dev,
I'd would prefer a blank line after the if block for readability.
> soc->ttgs[i]->id, zone,
> &tegra_of_thermal_ops);
> if (IS_ERR(z)) {
> err = PTR_ERR(z);
> - dev_err(&pdev->dev, "failed to register sensor: %d\n",
> - err);
> + dev_err(&pdev->dev, "failed to register sensor %s: %d\n",
> + soc->ttgs[i]->name, err);
> goto disable_clocks;
> }
>
> @@ -1434,6 +1474,8 @@ static int __maybe_unused soctherm_resume(struct device *dev)
> struct thermal_zone_device *tz;
>
> tz = tegra->thermctl_tzs[soc->ttgs[i]->id];
> + if (!tz)
> + continue;
> err = tegra_soctherm_set_hwtrips(dev, soc->ttgs[i], tz);
Same here:
if (!tz)
continue;
err = ...
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register
2018-11-28 10:25 ` Thierry Reding
@ 2018-11-29 5:55 ` Wei Ni
0 siblings, 0 replies; 19+ messages in thread
From: Wei Ni @ 2018-11-29 5:55 UTC (permalink / raw)
To: Thierry Reding
Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, srikars,
linux-kernel
On 28/11/2018 6:25 PM, Thierry Reding wrote:
> On Wed, Nov 28, 2018 at 01:44:37PM +0800, Wei Ni wrote:
>> Since different platforms may not support all 4
>> sensors, so the sensor registration may be failed.
>> Add codes to parse dt to find sensor id which
>> need to be registered. So that the registration
>> can be successful on all platform.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>> drivers/thermal/tegra/soctherm.c | 46 ++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
>> index 375cadbc24cd..79e4628224d7 100644
>> --- a/drivers/thermal/tegra/soctherm.c
>> +++ b/drivers/thermal/tegra/soctherm.c
>> @@ -1224,6 +1224,44 @@ static void soctherm_init(struct platform_device *pdev)
>> tegra_soctherm_throttle(&pdev->dev);
>> }
>>
>> +static bool tegra_soctherm_find_sensor_id(int sensor_id)
>> +{
>> + int id;
>
> You might want to make this and the sensor_id parameter unsigned int to
> match the signedness of the ID in the specifier arguments and the sensor
> groups.
Ok, will change it.
>
> Thierry
>
>> + bool ret = false;
>> + struct of_phandle_args sensor_specs;
>> + struct device_node *np, *sensor_np;
>> +
>> + np = of_find_node_by_name(NULL, "thermal-zones");
>> + if (!np)
>> + return ret;
>> +
>> + sensor_np = of_get_next_child(np, NULL);
>> + for_each_available_child_of_node(np, sensor_np) {
>
> Aren't we leaking np here? I think we need of_node_put() after
> of_get_next_child() to make sure the reference to the "thermal-zones"
> node is properly released.
No, we will not leak np here. Because the
for_each_available_child_of_node will call
of_get_next_available_child(), which will call of_node_put(prev) to
decrease refcount of the prev node. So we just need to of_node_put the
last node after break from this for block.
>
>> + if (of_parse_phandle_with_args(sensor_np, "thermal-sensors",
>> + "#thermal-sensor-cells",
>> + 0, &sensor_specs))
>> + continue;
>> +
>> + if (sensor_specs.args_count != 1) {
>> + WARN(sensor_specs.args_count > 1,
>> + "%s: wrong cells in sensor specifier %d\n",
>> + sensor_specs.np->name, sensor_specs.args_count);
>> + continue;
>
> This is odd. You check for args_count != 1 but then WARN on args_count >
> 1. Shouldn't both of these conditions be the same?
Sorry, it's my mistake, will fix it.
>
>> + } else {
>
> Also, since the above has "continue;", we don't really need the else
> block.
Will fix it.
>
>> + id = sensor_specs.args[0];
>> + if (sensor_id == id) {
>
> It might not be worth to store the ID in a separate variable, we could
> just do:
>
> if (sensor_specs.args[0] == sensor_id)
>
> Thierry
Sure, will fix it.
>> + ret = true;
>> + break;
>> + }
>> + }
>> + }
>> +
>> + of_node_put(np);
We decrease refcount of the last np.
>> + of_node_put(sensor_np);
>> +
>> + return ret;
>> +}
>> +
>> static const struct of_device_id tegra_soctherm_of_match[] = {
>> #ifdef CONFIG_ARCH_TEGRA_124_SOC
>> {
>> @@ -1365,13 +1403,15 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
>> zone->sg = soc->ttgs[i];
>> zone->ts = tegra;
>>
>> + if (!tegra_soctherm_find_sensor_id(soc->ttgs[i]->id))
>> + continue;
>> z = devm_thermal_zone_of_sensor_register(&pdev->dev,
>
> I'd would prefer a blank line after the if block for readability.
Sure, will update it.
>
>> soc->ttgs[i]->id, zone,
>> &tegra_of_thermal_ops);
>> if (IS_ERR(z)) {
>> err = PTR_ERR(z);
>> - dev_err(&pdev->dev, "failed to register sensor: %d\n",
>> - err);
>> + dev_err(&pdev->dev, "failed to register sensor %s: %d\n",
>> + soc->ttgs[i]->name, err);
>> goto disable_clocks;
>> }
>>
>> @@ -1434,6 +1474,8 @@ static int __maybe_unused soctherm_resume(struct device *dev)
>> struct thermal_zone_device *tz;
>>
>> tz = tegra->thermctl_tzs[soc->ttgs[i]->id];
>> + if (!tz)
>> + continue;
>> err = tegra_soctherm_set_hwtrips(dev, soc->ttgs[i], tz);
>
> Same here:
>
> if (!tz)
> continue;
>
> err = ...
>
> Thierry
Will update it.
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register
@ 2018-11-29 5:55 ` Wei Ni
0 siblings, 0 replies; 19+ messages in thread
From: Wei Ni @ 2018-11-29 5:55 UTC (permalink / raw)
To: Thierry Reding
Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, srikars,
linux-kernel
On 28/11/2018 6:25 PM, Thierry Reding wrote:
> On Wed, Nov 28, 2018 at 01:44:37PM +0800, Wei Ni wrote:
>> Since different platforms may not support all 4
>> sensors, so the sensor registration may be failed.
>> Add codes to parse dt to find sensor id which
>> need to be registered. So that the registration
>> can be successful on all platform.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>> drivers/thermal/tegra/soctherm.c | 46 ++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
>> index 375cadbc24cd..79e4628224d7 100644
>> --- a/drivers/thermal/tegra/soctherm.c
>> +++ b/drivers/thermal/tegra/soctherm.c
>> @@ -1224,6 +1224,44 @@ static void soctherm_init(struct platform_device *pdev)
>> tegra_soctherm_throttle(&pdev->dev);
>> }
>>
>> +static bool tegra_soctherm_find_sensor_id(int sensor_id)
>> +{
>> + int id;
>
> You might want to make this and the sensor_id parameter unsigned int to
> match the signedness of the ID in the specifier arguments and the sensor
> groups.
Ok, will change it.
>
> Thierry
>
>> + bool ret = false;
>> + struct of_phandle_args sensor_specs;
>> + struct device_node *np, *sensor_np;
>> +
>> + np = of_find_node_by_name(NULL, "thermal-zones");
>> + if (!np)
>> + return ret;
>> +
>> + sensor_np = of_get_next_child(np, NULL);
>> + for_each_available_child_of_node(np, sensor_np) {
>
> Aren't we leaking np here? I think we need of_node_put() after
> of_get_next_child() to make sure the reference to the "thermal-zones"
> node is properly released.
No, we will not leak np here. Because the
for_each_available_child_of_node will call
of_get_next_available_child(), which will call of_node_put(prev) to
decrease refcount of the prev node. So we just need to of_node_put the
last node after break from this for block.
>
>> + if (of_parse_phandle_with_args(sensor_np, "thermal-sensors",
>> + "#thermal-sensor-cells",
>> + 0, &sensor_specs))
>> + continue;
>> +
>> + if (sensor_specs.args_count != 1) {
>> + WARN(sensor_specs.args_count > 1,
>> + "%s: wrong cells in sensor specifier %d\n",
>> + sensor_specs.np->name, sensor_specs.args_count);
>> + continue;
>
> This is odd. You check for args_count != 1 but then WARN on args_count >
> 1. Shouldn't both of these conditions be the same?
Sorry, it's my mistake, will fix it.
>
>> + } else {
>
> Also, since the above has "continue;", we don't really need the else
> block.
Will fix it.
>
>> + id = sensor_specs.args[0];
>> + if (sensor_id == id) {
>
> It might not be worth to store the ID in a separate variable, we could
> just do:
>
> if (sensor_specs.args[0] == sensor_id)
>
> Thierry
Sure, will fix it.
>> + ret = true;
>> + break;
>> + }
>> + }
>> + }
>> +
>> + of_node_put(np);
We decrease refcount of the last np.
>> + of_node_put(sensor_np);
>> +
>> + return ret;
>> +}
>> +
>> static const struct of_device_id tegra_soctherm_of_match[] = {
>> #ifdef CONFIG_ARCH_TEGRA_124_SOC
>> {
>> @@ -1365,13 +1403,15 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
>> zone->sg = soc->ttgs[i];
>> zone->ts = tegra;
>>
>> + if (!tegra_soctherm_find_sensor_id(soc->ttgs[i]->id))
>> + continue;
>> z = devm_thermal_zone_of_sensor_register(&pdev->dev,
>
> I'd would prefer a blank line after the if block for readability.
Sure, will update it.
>
>> soc->ttgs[i]->id, zone,
>> &tegra_of_thermal_ops);
>> if (IS_ERR(z)) {
>> err = PTR_ERR(z);
>> - dev_err(&pdev->dev, "failed to register sensor: %d\n",
>> - err);
>> + dev_err(&pdev->dev, "failed to register sensor %s: %d\n",
>> + soc->ttgs[i]->name, err);
>> goto disable_clocks;
>> }
>>
>> @@ -1434,6 +1474,8 @@ static int __maybe_unused soctherm_resume(struct device *dev)
>> struct thermal_zone_device *tz;
>>
>> tz = tegra->thermctl_tzs[soc->ttgs[i]->id];
>> + if (!tz)
>> + continue;
>> err = tegra_soctherm_set_hwtrips(dev, soc->ttgs[i], tz);
>
> Same here:
>
> if (!tz)
> continue;
>
> err = ...
>
> Thierry
Will update it.
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register
2018-11-29 5:55 ` Wei Ni
(?)
@ 2018-11-29 17:13 ` Thierry Reding
2018-11-30 3:10 ` Wei Ni
-1 siblings, 1 reply; 19+ messages in thread
From: Thierry Reding @ 2018-11-29 17:13 UTC (permalink / raw)
To: Wei Ni
Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, srikars,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1731 bytes --]
On Thu, Nov 29, 2018 at 01:55:02PM +0800, Wei Ni wrote:
> On 28/11/2018 6:25 PM, Thierry Reding wrote:
> > On Wed, Nov 28, 2018 at 01:44:37PM +0800, Wei Ni wrote:
[...]
> >> + bool ret = false;
> >> + struct of_phandle_args sensor_specs;
> >> + struct device_node *np, *sensor_np;
> >> +
> >> + np = of_find_node_by_name(NULL, "thermal-zones");
> >> + if (!np)
> >> + return ret;
> >> +
> >> + sensor_np = of_get_next_child(np, NULL);
> >> + for_each_available_child_of_node(np, sensor_np) {
> >
> > Aren't we leaking np here? I think we need of_node_put() after
> > of_get_next_child() to make sure the reference to the "thermal-zones"
> > node is properly released.
>
> No, we will not leak np here. Because the
> for_each_available_child_of_node will call
> of_get_next_available_child(), which will call of_node_put(prev) to
> decrease refcount of the prev node. So we just need to of_node_put the
> last node after break from this for block.
Okay, looks like I misinterpreted what you were doing there. I thought
that for_each_available_child_of_node() took the child as first argument
and the parent as second and therefore np would be overwritten by the
first assignment in the macro.
But looking at this more closely I think there's something else wrong
here. for_each_available_child_of_node() is defined as:
for_each_available_child_of_node(parent, child)
so in the above, np will be the parent and sensor_np the child. Why do
you have to do
sensor_np = of_get_next_child(np, NULL);
? That's already done as part of the loop in the macro, right? So does
that not mean we get two references and we leak the first one? Can the
above not simply been dropped?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register
2018-11-29 17:13 ` Thierry Reding
@ 2018-11-30 3:10 ` Wei Ni
0 siblings, 0 replies; 19+ messages in thread
From: Wei Ni @ 2018-11-30 3:10 UTC (permalink / raw)
To: Thierry Reding
Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, srikars,
linux-kernel
On 30/11/2018 1:13 AM, Thierry Reding wrote:
> On Thu, Nov 29, 2018 at 01:55:02PM +0800, Wei Ni wrote:
>> On 28/11/2018 6:25 PM, Thierry Reding wrote:
>>> On Wed, Nov 28, 2018 at 01:44:37PM +0800, Wei Ni wrote:
> [...]
>>>> + bool ret = false;
>>>> + struct of_phandle_args sensor_specs;
>>>> + struct device_node *np, *sensor_np;
>>>> +
>>>> + np = of_find_node_by_name(NULL, "thermal-zones");
>>>> + if (!np)
>>>> + return ret;
>>>> +
>>>> + sensor_np = of_get_next_child(np, NULL);
>>>> + for_each_available_child_of_node(np, sensor_np) {
>>>
>>> Aren't we leaking np here? I think we need of_node_put() after
>>> of_get_next_child() to make sure the reference to the "thermal-zones"
>>> node is properly released.
>>
>> No, we will not leak np here. Because the
>> for_each_available_child_of_node will call
>> of_get_next_available_child(), which will call of_node_put(prev) to
>> decrease refcount of the prev node. So we just need to of_node_put the
>> last node after break from this for block.
>
> Okay, looks like I misinterpreted what you were doing there. I thought
> that for_each_available_child_of_node() took the child as first argument
> and the parent as second and therefore np would be overwritten by the
> first assignment in the macro.
>
> But looking at this more closely I think there's something else wrong
> here. for_each_available_child_of_node() is defined as:
>
> for_each_available_child_of_node(parent, child)
>
> so in the above, np will be the parent and sensor_np the child. Why do
> you have to do
>
> sensor_np = of_get_next_child(np, NULL);
>
> ? That's already done as part of the loop in the macro, right? So does
> that not mean we get two references and we leak the first one? Can the
> above not simply been dropped?
It's so sorry, it's my mistake, we should remove this line, it was my
develop code, forgot to remove it.
Will fix it in next version.
Thanks.
>
> Thierry
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register
@ 2018-11-30 3:10 ` Wei Ni
0 siblings, 0 replies; 19+ messages in thread
From: Wei Ni @ 2018-11-30 3:10 UTC (permalink / raw)
To: Thierry Reding
Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, srikars,
linux-kernel
On 30/11/2018 1:13 AM, Thierry Reding wrote:
> On Thu, Nov 29, 2018 at 01:55:02PM +0800, Wei Ni wrote:
>> On 28/11/2018 6:25 PM, Thierry Reding wrote:
>>> On Wed, Nov 28, 2018 at 01:44:37PM +0800, Wei Ni wrote:
> [...]
>>>> + bool ret = false;
>>>> + struct of_phandle_args sensor_specs;
>>>> + struct device_node *np, *sensor_np;
>>>> +
>>>> + np = of_find_node_by_name(NULL, "thermal-zones");
>>>> + if (!np)
>>>> + return ret;
>>>> +
>>>> + sensor_np = of_get_next_child(np, NULL);
>>>> + for_each_available_child_of_node(np, sensor_np) {
>>>
>>> Aren't we leaking np here? I think we need of_node_put() after
>>> of_get_next_child() to make sure the reference to the "thermal-zones"
>>> node is properly released.
>>
>> No, we will not leak np here. Because the
>> for_each_available_child_of_node will call
>> of_get_next_available_child(), which will call of_node_put(prev) to
>> decrease refcount of the prev node. So we just need to of_node_put the
>> last node after break from this for block.
>
> Okay, looks like I misinterpreted what you were doing there. I thought
> that for_each_available_child_of_node() took the child as first argument
> and the parent as second and therefore np would be overwritten by the
> first assignment in the macro.
>
> But looking at this more closely I think there's something else wrong
> here. for_each_available_child_of_node() is defined as:
>
> for_each_available_child_of_node(parent, child)
>
> so in the above, np will be the parent and sensor_np the child. Why do
> you have to do
>
> sensor_np = of_get_next_child(np, NULL);
>
> ? That's already done as part of the loop in the macro, right? So does
> that not mean we get two references and we leak the first one? Can the
> above not simply been dropped?
It's so sorry, it's my mistake, we should remove this line, it was my
develop code, forgot to remove it.
Will fix it in next version.
Thanks.
>
> Thierry
>
^ permalink raw reply [flat|nested] 19+ messages in thread