On 10/28/22 09:15, Cristian Marussi wrote:

On Fri, Oct 28, 2022 at 08:58:58AM -0700, Guenter Roeck wrote:

quoted

On 10/28/22 08:35, Cristian Marussi wrote:
[ ... ]

quoted

quoted

quoted

+	/*
+	 * Try to register a temperature sensor with the Thermal Framework:
+	 * skip sensors not defined as part of any thermal zone (-ENODEV) but
+	 * report any other errors related to misconfigured zones/sensors.
+	 */
+	tzd = devm_thermal_of_zone_register(dev, th_sensor->info->id, th_sensor,
+					    &scmi_hwmon_thermal_ops);
+	if (IS_ERR(tzd)) {
+		devm_kfree(dev, th_sensor);
+
+		if (PTR_ERR(tzd) != -ENODEV)
+			return PTR_ERR(tzd);
+
+		dev_info(dev, "Sensor '%s' not attached to any thermal zone.\n",
+			 sensor->name);

There were complaints about this message as it is noisy. If you send
another version, please drop it unless attaching each sensor to a thermal
zone is strongly expected. If you don't send another version, I'll drop it
while applying.

Ok fine for me. I am waiting to have some feedback from Sudeep too, but
I do not have plan for another version as of now.

As a side note, though, I understand the 'noisiness' argument, but,
sincerely this same message in the original HWMON code was the only
reason why I spotted that something was wrong with the SCMI/HWMON
interactions and discovered the indexes/ids mismatch...if not for
that it would have gone un-noticed that a perfectly configured
ThermalZone/Sensor was not working properly...
(un-noticed at least until something would have been burnt to fire
   in my house .. joking :P)

Good point.

Did you ever check the returned error code ? Maybe we could use it to
distinguish "it is not attached to a thermal zone because it is not
associated with one" from "attaching to a thermal zone failed because
its configuration is bad/incomplete".

Yes, it is what I do already indeed, in this regards I mimicked what
the hwmon-thermal bridge was doing.

In scmi_thermal_sensor_register() this message is printed out only
if Thermal registration returned -ENODEV and no err is reported
(which means teh specified sensor was not found attached to any TZ),
while in the caller of scmi_thermal_sensor_register() for any error
returned but -ENOMEM I print:

	"Thermal zone misconfigured for %s. err=%d\n",

since any error reported by Thermal other than ENODEV and ENOMEM
means the DT parsing unveiled some configuration anomaly.

Ok, then let's hope that this finds misconfigurations and drop the
info message.

I just noticed another problem in your code:

+		if (ret == -ENOMEM)
+			return ret;
+		else if (ret)
+			dev_warn(dev,
+				 "Thermal zone misconfigured for %s. err=%d\n",
+				 sensor->name, ret);

Static analyzers will rightfully notice that else after return is unnecessary.
Please rewrite and drop the else. I think something like

		if (ret) {
			if (ret == -ENOMEM)
				return ret;
			dev_warn(dev,
				 "Thermal zone misconfigured for %s. err=%d\n",
				 sensor->name, ret);
		}

would be better since ret would only be evaluated once in the no-error case.

Thanks,
Guenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel