Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register

[Wei Ni]

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

>