Re: [PATCH 06/16] hwmon: tmp102: expose to thermal fw via DT nodes

On Wed, Sep 18, 2013 at 11:54:18AM -0400, Eduardo Valentin wrote:

On 18-09-2013 11:17, Guenter Roeck wrote:

quoted

On Wed, Sep 18, 2013 at 10:29:09AM -0400, Eduardo Valentin wrote:

quoted

On 18-09-2013 07:18, Guenter Roeck wrote:

quoted

On Tue, Sep 17, 2013 at 06:29:45PM -0400, Eduardo Valentin wrote:

quoted

On 15-09-2013 19:33, Guenter Roeck wrote:

quoted

On 09/15/2013 03:02 PM, Eduardo Valentin wrote:

quoted

This patch adds to tmp102 temperature sensor the possibility to
expose itself as thermal zone device, registered on the thermal
framework.

The thermal zone is built only if a device tree node describing
a thermal zone for this sensor is present inside the tmp102 DT
node. Otherwise, the driver behavior will be the same.

Cc: Jean Delvare <redacted> Cc: Guenter Roeck
[off-list ref] Cc: lm-sensors-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org Cc:
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Signed-off-by: Eduardo Valentin
[off-list ref] --- drivers/hwmon/tmp102.c | 28
++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)

diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c 
index d7b47ab..e432444 100644 --- a/drivers/hwmon/tmp102.c +++

b/drivers/hwmon/tmp102.c @@ -27,6 +27,8 @@ #include
<linux/mutex.h> #include <linux/device.h> #include
<linux/jiffies.h> +#include <linux/thermal.h> +#include
<linux/of.h>

#define    DRIVER_NAME "tmp102"

@@ -50,6 +52,7 @@

struct tmp102 { struct device *hwmon_dev; +    struct
thermal_zone_device *tz; struct mutex lock; u16 config_orig; 
unsigned long last_update; @@ -93,6 +96,19 @@ static struct
tmp102 *tmp102_update_device(struct i2c_client *client) return
tmp102; }

+static int tmp102_read_temp(void *dev, long *temp) +{ +
struct tmp102 *tmp102 =
tmp102_update_device(to_i2c_client(dev)); + +    if
(tmp102->temp[0] < 0) +        dev_warn(tmp102->hwmon_dev, +
"operating in negative temp: %d\n", tmp102->temp[0]); +

Please drop this warning.

Done for both drivers.

quoted

Guenter

quoted

+    *temp = tmp102->temp[0]; + +    return 0; +} + static
ssize_t tmp102_show_temp(struct device *dev, struct
device_attribute *attr, char *buf) @@ -204,6 +220,16 @@ static
int tmp102_probe(struct i2c_client *client, goto
fail_remove_sysfs; }

+    tmp102->tz = thermal_zone_of_sensor_register(&client->dev,
0, +                             &client->dev, +
tmp102_read_temp, NULL); +    if (IS_ERR(tmp102->tz)) { +
dev_warn(&client->dev, +             "Could not parse thermal
data in device tree: %ld\n", +
PTR_ERR(tmp102->tz));

Please drop this warning. You already create error messages in 
thermal_zone_of_sensor_register(). That should be sufficient. The
same applies to the lm75 patch.

OK. Done for both.

quoted

As a side note, I would suggest to provide devm_ functions for 
registration. We are introducing those for hwmon registration,
which enables us to remove most _remove functions. It would be
great if we can keep it that way.

Right. This side note is taken. Actually this is on my todo list
for quite a while. But I believe this should not block this series,
should it? I will be probably cleaning the thermal framework code
after this current work is accepted at least.

quoted

On a higher level, I don't think it is a good idea to make
thermal zones and thermal zone data mandatory. Many systems may
neither need nor want it.

Well, I agree with you. Did you see something hard required in the
patch I sent. I made it so that it could continue the driver probe
without thermal zones, as you requested.

If it is not mandatory you should not dump an error message to the
console in the thermal registration function. Since you do, you at
least consider it mandatory if that function is called.

So please either drop the error message from the registration
function or add a check into the drivers to only register into the
thermal subsystem if there is a respective thermal entry for that
sensor in the devicetree data.

There are systes out there with literally dozens of temperature
sensors. In many cases, those are purely for system health
monitoring, not for thermal management. I don't want to end up in a
situation where users complain about dozens of error messages on the
console and no way to avoid it but providing dummy thermal subsystem
data.

Now I see.


Then I will rollback to the previous version in which lm sensors were
first probing for thermal properties within their dt node. Something like:

diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
index dc96598..cb1c663 100644
--- a/drivers/hwmon/tmp102.c
+++ b/drivers/hwmon/tmp102.c

@@ -216,11 +216,13 @@ static int tmp102_probe(struct i2c_client *client,
                goto fail_remove_sysfs;
        }

-       tmp102->tz = thermal_zone_of_sensor_register(&client->dev, 0,
-                                                    &client->dev,
-                                                    tmp102_read_temp,

NULL);
-       if (IS_ERR(tmp102->tz))
-               tmp102->tz = NULL;
+       if ((of_find_property(client->dev.of_node, "#sensor-cells", NULL)) {
+               tmp102->tz =
thermal_zone_of_sensor_register(&client->dev, 0,
+                                                            &client->dev,
+
tmp102_read_temp, NULL);
+               if (IS_ERR(tmp102->tz))
+                       tmp102->tz = NULL;
+       }

        dev_info(&client->dev, "initialized\n");


Does it sound reasonable?

Personally I would prefer if the registration code fails silently.
Pushing the above code into each driver is just adding the same code
repeatedly all over the place.

Fair enough. It becomes tedious and just duplicating code. I agree.

So I will keep the v2 I just sent and remove the annoying error messages
from of-thermal.c while registering the sensors.

quoted

Also, each sensor instance will still result in an error if there
is no global "thermal-zones" entry. Checking for that global entry
in each driver would be even more excessive, and I just don't like
that noisyness.

Also, I think you'll need to create devicetree bindings documents
for the two sensors.

Why would I? There is only one extra property and that is already
documented. I think the sensor still falls into the dummy dt node.

I'll leave that up to the devicetree folks to decide.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html