Re: [RESEND PATCH V1 8/9] hwmon: lm75: expose to thermal fw via DTnodes

[Eduardo Valentin]

Hi Wei,


On 18-07-2013 01:33, Wei Ni wrote:
> On 07/17/2013 11:17 PM, Eduardo Valentin wrote:
>> This patch adds to lm75 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 lm75 DT node. Otherwise, the driver behavior
>> will be the same.
>>
>> Cc: Jean Delvare <khali@xxxxxxxxxxxx>
>> Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
>> Cc: lm-sensors@xxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Eduardo Valentin <eduardo.valentin@xxxxxx>
>> ---
>>  drivers/hwmon/lm75.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
>> index c03b490..0aa5e28 100644
>> --- a/drivers/hwmon/lm75.c
>> +++ b/drivers/hwmon/lm75.c
>> @@ -27,6 +27,8 @@
>>  #include <linux/hwmon-sysfs.h>
>>  #include <linux/err.h>
>>  #include <linux/mutex.h>
>> +#include <linux/thermal.h>
>> +#include <linux/of.h>
>>  #include "lm75.h"
>>  
>>  
>> @@ -70,6 +72,7 @@ static const u8 LM75_REG_TEMP[3] = {
>>  /* Each client has this additional data */
>>  struct lm75_data {
>>  	struct device		*hwmon_dev;
>> +	struct thermal_zone_device	*tz;
>>  	struct mutex		update_lock;
>>  	u8			orig_conf;
>>  	u8			resolution;	/* In bits, between 9 and 12 */
>> @@ -92,6 +95,19 @@ static struct lm75_data *lm75_update_device(struct device *dev);
>>  
>>  /* sysfs attributes for hwmon */
>>  
>> +static int lm75_read_temp(void *dev, unsigned long *temp)
>> +{
>> +	struct lm75_data *data = lm75_update_device(dev);
>> +
>> +	if (IS_ERR(data))
>> +		return PTR_ERR(data);
>> +
>> +	*temp = ((data->temp[0] >> (16 - data->resolution)) * 1000) >>
>> +			(data->resolution - 8);
>> +
>> +	return 0;
>> +}
>> +
>>  static ssize_t show_temp(struct device *dev, struct device_attribute *da,
>>  			 char *buf)
>>  {
>> @@ -271,11 +287,23 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>  		goto exit_remove;
>>  	}
>>  
>> +	if (of_find_node_by_name(client->dev.of_node, "thermal_zone")) {
>> +		data->tz = thermal_zone_of_device_register(&client->dev,
>> +							   &client->dev,
>> +							   lm75_read_temp);
> 
> Hi, Eduardo
> I have two questions:
> 1. As we know, after register to the thermal framework, it will have
> duplicate hwmon devices. I think lm-sensor maintainer would not like this.

Yes I noticed. You have always the option to disable the
CONFIG_THERMAL_HWMON in your build.

> How about to add a flag to indicate it, which I talk about with
> Durgadoss in your [RFC 1/4]patch.

This would be very much appreciated, but I don't think this is a blocker
for this series. We can of course include this patch on it. Having a
flag to control this thermal fw to hwmon interface is actually a good idea.

> 
> 2. I'm also trying to use your codes on lm90. The LM90 serial has more
> then one sensors in one chip, local sensor, remote sensor and may have
> remote2 sensor, so it mean there may have more than one thermal_zone
> under the lm90 device node, will you consider it?
> 

I haven't looked lm90 source code. How do you map it? Do you probe each
sensor or do you probe 1 device and expose 3 sensors?

In first case, you can still reuse what is in this series.

Later case, we need to change it slightly.

That is the case which is pretty similar on OMAP5 for instance, where
the device has three sensors. This patch series does not cover for this
case. But it can be simply modified to get around it.

We would need to allow more than one 'thermal_zone' nodes inside a
single device. But then we would need to have a way to determine which
sensor goes to which zone too.


> Thanks.
> Wei.
> 
>> +		if (IS_ERR(data->tz)) {
>> +			status = PTR_ERR(data->tz);
>> +			goto exit_hwmon;
>> +		}
>> +	}
>> +
>>  	dev_info(&client->dev, "%s: sensor '%s'\n",
>>  		 dev_name(data->hwmon_dev), client->name);
>>  
>>  	return 0;
>>  
>> +exit_hwmon:
>> +	hwmon_device_unregister(data->hwmon_dev);
>>  exit_remove:
>>  	sysfs_remove_group(&client->dev.kobj, &lm75_group);
>>  	return status;
>> @@ -285,6 +313,7 @@ static int lm75_remove(struct i2c_client *client)
>>  {
>>  	struct lm75_data *data = i2c_get_clientdata(client);
>>  
>> +	thermal_zone_device_unregister(data->tz);
>>  	hwmon_device_unregister(data->hwmon_dev);
>>  	sysfs_remove_group(&client->dev.kobj, &lm75_group);
>>  	lm75_write_value(client, LM75_REG_CONF, data->orig_conf);
>>
> 
> 
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin

Attachment: signature.asc
Description: OpenPGP digital signature