[PATCH v2 0/3] Lm90 Enhancements

[PATCH v2 0/3] Lm90 Enhancements

[Wei Ni]

This patch set enhance the lm90 driver,
it make the driver more readable and easier to use thermal framework.

This series is v2, previous version patches:
[RFC]: http://thread.gmane.org/gmane.linux.power-management.general/31056
[v1]: http://thread.gmane.org/gmane.linux.ports.tegra/11710/

Changes from v1:
1. change the string "irq" to "IRQ"
2. add macro defines for the alarm status
3. consider the shared IRQ.

Changes from RFC:
1. change _show_temp() to read_temp(), _set_temp() to write_temp().
2. simply return value for the read_temp(), not use pointer.
3. use devm_request_threaded_irq() to request irq and set flag IRQF_ONESHOT.

Wei Ni (3):
  hwmon: (lm90) split set&show temp as common codes
  hwmon: (lm90) add support to handle IRQ.
  hwmon: (lm90) use enums for the indexes of temp8 and temp11

 drivers/hwmon/lm90.c |  374 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 253 insertions(+), 121 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/3] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

Split set&show temp codes as common functions, so we can use it directly when
implement linux thermal framework.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 drivers/hwmon/lm90.c |  118 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 75 insertions(+), 43 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 8eeb141..2cb7f8e 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -702,29 +702,37 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val)
  * Sysfs stuff
  */
 
-static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
-			  char *buf)
+static int read_temp8(struct device *dev, int index)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
+	struct lm90_data *data = lm90_update_device(dev);
 
 	if (data->kind == adt7461)
-		temp = temp_from_u8_adt7461(data, data->temp8[attr->index]);
+		temp = temp_from_u8_adt7461(data, data->temp8[index]);
 	else if (data->kind == max6646)
-		temp = temp_from_u8(data->temp8[attr->index]);
+		temp = temp_from_u8(data->temp8[index]);
 	else
-		temp = temp_from_s8(data->temp8[attr->index]);
+		temp = temp_from_s8(data->temp8[index]);
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind == lm99 && attr->index == 3)
+	if (data->kind == lm99 && index == 3)
 		temp += 16000;
 
+	return temp;
+}
+
+static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
+			  char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	int temp;
+
+	temp = read_temp8(dev, attr->index);
+
 	return sprintf(buf, "%d\n", temp);
 }
 
-static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
-			 const char *buf, size_t count)
+static void write_temp8(struct device *dev, int index, long val)
 {
 	static const u8 reg[8] = {
 		LM90_REG_W_LOCAL_LOW,
@@ -737,60 +745,76 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
 		MAX6659_REG_W_REMOTE_EMERG,
 	};
 
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm90_data *data = i2c_get_clientdata(client);
-	int nr = attr->index;
-	long val;
-	int err;
-
-	err = kstrtol(buf, 10, &val);
-	if (err < 0)
-		return err;
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind == lm99 && attr->index == 3)
+	if (data->kind == lm99 && index == 3)
 		val -= 16000;
 
 	mutex_lock(&data->update_lock);
 	if (data->kind == adt7461)
-		data->temp8[nr] = temp_to_u8_adt7461(data, val);
+		data->temp8[index] = temp_to_u8_adt7461(data, val);
 	else if (data->kind == max6646)
-		data->temp8[nr] = temp_to_u8(val);
+		data->temp8[index] = temp_to_u8(val);
 	else
-		data->temp8[nr] = temp_to_s8(val);
+		data->temp8[index] = temp_to_s8(val);
 
-	lm90_select_remote_channel(client, data, nr >= 6);
-	i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
+	lm90_select_remote_channel(client, data, index >= 6);
+	i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]);
 	lm90_select_remote_channel(client, data, 0);
 
 	mutex_unlock(&data->update_lock);
+}
+
+static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
+			 const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	int index = attr->index;
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	write_temp8(dev, index, val);
+
 	return count;
 }
 
-static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
-			   char *buf)
+static int read_temp11(struct device *dev, int index)
 {
-	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
-	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
+	struct lm90_data *data = lm90_update_device(dev);
 
 	if (data->kind == adt7461)
-		temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
+		temp = temp_from_u16_adt7461(data, data->temp11[index]);
 	else if (data->kind == max6646)
-		temp = temp_from_u16(data->temp11[attr->index]);
+		temp = temp_from_u16(data->temp11[index]);
 	else
-		temp = temp_from_s16(data->temp11[attr->index]);
+		temp = temp_from_s16(data->temp11[index]);
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind == lm99 &&  attr->index <= 2)
+	if (data->kind == lm99 &&  index <= 2)
 		temp += 16000;
 
+	return temp;
+}
+
+static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
+			   char *buf)
+{
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+	int temp;
+
+	temp = read_temp11(dev, attr->index);
+
 	return sprintf(buf, "%d\n", temp);
 }
 
-static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
-			  const char *buf, size_t count)
+static void write_temp11(struct device *dev, int nr, int index, long val)
 {
 	struct {
 		u8 high;
@@ -804,17 +828,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
 	};
 
-	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm90_data *data = i2c_get_clientdata(client);
-	int nr = attr->nr;
-	int index = attr->index;
-	long val;
-	int err;
-
-	err = kstrtol(buf, 10, &val);
-	if (err < 0)
-		return err;
 
 	/* +16 degrees offset for temp2 for the LM99 */
 	if (data->kind == lm99 && index <= 2)
@@ -839,6 +854,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 	lm90_select_remote_channel(client, data, 0);
 
 	mutex_unlock(&data->update_lock);
+}
+
+static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
+			  const char *buf, size_t count)
+{
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+	int nr = attr->nr;
+	int index = attr->index;
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	write_temp11(dev, nr, index, val);
+
 	return count;
 }
 
-- 
1.7.9.5

Re: [PATCH v2 1/3] hwmon: (lm90) split set&show temp as common codes

[Guenter Roeck]

On Wed, Jul 10, 2013 at 07:25:37PM +0800, Wei Ni wrote:
> Split set&show temp codes as common functions, so we can use it directly when
> implement linux thermal framework.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  drivers/hwmon/lm90.c |  118 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 75 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 8eeb141..2cb7f8e 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -702,29 +702,37 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val)
>   * Sysfs stuff
>   */
>  
> -static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
> -			  char *buf)
> +static int read_temp8(struct device *dev, int index)
>  {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct lm90_data *data = lm90_update_device(dev);
>  	int temp;
> +	struct lm90_data *data = lm90_update_device(dev);
>  
Why this reordering of variables ?

>  	if (data->kind == adt7461)
> -		temp = temp_from_u8_adt7461(data, data->temp8[attr->index]);
> +		temp = temp_from_u8_adt7461(data, data->temp8[index]);
>  	else if (data->kind == max6646)
> -		temp = temp_from_u8(data->temp8[attr->index]);
> +		temp = temp_from_u8(data->temp8[index]);
>  	else
> -		temp = temp_from_s8(data->temp8[attr->index]);
> +		temp = temp_from_s8(data->temp8[index]);
>  
>  	/* +16 degrees offset for temp2 for the LM99 */
> -	if (data->kind == lm99 && attr->index == 3)
> +	if (data->kind == lm99 && index == 3)
>  		temp += 16000;
>  
> +	return temp;
> +}
> +
> +static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
> +			  char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	int temp;
> +
> +	temp = read_temp8(dev, attr->index);
> +
>  	return sprintf(buf, "%d\n", temp);

Might as well simplify with
	return sprintf(buf, "%d\n", read_temp8(dev, attr->index));

>  }
>  
> -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> -			 const char *buf, size_t count)
> +static void write_temp8(struct device *dev, int index, long val)
>  {
>  	static const u8 reg[8] = {
>  		LM90_REG_W_LOCAL_LOW,
> @@ -737,60 +745,76 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>  		MAX6659_REG_W_REMOTE_EMERG,
>  	};
>  
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct lm90_data *data = i2c_get_clientdata(client);
> -	int nr = attr->index;
> -	long val;
> -	int err;
> -
> -	err = kstrtol(buf, 10, &val);
> -	if (err < 0)
> -		return err;
>  
>  	/* +16 degrees offset for temp2 for the LM99 */
> -	if (data->kind == lm99 && attr->index == 3)
> +	if (data->kind == lm99 && index == 3)
>  		val -= 16000;
>  
>  	mutex_lock(&data->update_lock);
>  	if (data->kind == adt7461)
> -		data->temp8[nr] = temp_to_u8_adt7461(data, val);
> +		data->temp8[index] = temp_to_u8_adt7461(data, val);
>  	else if (data->kind == max6646)
> -		data->temp8[nr] = temp_to_u8(val);
> +		data->temp8[index] = temp_to_u8(val);
>  	else
> -		data->temp8[nr] = temp_to_s8(val);
> +		data->temp8[index] = temp_to_s8(val);
>  
> -	lm90_select_remote_channel(client, data, nr >= 6);
> -	i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
> +	lm90_select_remote_channel(client, data, index >= 6);
> +	i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]);
>  	lm90_select_remote_channel(client, data, 0);
>  
>  	mutex_unlock(&data->update_lock);
> +}
> +
> +static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> +			 const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	int index = attr->index;
> +	long val;
> +	int err;
> +
> +	err = kstrtol(buf, 10, &val);
> +	if (err < 0)
> +		return err;
> +
> +	write_temp8(dev, index, val);
> +
>  	return count;
>  }
>  
> -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
> -			   char *buf)
> +static int read_temp11(struct device *dev, int index)
>  {
> -	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> -	struct lm90_data *data = lm90_update_device(dev);
>  	int temp;
> +	struct lm90_data *data = lm90_update_device(dev);
>  
Another unnecessary variable reorder.

>  	if (data->kind == adt7461)
> -		temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
> +		temp = temp_from_u16_adt7461(data, data->temp11[index]);
>  	else if (data->kind == max6646)
> -		temp = temp_from_u16(data->temp11[attr->index]);
> +		temp = temp_from_u16(data->temp11[index]);
>  	else
> -		temp = temp_from_s16(data->temp11[attr->index]);
> +		temp = temp_from_s16(data->temp11[index]);
>  
>  	/* +16 degrees offset for temp2 for the LM99 */
> -	if (data->kind == lm99 &&  attr->index <= 2)
> +	if (data->kind == lm99 &&  index <= 2)
>  		temp += 16000;
>  
> +	return temp;
> +}
> +
> +static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
> +			   char *buf)
> +{
> +	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> +	int temp;
> +
> +	temp = read_temp11(dev, attr->index);
> +
>  	return sprintf(buf, "%d\n", temp);

Same simplification as above.

>  }
>  
> -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> -			  const char *buf, size_t count)
> +static void write_temp11(struct device *dev, int nr, int index, long val)
>  {
>  	struct {
>  		u8 high;
> @@ -804,17 +828,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>  		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
>  	};
>  
> -	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct lm90_data *data = i2c_get_clientdata(client);
> -	int nr = attr->nr;
> -	int index = attr->index;
> -	long val;
> -	int err;
> -
> -	err = kstrtol(buf, 10, &val);
> -	if (err < 0)
> -		return err;
>  
>  	/* +16 degrees offset for temp2 for the LM99 */
>  	if (data->kind == lm99 && index <= 2)
> @@ -839,6 +854,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>  	lm90_select_remote_channel(client, data, 0);
>  
>  	mutex_unlock(&data->update_lock);
> +}
> +
> +static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> +			  const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> +	int nr = attr->nr;
> +	int index = attr->index;
> +	long val;
> +	int err;
> +
> +	err = kstrtol(buf, 10, &val);
> +	if (err < 0)
> +		return err;
> +
> +	write_temp11(dev, nr, index, val);
> +
>  	return count;
>  }
>  
> -- 
> 1.7.9.5
> 
> 

Re: [PATCH v2 1/3] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

On 07/11/2013 02:21 AM, Guenter Roeck wrote:
> On Wed, Jul 10, 2013 at 07:25:37PM +0800, Wei Ni wrote:
>> Split set&show temp codes as common functions, so we can use it directly when
>> implement linux thermal framework.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  drivers/hwmon/lm90.c |  118 ++++++++++++++++++++++++++++++++------------------
>>  1 file changed, 75 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index 8eeb141..2cb7f8e 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -702,29 +702,37 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val)
>>   * Sysfs stuff
>>   */
>>
>> -static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
>> -                       char *buf)
>> +static int read_temp8(struct device *dev, int index)
>>  {
>> -     struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> -     struct lm90_data *data = lm90_update_device(dev);
>>       int temp;
>> +     struct lm90_data *data = lm90_update_device(dev);
>>
> Why this reordering of variables ?

Oh, I didn't noticed it yet, thanks for your review, I will change it.

> 
>>       if (data->kind == adt7461)
>> -             temp = temp_from_u8_adt7461(data, data->temp8[attr->index]);
>> +             temp = temp_from_u8_adt7461(data, data->temp8[index]);
>>       else if (data->kind == max6646)
>> -             temp = temp_from_u8(data->temp8[attr->index]);
>> +             temp = temp_from_u8(data->temp8[index]);
>>       else
>> -             temp = temp_from_s8(data->temp8[attr->index]);
>> +             temp = temp_from_s8(data->temp8[index]);
>>
>>       /* +16 degrees offset for temp2 for the LM99 */
>> -     if (data->kind == lm99 && attr->index == 3)
>> +     if (data->kind == lm99 && index == 3)
>>               temp += 16000;
>>
>> +     return temp;
>> +}
>> +
>> +static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
>> +                       char *buf)
>> +{
>> +     struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> +     int temp;
>> +
>> +     temp = read_temp8(dev, attr->index);
>> +
>>       return sprintf(buf, "%d\n", temp);
> 
> Might as well simplify with
>         return sprintf(buf, "%d\n", read_temp8(dev, attr->index));

Ok, I will update it.

> 
>>  }
>>
>> -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>> -                      const char *buf, size_t count)
>> +static void write_temp8(struct device *dev, int index, long val)
>>  {
>>       static const u8 reg[8] = {
>>               LM90_REG_W_LOCAL_LOW,
>> @@ -737,60 +745,76 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>>               MAX6659_REG_W_REMOTE_EMERG,
>>       };
>>
>> -     struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>>       struct i2c_client *client = to_i2c_client(dev);
>>       struct lm90_data *data = i2c_get_clientdata(client);
>> -     int nr = attr->index;
>> -     long val;
>> -     int err;
>> -
>> -     err = kstrtol(buf, 10, &val);
>> -     if (err < 0)
>> -             return err;
>>
>>       /* +16 degrees offset for temp2 for the LM99 */
>> -     if (data->kind == lm99 && attr->index == 3)
>> +     if (data->kind == lm99 && index == 3)
>>               val -= 16000;
>>
>>       mutex_lock(&data->update_lock);
>>       if (data->kind == adt7461)
>> -             data->temp8[nr] = temp_to_u8_adt7461(data, val);
>> +             data->temp8[index] = temp_to_u8_adt7461(data, val);
>>       else if (data->kind == max6646)
>> -             data->temp8[nr] = temp_to_u8(val);
>> +             data->temp8[index] = temp_to_u8(val);
>>       else
>> -             data->temp8[nr] = temp_to_s8(val);
>> +             data->temp8[index] = temp_to_s8(val);
>>
>> -     lm90_select_remote_channel(client, data, nr >= 6);
>> -     i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
>> +     lm90_select_remote_channel(client, data, index >= 6);
>> +     i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]);
>>       lm90_select_remote_channel(client, data, 0);
>>
>>       mutex_unlock(&data->update_lock);
>> +}
>> +
>> +static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>> +                      const char *buf, size_t count)
>> +{
>> +     struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> +     int index = attr->index;
>> +     long val;
>> +     int err;
>> +
>> +     err = kstrtol(buf, 10, &val);
>> +     if (err < 0)
>> +             return err;
>> +
>> +     write_temp8(dev, index, val);
>> +
>>       return count;
>>  }
>>
>> -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
>> -                        char *buf)
>> +static int read_temp11(struct device *dev, int index)
>>  {
>> -     struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>> -     struct lm90_data *data = lm90_update_device(dev);
>>       int temp;
>> +     struct lm90_data *data = lm90_update_device(dev);
>>
> Another unnecessary variable reorder.
> 
>>       if (data->kind == adt7461)
>> -             temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
>> +             temp = temp_from_u16_adt7461(data, data->temp11[index]);
>>       else if (data->kind == max6646)
>> -             temp = temp_from_u16(data->temp11[attr->index]);
>> +             temp = temp_from_u16(data->temp11[index]);
>>       else
>> -             temp = temp_from_s16(data->temp11[attr->index]);
>> +             temp = temp_from_s16(data->temp11[index]);
>>
>>       /* +16 degrees offset for temp2 for the LM99 */
>> -     if (data->kind == lm99 &&  attr->index <= 2)
>> +     if (data->kind == lm99 &&  index <= 2)
>>               temp += 16000;
>>
>> +     return temp;
>> +}
>> +
>> +static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
>> +                        char *buf)
>> +{
>> +     struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>> +     int temp;
>> +
>> +     temp = read_temp11(dev, attr->index);
>> +
>>       return sprintf(buf, "%d\n", temp);
> 
> Same simplification as above.
> 
>>  }
>>
>> -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>> -                       const char *buf, size_t count)
>> +static void write_temp11(struct device *dev, int nr, int index, long val)
>>  {
>>       struct {
>>               u8 high;
>> @@ -804,17 +828,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>>               { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
>>       };
>>
>> -     struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>>       struct i2c_client *client = to_i2c_client(dev);
>>       struct lm90_data *data = i2c_get_clientdata(client);
>> -     int nr = attr->nr;
>> -     int index = attr->index;
>> -     long val;
>> -     int err;
>> -
>> -     err = kstrtol(buf, 10, &val);
>> -     if (err < 0)
>> -             return err;
>>
>>       /* +16 degrees offset for temp2 for the LM99 */
>>       if (data->kind == lm99 && index <= 2)
>> @@ -839,6 +854,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>>       lm90_select_remote_channel(client, data, 0);
>>
>>       mutex_unlock(&data->update_lock);
>> +}
>> +
>> +static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>> +                       const char *buf, size_t count)
>> +{
>> +     struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>> +     int nr = attr->nr;
>> +     int index = attr->index;
>> +     long val;
>> +     int err;
>> +
>> +     err = kstrtol(buf, 10, &val);
>> +     if (err < 0)
>> +             return err;
>> +
>> +     write_temp11(dev, nr, index, val);
>> +
>>       return count;
>>  }
>>
>> --
>> 1.7.9.5
>>
>>

[PATCH v2 2/3] hwmon: (lm90) add support to handle IRQ.

[Wei Ni]

When the temperature exceed the limit range value,
the driver can handle the interrupt.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 drivers/hwmon/lm90.c |   77 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 2cb7f8e..88ff362 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -89,6 +89,7 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/sysfs.h>
+#include <linux/interrupt.h>
 
 /*
  * Addresses to scan
@@ -179,6 +180,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
 #define LM90_HAVE_TEMP3		(1 << 6) /* 3rd temperature sensor	*/
 #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
 
+/* LM90 status */
+#define LM90_LTHRM	(1 << 0)	/* local THERM limit tripped */
+#define LM90_RTHRM	(1 << 1)	/* remote THERM limit tripped */
+#define LM90_OPEN	(1 << 2)	/* remote is an open circuit */
+#define LM90_RLOW	(1 << 3)	/* remote low temp limit tripped */
+#define LM90_RHIGH	(1 << 4)	/* remote high temp limit tripped */
+#define LM90_LLOW	(1 << 5)	/* local low temp limit tripped */
+#define LM90_LHIGH	(1 << 6)	/* local high temp limit tripped */
+#define LM90_BUSY	(1 << 7)	/* ADC is converting */
+
+#define MAX6696_RLOW	(1 << 3)	/* remote2 low temp limit tripped */
+#define MAX6696_RHIGH	(1 << 4)	/* remote2 high temp limit tripped */
+
 /*
  * Driver data (common to all clients)
  */
@@ -1423,6 +1437,43 @@ static void lm90_init_client(struct i2c_client *client)
 		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
 }
 
+static void lm90_alarm_status(struct i2c_client *client,
+			      u8 alarms, u8 alarms_max6696)
+{
+	if (alarms & (LM90_LLOW | LM90_LHIGH | LM90_LTHRM))
+		dev_warn(&client->dev,
+			 "temp%d out of range, please check!\n", 1);
+	if (alarms & (LM90_RLOW | LM90_RHIGH | LM90_RTHRM))
+		dev_warn(&client->dev,
+			 "temp%d out of range, please check!\n", 2);
+	if (alarms & LM90_OPEN)
+		dev_warn(&client->dev,
+			 "temp%d diode open, please check!\n", 2);
+
+	if (alarms_max6696 & (MAX6696_RLOW | MAX6696_RHIGH))
+		dev_warn(&client->dev,
+			 "temp%d out of range, please check!\n", 3);
+}
+
+static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
+{
+	struct lm90_data *data = dev_id;
+	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
+	u8 alarms, alarms_max6696 = 0;
+
+	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
+
+	if (data->kind == max6696)
+		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms_max6696);
+
+	if ((alarms & 0x7f) == 0 && (alarms_max6696 & 0xfe) == 0) {
+		return IRQ_NONE;
+	} else {
+		lm90_alarm_status(client, alarms, alarms_max6696);
+		return IRQ_HANDLED;
+	}
+}
+
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id)
 {
@@ -1499,6 +1550,18 @@ static int lm90_probe(struct i2c_client *client,
 		goto exit_remove_files;
 	}
 
+	if (client->irq >= 0) {
+		dev_dbg(dev, "lm90 IRQ: %d\n", client->irq);
+		err = devm_request_threaded_irq(dev, client->irq,
+						NULL, lm90_irq_thread,
+						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+						"lm90", data);
+		if (err < 0) {
+			dev_err(dev, "cannot request interrupt\n");
+			goto exit_remove_files;
+		}
+	}
+
 	return 0;
 
 exit_remove_files:
@@ -1532,19 +1595,7 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
 	if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {
 		dev_info(&client->dev, "Everything OK\n");
 	} else {
-		if (alarms & 0x61)
-			dev_warn(&client->dev,
-				 "temp%d out of range, please check!\n", 1);
-		if (alarms & 0x1a)
-			dev_warn(&client->dev,
-				 "temp%d out of range, please check!\n", 2);
-		if (alarms & 0x04)
-			dev_warn(&client->dev,
-				 "temp%d diode open, please check!\n", 2);
-
-		if (alarms2 & 0x18)
-			dev_warn(&client->dev,
-				 "temp%d out of range, please check!\n", 3);
+		lm90_alarm_status(client, alarms, alarms2);
 
 		/*
 		 * Disable ALERT# output, because these chips don't implement
-- 
1.7.9.5

Re: [PATCH v2 2/3] hwmon: (lm90) add support to handle IRQ.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4236 bytes --]

On Wed, Jul 10, 2013 at 07:25:38PM +0800, Wei Ni wrote:
> When the temperature exceed the limit range value,
> the driver can handle the interrupt.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  drivers/hwmon/lm90.c |   77 +++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 64 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 2cb7f8e..88ff362 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -89,6 +89,7 @@
>  #include <linux/err.h>
>  #include <linux/mutex.h>
>  #include <linux/sysfs.h>
> +#include <linux/interrupt.h>
>  
>  /*
>   * Addresses to scan
> @@ -179,6 +180,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
>  #define LM90_HAVE_TEMP3		(1 << 6) /* 3rd temperature sensor	*/
>  #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
>  
> +/* LM90 status */
> +#define LM90_LTHRM	(1 << 0)	/* local THERM limit tripped */
> +#define LM90_RTHRM	(1 << 1)	/* remote THERM limit tripped */
> +#define LM90_OPEN	(1 << 2)	/* remote is an open circuit */
> +#define LM90_RLOW	(1 << 3)	/* remote low temp limit tripped */
> +#define LM90_RHIGH	(1 << 4)	/* remote high temp limit tripped */
> +#define LM90_LLOW	(1 << 5)	/* local low temp limit tripped */
> +#define LM90_LHIGH	(1 << 6)	/* local high temp limit tripped */
> +#define LM90_BUSY	(1 << 7)	/* ADC is converting */
> +
> +#define MAX6696_RLOW	(1 << 3)	/* remote2 low temp limit tripped */
> +#define MAX6696_RHIGH	(1 << 4)	/* remote2 high temp limit tripped */

I think this is a nice cleanup, but I'll leave it up to Guenter or Jean
to decide if they want to have this. One problem with the above is that
it's not immediately clear which register contains these bits. That's
often solved by using the register name as prefix but that will in turn
make the names for these bits rather long:

	#define LM90_REG_R_STATUS_LTHRM	(1 << 0)
	...

Perhaps something like

	#define LM90_STATUS_LTHRM	(1 << 0)

would be a good compromise?

Also if Guenter and Jean agree that this is a nice cleanup, it should
probably go into a separate patch since it isn't directly related to the
IRQ support.

>  /*
>   * Driver data (common to all clients)
>   */
> @@ -1423,6 +1437,43 @@ static void lm90_init_client(struct i2c_client *client)
>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>  }
>  
> +static void lm90_alarm_status(struct i2c_client *client,
> +			      u8 alarms, u8 alarms_max6696)
> +{
> +	if (alarms & (LM90_LLOW | LM90_LHIGH | LM90_LTHRM))
> +		dev_warn(&client->dev,
> +			 "temp%d out of range, please check!\n", 1);
> +	if (alarms & (LM90_RLOW | LM90_RHIGH | LM90_RTHRM))
> +		dev_warn(&client->dev,
> +			 "temp%d out of range, please check!\n", 2);
> +	if (alarms & LM90_OPEN)
> +		dev_warn(&client->dev,
> +			 "temp%d diode open, please check!\n", 2);
> +
> +	if (alarms_max6696 & (MAX6696_RLOW | MAX6696_RHIGH))
> +		dev_warn(&client->dev,
> +			 "temp%d out of range, please check!\n", 3);
> +}
> +
> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
> +{
> +	struct lm90_data *data = dev_id;
> +	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
> +	u8 alarms, alarms_max6696 = 0;
> +
> +	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> +
> +	if (data->kind == max6696)
> +		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms_max6696);
> +
> +	if ((alarms & 0x7f) == 0 && (alarms_max6696 & 0xfe) == 0) {
> +		return IRQ_NONE;

For non-MAX6696 chips this will evaluate to:

	if ((alarms & 0x7f) == 0 && (0 & 0xfe) == 0)

and therefore be true for any value of "alarms" and therefore always
result in IRQ_NONE being returned.

One other thing that slightly bugs me about this is that it's a little
tedious to pass alarms_max6696 around like this. Suppose yet another
slightly different variant is supported by this chip in the future,
it's possible it will require another alarms_XYZ variable that has to be
passed around. I don't have a better suggestion though, so maybe it can
remain like this and be rewritten at some point should the need arise.

Thierry

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/3] hwmon: (lm90) add support to handle IRQ.

[Guenter Roeck]

On Wed, Jul 10, 2013 at 10:05:53AM -0700, Thierry Reding wrote:
> On Wed, Jul 10, 2013 at 07:25:38PM +0800, Wei Ni wrote:
> > When the temperature exceed the limit range value,
> > the driver can handle the interrupt.
> > 
> > Signed-off-by: Wei Ni <wni@nvidia.com>
> > ---
> >  drivers/hwmon/lm90.c |   77 +++++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 64 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index 2cb7f8e..88ff362 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -89,6 +89,7 @@
> >  #include <linux/err.h>
> >  #include <linux/mutex.h>
> >  #include <linux/sysfs.h>
> > +#include <linux/interrupt.h>
> >  
> >  /*
> >   * Addresses to scan
> > @@ -179,6 +180,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
> >  #define LM90_HAVE_TEMP3		(1 << 6) /* 3rd temperature sensor	*/
> >  #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
> >  
> > +/* LM90 status */
> > +#define LM90_LTHRM	(1 << 0)	/* local THERM limit tripped */
> > +#define LM90_RTHRM	(1 << 1)	/* remote THERM limit tripped */
> > +#define LM90_OPEN	(1 << 2)	/* remote is an open circuit */
> > +#define LM90_RLOW	(1 << 3)	/* remote low temp limit tripped */
> > +#define LM90_RHIGH	(1 << 4)	/* remote high temp limit tripped */
> > +#define LM90_LLOW	(1 << 5)	/* local low temp limit tripped */
> > +#define LM90_LHIGH	(1 << 6)	/* local high temp limit tripped */
> > +#define LM90_BUSY	(1 << 7)	/* ADC is converting */
> > +
> > +#define MAX6696_RLOW	(1 << 3)	/* remote2 low temp limit tripped */
> > +#define MAX6696_RHIGH	(1 << 4)	/* remote2 high temp limit tripped */
> 
> I think this is a nice cleanup, but I'll leave it up to Guenter or Jean
> to decide if they want to have this. One problem with the above is that
> it's not immediately clear which register contains these bits. That's
> often solved by using the register name as prefix but that will in turn
> make the names for these bits rather long:
> 
> 	#define LM90_REG_R_STATUS_LTHRM	(1 << 0)
> 	...
> 
> Perhaps something like
> 
> 	#define LM90_STATUS_LTHRM	(1 << 0)
> 
> would be a good compromise?
> 
Something like that, yes.

> Also if Guenter and Jean agree that this is a nice cleanup, it should
> probably go into a separate patch since it isn't directly related to the
> IRQ support.
> 
Correct.

> >  /*
> >   * Driver data (common to all clients)
> >   */
> > @@ -1423,6 +1437,43 @@ static void lm90_init_client(struct i2c_client *client)
> >  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> >  }
> >  
> > +static void lm90_alarm_status(struct i2c_client *client,
> > +			      u8 alarms, u8 alarms_max6696)
> > +{
> > +	if (alarms & (LM90_LLOW | LM90_LHIGH | LM90_LTHRM))
> > +		dev_warn(&client->dev,
> > +			 "temp%d out of range, please check!\n", 1);
> > +	if (alarms & (LM90_RLOW | LM90_RHIGH | LM90_RTHRM))
> > +		dev_warn(&client->dev,
> > +			 "temp%d out of range, please check!\n", 2);
> > +	if (alarms & LM90_OPEN)
> > +		dev_warn(&client->dev,
> > +			 "temp%d diode open, please check!\n", 2);
> > +
> > +	if (alarms_max6696 & (MAX6696_RLOW | MAX6696_RHIGH))
> > +		dev_warn(&client->dev,
> > +			 "temp%d out of range, please check!\n", 3);
> > +}
> > +
> > +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
> > +{
> > +	struct lm90_data *data = dev_id;
> > +	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
> > +	u8 alarms, alarms_max6696 = 0;
> > +
> > +	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> > +
> > +	if (data->kind == max6696)
> > +		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms_max6696);
> > +
> > +	if ((alarms & 0x7f) == 0 && (alarms_max6696 & 0xfe) == 0) {
> > +		return IRQ_NONE;
> 
> For non-MAX6696 chips this will evaluate to:
> 
> 	if ((alarms & 0x7f) == 0 && (0 & 0xfe) == 0)
> 
> and therefore be true for any value of "alarms" and therefore always
> result in IRQ_NONE being returned.
> 
Not really. If
	(alarms & 0xfe) == 0
returns false (ie thee is an alarm) the expression is false and the
if statement won't be executed. Or maybe I didn't get enough sleep
last night ;).

> One other thing that slightly bugs me about this is that it's a little
> tedious to pass alarms_max6696 around like this. Suppose yet another
> slightly different variant is supported by this chip in the future,
> it's possible it will require another alarms_XYZ variable that has to be
> passed around. I don't have a better suggestion though, so maybe it can
> remain like this and be rewritten at some point should the need arise.
> 
The driver has tables to separate chips, so masks for status and status2 can
be made generic and configurable if needed. But, yes, it would be better to
select a generic name for the variable from the beginning (status and status2
would do quite nicely).

> Thierry

Re: [PATCH v2 2/3] hwmon: (lm90) add support to handle IRQ.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 895 bytes --]

On Wed, Jul 10, 2013 at 11:12:49AM -0700, Guenter Roeck wrote:
> On Wed, Jul 10, 2013 at 10:05:53AM -0700, Thierry Reding wrote:
> > On Wed, Jul 10, 2013 at 07:25:38PM +0800, Wei Ni wrote:
[...]
> > > +	if ((alarms & 0x7f) == 0 && (alarms_max6696 & 0xfe) == 0) {
> > > +		return IRQ_NONE;
> > 
> > For non-MAX6696 chips this will evaluate to:
> > 
> > 	if ((alarms & 0x7f) == 0 && (0 & 0xfe) == 0)
> > 
> > and therefore be true for any value of "alarms" and therefore always
> > result in IRQ_NONE being returned.
> > 
> Not really. If
> 	(alarms & 0xfe) == 0
> returns false (ie thee is an alarm) the expression is false and the
> if statement won't be executed. Or maybe I didn't get enough sleep
> last night ;).

You're right. Looks like I am the one that didn't get enough sleep last
night. =) So the original code indeed seems to be doing what it should.

Thierry

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/3] hwmon: (lm90) add support to handle IRQ.

[Wei Ni]

On 07/11/2013 02:12 AM, Guenter Roeck wrote:
> On Wed, Jul 10, 2013 at 10:05:53AM -0700, Thierry Reding wrote:
>> On Wed, Jul 10, 2013 at 07:25:38PM +0800, Wei Ni wrote:
>>> When the temperature exceed the limit range value,
>>> the driver can handle the interrupt.
>>>
>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>> ---
>>>  drivers/hwmon/lm90.c |   77 +++++++++++++++++++++++++++++++++++++++++---------
>>>  1 file changed, 64 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>> index 2cb7f8e..88ff362 100644
>>> --- a/drivers/hwmon/lm90.c
>>> +++ b/drivers/hwmon/lm90.c
>>> @@ -89,6 +89,7 @@
>>>  #include <linux/err.h>
>>>  #include <linux/mutex.h>
>>>  #include <linux/sysfs.h>
>>> +#include <linux/interrupt.h>
>>>  
>>>  /*
>>>   * Addresses to scan
>>> @@ -179,6 +180,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
>>>  #define LM90_HAVE_TEMP3		(1 << 6) /* 3rd temperature sensor	*/
>>>  #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
>>>  
>>> +/* LM90 status */
>>> +#define LM90_LTHRM	(1 << 0)	/* local THERM limit tripped */
>>> +#define LM90_RTHRM	(1 << 1)	/* remote THERM limit tripped */
>>> +#define LM90_OPEN	(1 << 2)	/* remote is an open circuit */
>>> +#define LM90_RLOW	(1 << 3)	/* remote low temp limit tripped */
>>> +#define LM90_RHIGH	(1 << 4)	/* remote high temp limit tripped */
>>> +#define LM90_LLOW	(1 << 5)	/* local low temp limit tripped */
>>> +#define LM90_LHIGH	(1 << 6)	/* local high temp limit tripped */
>>> +#define LM90_BUSY	(1 << 7)	/* ADC is converting */
>>> +
>>> +#define MAX6696_RLOW	(1 << 3)	/* remote2 low temp limit tripped */
>>> +#define MAX6696_RHIGH	(1 << 4)	/* remote2 high temp limit tripped */
>>
>> I think this is a nice cleanup, but I'll leave it up to Guenter or Jean
>> to decide if they want to have this. One problem with the above is that
>> it's not immediately clear which register contains these bits. That's
>> often solved by using the register name as prefix but that will in turn
>> make the names for these bits rather long:
>>
>> 	#define LM90_REG_R_STATUS_LTHRM	(1 << 0)
>> 	...
>>
>> Perhaps something like
>>
>> 	#define LM90_STATUS_LTHRM	(1 << 0)
>>
>> would be a good compromise?
>>
> Something like that, yes.

Ok, I will change it.

> 
>> Also if Guenter and Jean agree that this is a nice cleanup, it should
>> probably go into a separate patch since it isn't directly related to the
>> IRQ support.
>>
> Correct.

Yes, it should be a separate patch, I will do it.

> 
>>>  /*
>>>   * Driver data (common to all clients)
>>>   */
>>> @@ -1423,6 +1437,43 @@ static void lm90_init_client(struct i2c_client *client)
>>>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>>>  }
>>>  
>>> +static void lm90_alarm_status(struct i2c_client *client,
>>> +			      u8 alarms, u8 alarms_max6696)
>>> +{
>>> +	if (alarms & (LM90_LLOW | LM90_LHIGH | LM90_LTHRM))
>>> +		dev_warn(&client->dev,
>>> +			 "temp%d out of range, please check!\n", 1);
>>> +	if (alarms & (LM90_RLOW | LM90_RHIGH | LM90_RTHRM))
>>> +		dev_warn(&client->dev,
>>> +			 "temp%d out of range, please check!\n", 2);
>>> +	if (alarms & LM90_OPEN)
>>> +		dev_warn(&client->dev,
>>> +			 "temp%d diode open, please check!\n", 2);
>>> +
>>> +	if (alarms_max6696 & (MAX6696_RLOW | MAX6696_RHIGH))
>>> +		dev_warn(&client->dev,
>>> +			 "temp%d out of range, please check!\n", 3);
>>> +}
>>> +
>>> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
>>> +{
>>> +	struct lm90_data *data = dev_id;
>>> +	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
>>> +	u8 alarms, alarms_max6696 = 0;
>>> +
>>> +	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
>>> +
>>> +	if (data->kind == max6696)
>>> +		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms_max6696);
>>> +
>>> +	if ((alarms & 0x7f) == 0 && (alarms_max6696 & 0xfe) == 0) {
>>> +		return IRQ_NONE;
>>
>> For non-MAX6696 chips this will evaluate to:
>>
>> 	if ((alarms & 0x7f) == 0 && (0 & 0xfe) == 0)
>>
>> and therefore be true for any value of "alarms" and therefore always
>> result in IRQ_NONE being returned.
>>
> Not really. If
> 	(alarms & 0xfe) == 0
> returns false (ie thee is an alarm) the expression is false and the
> if statement won't be executed. Or maybe I didn't get enough sleep
> last night ;).
> 
>> One other thing that slightly bugs me about this is that it's a little
>> tedious to pass alarms_max6696 around like this. Suppose yet another
>> slightly different variant is supported by this chip in the future,
>> it's possible it will require another alarms_XYZ variable that has to be
>> passed around. I don't have a better suggestion though, so maybe it can
>> remain like this and be rewritten at some point should the need arise.
>>
> The driver has tables to separate chips, so masks for status and status2 can
> be made generic and configurable if needed. But, yes, it would be better to
> select a generic name for the variable from the beginning (status and status2
> would do quite nicely).
> 
>> Thierry
> 
> 

Re: [PATCH v2 2/3] hwmon: (lm90) add support to handle IRQ.

[Guenter Roeck]

On Wed, Jul 10, 2013 at 07:25:38PM +0800, Wei Ni wrote:
> When the temperature exceed the limit range value,
> the driver can handle the interrupt.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  drivers/hwmon/lm90.c |   77 +++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 64 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 2cb7f8e..88ff362 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -89,6 +89,7 @@
>  #include <linux/err.h>
>  #include <linux/mutex.h>
>  #include <linux/sysfs.h>
> +#include <linux/interrupt.h>
>  
>  /*
>   * Addresses to scan
> @@ -179,6 +180,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
>  #define LM90_HAVE_TEMP3		(1 << 6) /* 3rd temperature sensor	*/
>  #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
>  
> +/* LM90 status */
> +#define LM90_LTHRM	(1 << 0)	/* local THERM limit tripped */
> +#define LM90_RTHRM	(1 << 1)	/* remote THERM limit tripped */
> +#define LM90_OPEN	(1 << 2)	/* remote is an open circuit */
> +#define LM90_RLOW	(1 << 3)	/* remote low temp limit tripped */
> +#define LM90_RHIGH	(1 << 4)	/* remote high temp limit tripped */
> +#define LM90_LLOW	(1 << 5)	/* local low temp limit tripped */
> +#define LM90_LHIGH	(1 << 6)	/* local high temp limit tripped */
> +#define LM90_BUSY	(1 << 7)	/* ADC is converting */
> +
> +#define MAX6696_RLOW	(1 << 3)	/* remote2 low temp limit tripped */
> +#define MAX6696_RHIGH	(1 << 4)	/* remote2 high temp limit tripped */
> +
>  /*
>   * Driver data (common to all clients)
>   */
> @@ -1423,6 +1437,43 @@ static void lm90_init_client(struct i2c_client *client)
>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>  }
>  
> +static void lm90_alarm_status(struct i2c_client *client,
> +			      u8 alarms, u8 alarms_max6696)
> +{
If you are introdcing a function to evaluate the alarm status, you might as well
copy the register reads as well as the mask evaluations into this function.

If you don't want to see the "Everything ok" output if nothing is wrong, it can
return a boolean indicating if a status bit was set. This way the calling code
can also more easily determine if it should return IRQ_NONE or IRQ_HANDLED.

> +	if (alarms & (LM90_LLOW | LM90_LHIGH | LM90_LTHRM))
> +		dev_warn(&client->dev,
> +			 "temp%d out of range, please check!\n", 1);
> +	if (alarms & (LM90_RLOW | LM90_RHIGH | LM90_RTHRM))
> +		dev_warn(&client->dev,
> +			 "temp%d out of range, please check!\n", 2);
> +	if (alarms & LM90_OPEN)
> +		dev_warn(&client->dev,
> +			 "temp%d diode open, please check!\n", 2);
> +
> +	if (alarms_max6696 & (MAX6696_RLOW | MAX6696_RHIGH))
> +		dev_warn(&client->dev,
> +			 "temp%d out of range, please check!\n", 3);
> +}
> +
> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
> +{
> +	struct lm90_data *data = dev_id;
> +	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
> +	u8 alarms, alarms_max6696 = 0;
> +
Please stick with alarms2 instead of alarms_max6696 as in the original code.

> +	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> +
> +	if (data->kind == max6696)
> +		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms_max6696);
> +
> +	if ((alarms & 0x7f) == 0 && (alarms_max6696 & 0xfe) == 0) {
> +		return IRQ_NONE;
> +	} else {

That else statement is unnecessary.

> +		lm90_alarm_status(client, alarms, alarms_max6696);
> +		return IRQ_HANDLED;
> +	}
> +}
> +
>  static int lm90_probe(struct i2c_client *client,
>  		      const struct i2c_device_id *id)
>  {
> @@ -1499,6 +1550,18 @@ static int lm90_probe(struct i2c_client *client,
>  		goto exit_remove_files;
>  	}
>  
> +	if (client->irq >= 0) {
> +		dev_dbg(dev, "lm90 IRQ: %d\n", client->irq);
> +		err = devm_request_threaded_irq(dev, client->irq,
> +						NULL, lm90_irq_thread,
> +						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +						"lm90", data);
> +		if (err < 0) {
> +			dev_err(dev, "cannot request interrupt\n");
> +			goto exit_remove_files;
> +		}
> +	}
> +
>  	return 0;
>  
>  exit_remove_files:
> @@ -1532,19 +1595,7 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
>  	if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {
>  		dev_info(&client->dev, "Everything OK\n");
>  	} else {
> -		if (alarms & 0x61)
> -			dev_warn(&client->dev,
> -				 "temp%d out of range, please check!\n", 1);
> -		if (alarms & 0x1a)
> -			dev_warn(&client->dev,
> -				 "temp%d out of range, please check!\n", 2);
> -		if (alarms & 0x04)
> -			dev_warn(&client->dev,
> -				 "temp%d diode open, please check!\n", 2);
> -
> -		if (alarms2 & 0x18)
> -			dev_warn(&client->dev,
> -				 "temp%d out of range, please check!\n", 3);
> +		lm90_alarm_status(client, alarms, alarms2);
>  
>  		/*
>  		 * Disable ALERT# output, because these chips don't implement
> -- 
> 1.7.9.5
> 
> 

Re: [PATCH v2 2/3] hwmon: (lm90) add support to handle IRQ.

[Wei Ni]

On 07/11/2013 02:18 AM, Guenter Roeck wrote:
> On Wed, Jul 10, 2013 at 07:25:38PM +0800, Wei Ni wrote:
>> When the temperature exceed the limit range value,
>> the driver can handle the interrupt.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  drivers/hwmon/lm90.c |   77 +++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 64 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index 2cb7f8e..88ff362 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -89,6 +89,7 @@
>>  #include <linux/err.h>
>>  #include <linux/mutex.h>
>>  #include <linux/sysfs.h>
>> +#include <linux/interrupt.h>
>>  
>>  /*
>>   * Addresses to scan
>> @@ -179,6 +180,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
>>  #define LM90_HAVE_TEMP3		(1 << 6) /* 3rd temperature sensor	*/
>>  #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
>>  
>> +/* LM90 status */
>> +#define LM90_LTHRM	(1 << 0)	/* local THERM limit tripped */
>> +#define LM90_RTHRM	(1 << 1)	/* remote THERM limit tripped */
>> +#define LM90_OPEN	(1 << 2)	/* remote is an open circuit */
>> +#define LM90_RLOW	(1 << 3)	/* remote low temp limit tripped */
>> +#define LM90_RHIGH	(1 << 4)	/* remote high temp limit tripped */
>> +#define LM90_LLOW	(1 << 5)	/* local low temp limit tripped */
>> +#define LM90_LHIGH	(1 << 6)	/* local high temp limit tripped */
>> +#define LM90_BUSY	(1 << 7)	/* ADC is converting */
>> +
>> +#define MAX6696_RLOW	(1 << 3)	/* remote2 low temp limit tripped */
>> +#define MAX6696_RHIGH	(1 << 4)	/* remote2 high temp limit tripped */
>> +
>>  /*
>>   * Driver data (common to all clients)
>>   */
>> @@ -1423,6 +1437,43 @@ static void lm90_init_client(struct i2c_client *client)
>>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>>  }
>>  
>> +static void lm90_alarm_status(struct i2c_client *client,
>> +			      u8 alarms, u8 alarms_max6696)
>> +{
> If you are introdcing a function to evaluate the alarm status, you might as well
> copy the register reads as well as the mask evaluations into this function.
> 
> If you don't want to see the "Everything ok" output if nothing is wrong, it can
> return a boolean indicating if a status bit was set. This way the calling code
> can also more easily determine if it should return IRQ_NONE or IRQ_HANDLED.

Yes, you are right, I will change change the function to:
lm90_is_tripped(*client)
{
/* read status register */
/* check bit */
/* return true or false to indicate if the limit tripped */
}

> 
>> +	if (alarms & (LM90_LLOW | LM90_LHIGH | LM90_LTHRM))
>> +		dev_warn(&client->dev,
>> +			 "temp%d out of range, please check!\n", 1);
>> +	if (alarms & (LM90_RLOW | LM90_RHIGH | LM90_RTHRM))
>> +		dev_warn(&client->dev,
>> +			 "temp%d out of range, please check!\n", 2);
>> +	if (alarms & LM90_OPEN)
>> +		dev_warn(&client->dev,
>> +			 "temp%d diode open, please check!\n", 2);
>> +
>> +	if (alarms_max6696 & (MAX6696_RLOW | MAX6696_RHIGH))
>> +		dev_warn(&client->dev,
>> +			 "temp%d out of range, please check!\n", 3);
>> +}
>> +
>> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
>> +{
>> +	struct lm90_data *data = dev_id;
>> +	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
>> +	u8 alarms, alarms_max6696 = 0;
>> +
> Please stick with alarms2 instead of alarms_max6696 as in the original code.
> 
>> +	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
>> +
>> +	if (data->kind == max6696)
>> +		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms_max6696);
>> +
>> +	if ((alarms & 0x7f) == 0 && (alarms_max6696 & 0xfe) == 0) {
>> +		return IRQ_NONE;
>> +	} else {
> 
> That else statement is unnecessary.
> 
>> +		lm90_alarm_status(client, alarms, alarms_max6696);
>> +		return IRQ_HANDLED;
>> +	}
>> +}
>> +
>>  static int lm90_probe(struct i2c_client *client,
>>  		      const struct i2c_device_id *id)
>>  {
>> @@ -1499,6 +1550,18 @@ static int lm90_probe(struct i2c_client *client,
>>  		goto exit_remove_files;
>>  	}
>>  
>> +	if (client->irq >= 0) {
>> +		dev_dbg(dev, "lm90 IRQ: %d\n", client->irq);
>> +		err = devm_request_threaded_irq(dev, client->irq,
>> +						NULL, lm90_irq_thread,
>> +						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>> +						"lm90", data);
>> +		if (err < 0) {
>> +			dev_err(dev, "cannot request interrupt\n");
>> +			goto exit_remove_files;
>> +		}
>> +	}
>> +
>>  	return 0;
>>  
>>  exit_remove_files:
>> @@ -1532,19 +1595,7 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
>>  	if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {
>>  		dev_info(&client->dev, "Everything OK\n");
>>  	} else {
>> -		if (alarms & 0x61)
>> -			dev_warn(&client->dev,
>> -				 "temp%d out of range, please check!\n", 1);
>> -		if (alarms & 0x1a)
>> -			dev_warn(&client->dev,
>> -				 "temp%d out of range, please check!\n", 2);
>> -		if (alarms & 0x04)
>> -			dev_warn(&client->dev,
>> -				 "temp%d diode open, please check!\n", 2);
>> -
>> -		if (alarms2 & 0x18)
>> -			dev_warn(&client->dev,
>> -				 "temp%d out of range, please check!\n", 3);
>> +		lm90_alarm_status(client, alarms, alarms2);
>>  
>>  		/*
>>  		 * Disable ALERT# output, because these chips don't implement
>> -- 
>> 1.7.9.5
>>
>>

[PATCH v2 3/3] hwmon: (lm90) use enums for the indexes of temp8 and temp11

[Wei Ni]

Using enums for the indexes and nrs of temp8 and temp11.
This make the code much more readable.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 drivers/hwmon/lm90.c |  179 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 114 insertions(+), 65 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 88ff362..14e0b5b 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -310,6 +310,59 @@ static const struct lm90_params lm90_params[] = {
 };
 
 /*
+ * TEMP8 register index
+ */
+enum lm90_temp8_reg_index {
+	TEMP8_LOCAL_LOW = 0,	/* 0: local low limit */
+	TEMP8_LOCAL_HIGH,	/* 1: local high limit */
+	TEMP8_LOCAL_CRIT,	/* 2: local critical limit */
+	TEMP8_REMOTE_CRIT,	/* 3: remote critical limit */
+	TEMP8_LOCAL_EMERG,	/* 4: local emergency limit
+				 * (max6659 and max6695/96)
+				 */
+	TEMP8_REMOTE_EMERG,	/* 5: remote emergency limit
+				 * (max6659 and max6695/96)
+				 */
+	TEMP8_REMOTE2_CRIT,	/* 6: remote 2 critical limit
+				 * (max6695/96 only)
+				 */
+	TEMP8_REMOTE2_EMERG,	/* 7: remote 2 emergency limit
+				 * (max6695/96 only)
+				 */
+	TEMP8_REG_NUM
+};
+
+/*
+ * TEMP11 register index
+ */
+enum lm90_temp11_reg_index {
+	TEMP11_REMOTE_TEMP = 0,	/* 0: remote input */
+	TEMP11_REMOTE_LOW,	/* 1: remote low limit */
+	TEMP11_REMOTE_HIGH,	/* 2: remote high limit */
+	TEMP11_REMOTE_OFFSET,	/* 3: remote offset
+				 * (except max6646, max6657/58/59,
+				 *  and max6695/96)
+				 */
+	TEMP11_LOCAL_TEMP,	/* 4: local input */
+	TEMP11_REMOTE2_TEMP,	/* 5: remote 2 input (max6695/96 only) */
+	TEMP11_REMOTE2_LOW,	/* 6: remote 2 low limit (max6695/96 only) */
+	TEMP11_REMOTE2_HIGH,	/* 7: remote 2 high limit (max6695/96 only) */
+	TEMP11_REG_NUM
+};
+
+/*
+ * TEMP11 register NR
+ */
+enum lm90_temp11_reg_nr {
+	NR_CHAN_0_REMOTE_LOW = 0,	/* 0: channel 0, remote low limit */
+	NR_CHAN_0_REMOTE_HIGH,		/* 1: channel 0, remote high limit */
+	NR_CHAN_0_REMOTE_OFFSET,	/* 2: channel 0, remote offset */
+	NR_CHAN_1_REMOTE_LOW,		/* 3: channel 1, remote low limit */
+	NR_CHAN_1_REMOTE_HIGH,		/* 4: channel 1, remote high limit */
+	NR_NUM				/* number of the NRs for temp11 */
+};
+
+/*
  * Client data (each client gets its own)
  */
 
@@ -331,25 +384,8 @@ struct lm90_data {
 	u8 reg_local_ext;	/* local extension register offset */
 
 	/* registers values */
-	s8 temp8[8];	/* 0: local low limit
-			 * 1: local high limit
-			 * 2: local critical limit
-			 * 3: remote critical limit
-			 * 4: local emergency limit (max6659 and max6695/96)
-			 * 5: remote emergency limit (max6659 and max6695/96)
-			 * 6: remote 2 critical limit (max6695/96 only)
-			 * 7: remote 2 emergency limit (max6695/96 only)
-			 */
-	s16 temp11[8];	/* 0: remote input
-			 * 1: remote low limit
-			 * 2: remote high limit
-			 * 3: remote offset (except max6646, max6657/58/59,
-			 *		     and max6695/96)
-			 * 4: local input
-			 * 5: remote 2 input (max6695/96 only)
-			 * 6: remote 2 low limit (max6695/96 only)
-			 * 7: remote 2 high limit (max6695/96 only)
-			 */
+	s8 temp8[TEMP8_REG_NUM];
+	s16 temp11[TEMP11_REG_NUM];
 	u8 temp_hyst;
 	u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
 };
@@ -491,37 +527,42 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 		u8 alarms;
 
 		dev_dbg(&client->dev, "Updating lm90 data.\n");
-		lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
-		lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, &data->temp8[1]);
-		lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, &data->temp8[2]);
-		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_LOW,
+				&data->temp8[TEMP8_LOCAL_LOW]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH,
+				&data->temp8[TEMP8_LOCAL_HIGH]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT,
+				&data->temp8[TEMP8_LOCAL_CRIT]);
+		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
+				&data->temp8[TEMP8_REMOTE_CRIT]);
 		lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
 
 		if (data->reg_local_ext) {
 			lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
 				    data->reg_local_ext,
-				    &data->temp11[4]);
+				    &data->temp11[TEMP11_LOCAL_TEMP]);
 		} else {
 			if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
 					  &h) == 0)
-				data->temp11[4] = h << 8;
+				data->temp11[TEMP11_LOCAL_TEMP] = h << 8;
 		}
 		lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
-			    LM90_REG_R_REMOTE_TEMPL, &data->temp11[0]);
+			LM90_REG_R_REMOTE_TEMPL,
+			&data->temp11[TEMP11_REMOTE_TEMP]);
 
 		if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
-			data->temp11[1] = h << 8;
+			data->temp11[TEMP11_REMOTE_LOW] = h << 8;
 			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
 					  &l) == 0)
-				data->temp11[1] |= l;
+				data->temp11[TEMP11_REMOTE_LOW] |= l;
 		}
 		if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
-			data->temp11[2] = h << 8;
+			data->temp11[TEMP11_REMOTE_HIGH] = h << 8;
 			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
 					  &l) == 0)
-				data->temp11[2] |= l;
+				data->temp11[TEMP11_REMOTE_HIGH] |= l;
 		}
 
 		if (data->flags & LM90_HAVE_OFFSET) {
@@ -529,13 +570,14 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 					  &h) == 0
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
 					  &l) == 0)
-				data->temp11[3] = (h << 8) | l;
+				data->temp11[TEMP11_REMOTE_OFFSET] =
+								(h << 8) | l;
 		}
 		if (data->flags & LM90_HAVE_EMERGENCY) {
 			lm90_read_reg(client, MAX6659_REG_R_LOCAL_EMERG,
-				      &data->temp8[4]);
+				      &data->temp8[TEMP8_LOCAL_EMERG]);
 			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
-				      &data->temp8[5]);
+				      &data->temp8[TEMP8_REMOTE_EMERG]);
 		}
 		lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
 		data->alarms = alarms;	/* save as 16 bit value */
@@ -543,15 +585,16 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 		if (data->kind == max6696) {
 			lm90_select_remote_channel(client, data, 1);
 			lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
-				      &data->temp8[6]);
+				      &data->temp8[TEMP8_REMOTE2_CRIT]);
 			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
-				      &data->temp8[7]);
+				      &data->temp8[TEMP8_REMOTE2_EMERG]);
 			lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
-				    LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
+					LM90_REG_R_REMOTE_TEMPL,
+					&data->temp11[TEMP11_REMOTE2_TEMP]);
 			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h))
-				data->temp11[6] = h << 8;
+				data->temp11[TEMP11_REMOTE2_LOW] = h << 8;
 			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h))
-				data->temp11[7] = h << 8;
+				data->temp11[TEMP11_REMOTE2_HIGH] = h << 8;
 			lm90_select_remote_channel(client, data, 0);
 
 			if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2,
@@ -748,7 +791,7 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
 
 static void write_temp8(struct device *dev, int index, long val)
 {
-	static const u8 reg[8] = {
+	static const u8 reg[TEMP8_REG_NUM] = {
 		LM90_REG_W_LOCAL_LOW,
 		LM90_REG_W_LOCAL_HIGH,
 		LM90_REG_W_LOCAL_CRIT,
@@ -834,7 +877,7 @@ static void write_temp11(struct device *dev, int nr, int index, long val)
 		u8 high;
 		u8 low;
 		int channel;
-	} reg[5] = {
+	} reg[NR_NUM] = {
 		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 },
 		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 },
 		{ LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 },
@@ -925,11 +968,12 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
 
 	mutex_lock(&data->update_lock);
 	if (data->kind == adt7461)
-		temp = temp_from_u8_adt7461(data, data->temp8[2]);
+		temp = temp_from_u8_adt7461(data,
+					data->temp8[TEMP8_LOCAL_CRIT]);
 	else if (data->kind == max6646)
-		temp = temp_from_u8(data->temp8[2]);
+		temp = temp_from_u8(data->temp8[TEMP8_LOCAL_CRIT]);
 	else
-		temp = temp_from_s8(data->temp8[2]);
+		temp = temp_from_s8(data->temp8[TEMP8_LOCAL_CRIT]);
 
 	data->temp_hyst = hyst_to_reg(temp - val);
 	i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST,
@@ -983,25 +1027,28 @@ static ssize_t set_update_interval(struct device *dev,
 	return count;
 }
 
-static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4);
-static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0);
+static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL,
+	NR_CHAN_0_REMOTE_LOW, TEMP11_LOCAL_TEMP);
+static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL,
+	NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_TEMP);
 static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 0);
+	set_temp8, TEMP8_LOCAL_LOW);
 static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 0, 1);
+	set_temp11, NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_LOW);
 static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 1);
+	set_temp8, TEMP8_LOCAL_HIGH);
 static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 1, 2);
+	set_temp11, NR_CHAN_0_REMOTE_HIGH, TEMP11_REMOTE_HIGH);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 2);
+	set_temp8, TEMP8_LOCAL_CRIT);
 static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 3);
+	set_temp8, TEMP8_REMOTE_CRIT);
 static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst,
-	set_temphyst, 2);
-static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3);
+	set_temphyst, TEMP8_LOCAL_CRIT);
+static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL,
+	TEMP8_REMOTE_CRIT);
 static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 2, 3);
+	set_temp11, NR_CHAN_0_REMOTE_OFFSET, TEMP11_REMOTE_OFFSET);
 
 /* Individual alarm files */
 static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0);
@@ -1049,13 +1096,13 @@ static const struct attribute_group lm90_group = {
  * Additional attributes for devices with emergency sensors
  */
 static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 4);
+	set_temp8, TEMP8_LOCAL_EMERG);
 static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 5);
+	set_temp8, TEMP8_REMOTE_EMERG);
 static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 4);
+			  NULL, TEMP8_LOCAL_EMERG);
 static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 5);
+			  NULL, TEMP8_REMOTE_EMERG);
 
 static struct attribute *lm90_emergency_attributes[] = {
 	&sensor_dev_attr_temp1_emergency.dev_attr.attr,
@@ -1085,18 +1132,20 @@ static const struct attribute_group lm90_emergency_alarm_group = {
 /*
  * Additional attributes for devices with 3 temperature sensors
  */
-static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5);
+static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL,
+	NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE2_TEMP);
 static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 3, 6);
+	set_temp11, NR_CHAN_1_REMOTE_LOW, TEMP11_REMOTE2_LOW);
 static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 4, 7);
+	set_temp11, NR_CHAN_1_REMOTE_HIGH, TEMP11_REMOTE2_HIGH);
 static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 6);
-static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 6);
+	set_temp8, TEMP8_REMOTE2_CRIT);
+static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL,
+	TEMP8_REMOTE2_CRIT);
 static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 7);
+	set_temp8, TEMP8_REMOTE2_EMERG);
 static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 7);
+			  NULL, TEMP8_REMOTE2_EMERG);
 
 static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9);
 static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10);
-- 
1.7.9.5

Re: [PATCH v2 3/3] hwmon: (lm90) use enums for the indexes of temp8 and temp11

[Guenter Roeck]

On Wed, Jul 10, 2013 at 07:25:39PM +0800, Wei Ni wrote:
> Using enums for the indexes and nrs of temp8 and temp11.
> This make the code much more readable.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>

I don't really have a strong opinion on this one. Nice, but is it really worth it ? 
Jean, any comments ?

Guenter

> ---
>  drivers/hwmon/lm90.c |  179 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 114 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 88ff362..14e0b5b 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -310,6 +310,59 @@ static const struct lm90_params lm90_params[] = {
>  };
>  
>  /*
> + * TEMP8 register index
> + */
> +enum lm90_temp8_reg_index {
> +	TEMP8_LOCAL_LOW = 0,	/* 0: local low limit */
> +	TEMP8_LOCAL_HIGH,	/* 1: local high limit */
> +	TEMP8_LOCAL_CRIT,	/* 2: local critical limit */
> +	TEMP8_REMOTE_CRIT,	/* 3: remote critical limit */
> +	TEMP8_LOCAL_EMERG,	/* 4: local emergency limit
> +				 * (max6659 and max6695/96)
> +				 */
> +	TEMP8_REMOTE_EMERG,	/* 5: remote emergency limit
> +				 * (max6659 and max6695/96)
> +				 */
> +	TEMP8_REMOTE2_CRIT,	/* 6: remote 2 critical limit
> +				 * (max6695/96 only)
> +				 */
> +	TEMP8_REMOTE2_EMERG,	/* 7: remote 2 emergency limit
> +				 * (max6695/96 only)
> +				 */
> +	TEMP8_REG_NUM
> +};
> +
> +/*
> + * TEMP11 register index
> + */
> +enum lm90_temp11_reg_index {
> +	TEMP11_REMOTE_TEMP = 0,	/* 0: remote input */
> +	TEMP11_REMOTE_LOW,	/* 1: remote low limit */
> +	TEMP11_REMOTE_HIGH,	/* 2: remote high limit */
> +	TEMP11_REMOTE_OFFSET,	/* 3: remote offset
> +				 * (except max6646, max6657/58/59,
> +				 *  and max6695/96)
> +				 */
> +	TEMP11_LOCAL_TEMP,	/* 4: local input */
> +	TEMP11_REMOTE2_TEMP,	/* 5: remote 2 input (max6695/96 only) */
> +	TEMP11_REMOTE2_LOW,	/* 6: remote 2 low limit (max6695/96 only) */
> +	TEMP11_REMOTE2_HIGH,	/* 7: remote 2 high limit (max6695/96 only) */
> +	TEMP11_REG_NUM
> +};
> +
> +/*
> + * TEMP11 register NR
> + */
> +enum lm90_temp11_reg_nr {
> +	NR_CHAN_0_REMOTE_LOW = 0,	/* 0: channel 0, remote low limit */
> +	NR_CHAN_0_REMOTE_HIGH,		/* 1: channel 0, remote high limit */
> +	NR_CHAN_0_REMOTE_OFFSET,	/* 2: channel 0, remote offset */
> +	NR_CHAN_1_REMOTE_LOW,		/* 3: channel 1, remote low limit */
> +	NR_CHAN_1_REMOTE_HIGH,		/* 4: channel 1, remote high limit */
> +	NR_NUM				/* number of the NRs for temp11 */
> +};
> +
> +/*
>   * Client data (each client gets its own)
>   */
>  
> @@ -331,25 +384,8 @@ struct lm90_data {
>  	u8 reg_local_ext;	/* local extension register offset */
>  
>  	/* registers values */
> -	s8 temp8[8];	/* 0: local low limit
> -			 * 1: local high limit
> -			 * 2: local critical limit
> -			 * 3: remote critical limit
> -			 * 4: local emergency limit (max6659 and max6695/96)
> -			 * 5: remote emergency limit (max6659 and max6695/96)
> -			 * 6: remote 2 critical limit (max6695/96 only)
> -			 * 7: remote 2 emergency limit (max6695/96 only)
> -			 */
> -	s16 temp11[8];	/* 0: remote input
> -			 * 1: remote low limit
> -			 * 2: remote high limit
> -			 * 3: remote offset (except max6646, max6657/58/59,
> -			 *		     and max6695/96)
> -			 * 4: local input
> -			 * 5: remote 2 input (max6695/96 only)
> -			 * 6: remote 2 low limit (max6695/96 only)
> -			 * 7: remote 2 high limit (max6695/96 only)
> -			 */
> +	s8 temp8[TEMP8_REG_NUM];
> +	s16 temp11[TEMP11_REG_NUM];
>  	u8 temp_hyst;
>  	u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
>  };
> @@ -491,37 +527,42 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>  		u8 alarms;
>  
>  		dev_dbg(&client->dev, "Updating lm90 data.\n");
> -		lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
> -		lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, &data->temp8[1]);
> -		lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, &data->temp8[2]);
> -		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
> +		lm90_read_reg(client, LM90_REG_R_LOCAL_LOW,
> +				&data->temp8[TEMP8_LOCAL_LOW]);
> +		lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH,
> +				&data->temp8[TEMP8_LOCAL_HIGH]);
> +		lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT,
> +				&data->temp8[TEMP8_LOCAL_CRIT]);
> +		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
> +				&data->temp8[TEMP8_REMOTE_CRIT]);
>  		lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
>  
>  		if (data->reg_local_ext) {
>  			lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
>  				    data->reg_local_ext,
> -				    &data->temp11[4]);
> +				    &data->temp11[TEMP11_LOCAL_TEMP]);
>  		} else {
>  			if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
>  					  &h) == 0)
> -				data->temp11[4] = h << 8;
> +				data->temp11[TEMP11_LOCAL_TEMP] = h << 8;
>  		}
>  		lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
> -			    LM90_REG_R_REMOTE_TEMPL, &data->temp11[0]);
> +			LM90_REG_R_REMOTE_TEMPL,
> +			&data->temp11[TEMP11_REMOTE_TEMP]);
>  
>  		if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
> -			data->temp11[1] = h << 8;
> +			data->temp11[TEMP11_REMOTE_LOW] = h << 8;
>  			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
>  			 && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
>  					  &l) == 0)
> -				data->temp11[1] |= l;
> +				data->temp11[TEMP11_REMOTE_LOW] |= l;
>  		}
>  		if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
> -			data->temp11[2] = h << 8;
> +			data->temp11[TEMP11_REMOTE_HIGH] = h << 8;
>  			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
>  			 && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
>  					  &l) == 0)
> -				data->temp11[2] |= l;
> +				data->temp11[TEMP11_REMOTE_HIGH] |= l;
>  		}
>  
>  		if (data->flags & LM90_HAVE_OFFSET) {
> @@ -529,13 +570,14 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>  					  &h) == 0
>  			 && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
>  					  &l) == 0)
> -				data->temp11[3] = (h << 8) | l;
> +				data->temp11[TEMP11_REMOTE_OFFSET] =
> +								(h << 8) | l;
>  		}
>  		if (data->flags & LM90_HAVE_EMERGENCY) {
>  			lm90_read_reg(client, MAX6659_REG_R_LOCAL_EMERG,
> -				      &data->temp8[4]);
> +				      &data->temp8[TEMP8_LOCAL_EMERG]);
>  			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
> -				      &data->temp8[5]);
> +				      &data->temp8[TEMP8_REMOTE_EMERG]);
>  		}
>  		lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
>  		data->alarms = alarms;	/* save as 16 bit value */
> @@ -543,15 +585,16 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>  		if (data->kind == max6696) {
>  			lm90_select_remote_channel(client, data, 1);
>  			lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
> -				      &data->temp8[6]);
> +				      &data->temp8[TEMP8_REMOTE2_CRIT]);
>  			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
> -				      &data->temp8[7]);
> +				      &data->temp8[TEMP8_REMOTE2_EMERG]);
>  			lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
> -				    LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
> +					LM90_REG_R_REMOTE_TEMPL,
> +					&data->temp11[TEMP11_REMOTE2_TEMP]);
>  			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h))
> -				data->temp11[6] = h << 8;
> +				data->temp11[TEMP11_REMOTE2_LOW] = h << 8;
>  			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h))
> -				data->temp11[7] = h << 8;
> +				data->temp11[TEMP11_REMOTE2_HIGH] = h << 8;
>  			lm90_select_remote_channel(client, data, 0);
>  
>  			if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2,
> @@ -748,7 +791,7 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
>  
>  static void write_temp8(struct device *dev, int index, long val)
>  {
> -	static const u8 reg[8] = {
> +	static const u8 reg[TEMP8_REG_NUM] = {
>  		LM90_REG_W_LOCAL_LOW,
>  		LM90_REG_W_LOCAL_HIGH,
>  		LM90_REG_W_LOCAL_CRIT,
> @@ -834,7 +877,7 @@ static void write_temp11(struct device *dev, int nr, int index, long val)
>  		u8 high;
>  		u8 low;
>  		int channel;
> -	} reg[5] = {
> +	} reg[NR_NUM] = {
>  		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 },
>  		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 },
>  		{ LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 },
> @@ -925,11 +968,12 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
>  
>  	mutex_lock(&data->update_lock);
>  	if (data->kind == adt7461)
> -		temp = temp_from_u8_adt7461(data, data->temp8[2]);
> +		temp = temp_from_u8_adt7461(data,
> +					data->temp8[TEMP8_LOCAL_CRIT]);
>  	else if (data->kind == max6646)
> -		temp = temp_from_u8(data->temp8[2]);
> +		temp = temp_from_u8(data->temp8[TEMP8_LOCAL_CRIT]);
>  	else
> -		temp = temp_from_s8(data->temp8[2]);
> +		temp = temp_from_s8(data->temp8[TEMP8_LOCAL_CRIT]);
>  
>  	data->temp_hyst = hyst_to_reg(temp - val);
>  	i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST,
> @@ -983,25 +1027,28 @@ static ssize_t set_update_interval(struct device *dev,
>  	return count;
>  }
>  
> -static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4);
> -static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0);
> +static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL,
> +	NR_CHAN_0_REMOTE_LOW, TEMP11_LOCAL_TEMP);
> +static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL,
> +	NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_TEMP);
>  static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, 0);
> +	set_temp8, TEMP8_LOCAL_LOW);
>  static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
> -	set_temp11, 0, 1);
> +	set_temp11, NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_LOW);
>  static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, 1);
> +	set_temp8, TEMP8_LOCAL_HIGH);
>  static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
> -	set_temp11, 1, 2);
> +	set_temp11, NR_CHAN_0_REMOTE_HIGH, TEMP11_REMOTE_HIGH);
>  static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, 2);
> +	set_temp8, TEMP8_LOCAL_CRIT);
>  static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, 3);
> +	set_temp8, TEMP8_REMOTE_CRIT);
>  static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst,
> -	set_temphyst, 2);
> -static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3);
> +	set_temphyst, TEMP8_LOCAL_CRIT);
> +static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL,
> +	TEMP8_REMOTE_CRIT);
>  static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
> -	set_temp11, 2, 3);
> +	set_temp11, NR_CHAN_0_REMOTE_OFFSET, TEMP11_REMOTE_OFFSET);
>  
>  /* Individual alarm files */
>  static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0);
> @@ -1049,13 +1096,13 @@ static const struct attribute_group lm90_group = {
>   * Additional attributes for devices with emergency sensors
>   */
>  static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, 4);
> +	set_temp8, TEMP8_LOCAL_EMERG);
>  static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, 5);
> +	set_temp8, TEMP8_REMOTE_EMERG);
>  static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst,
> -			  NULL, 4);
> +			  NULL, TEMP8_LOCAL_EMERG);
>  static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst,
> -			  NULL, 5);
> +			  NULL, TEMP8_REMOTE_EMERG);
>  
>  static struct attribute *lm90_emergency_attributes[] = {
>  	&sensor_dev_attr_temp1_emergency.dev_attr.attr,
> @@ -1085,18 +1132,20 @@ static const struct attribute_group lm90_emergency_alarm_group = {
>  /*
>   * Additional attributes for devices with 3 temperature sensors
>   */
> -static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5);
> +static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL,
> +	NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE2_TEMP);
>  static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
> -	set_temp11, 3, 6);
> +	set_temp11, NR_CHAN_1_REMOTE_LOW, TEMP11_REMOTE2_LOW);
>  static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
> -	set_temp11, 4, 7);
> +	set_temp11, NR_CHAN_1_REMOTE_HIGH, TEMP11_REMOTE2_HIGH);
>  static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, 6);
> -static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 6);
> +	set_temp8, TEMP8_REMOTE2_CRIT);
> +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL,
> +	TEMP8_REMOTE2_CRIT);
>  static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, 7);
> +	set_temp8, TEMP8_REMOTE2_EMERG);
>  static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst,
> -			  NULL, 7);
> +			  NULL, TEMP8_REMOTE2_EMERG);
>  
>  static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9);
>  static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10);
> -- 
> 1.7.9.5
> 
> 

[PATCH v2 0/3] expose lm90 to thermal fw

[Wei Ni]

Expose lm90 to thermal framework via DT nodes.

Changes from v1:
1. remove the unnecessary log messages, per Guenter's request.
2. add thermal zones node for nct1008 on dalmore.

Wei Ni (3):
  hwmon: (lm90) split set&show temp as common codes
  hwmon: lm90: expose to thermal fw via DT nodes
  ARM: tegra: dalmore: add thermal zones for nct1008

 arch/arm/boot/dts/tegra114-dalmore.dts |   20 ++-
 drivers/hwmon/lm90.c                   |  222 ++++++++++++++++++++++++--------
 2 files changed, 186 insertions(+), 56 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/3] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

Split set&show temp codes as common functions, so we can use it
directly when implement linux thermal framework.
And handle error return value for the lm90_select_remote_channel
and write_tempx, then set_temp8 and set_temp11 could return it
to user-space.

Change-Id: Id7178f680718fd54b9ba953118d91052b59b692e
Signed-off-by: Wei Ni <wni@nvidia.com>
Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/hwmon/lm90.c |  164 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 109 insertions(+), 55 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index c9ff08d..cb33dcf 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -473,20 +473,23 @@ static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
  * various registers have different meanings as a result of selecting a
  * non-default remote channel.
  */
-static inline void lm90_select_remote_channel(struct i2c_client *client,
-					      struct lm90_data *data,
-					      int channel)
+static inline int lm90_select_remote_channel(struct i2c_client *client,
+					     struct lm90_data *data,
+					     int channel)
 {
 	u8 config;
+	int err = 0;
 
 	if (data->kind == max6696) {
 		lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
 		config &= ~0x08;
 		if (channel)
 			config |= 0x08;
-		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
-					  config);
+		err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
+						config);
 	}
+
+	return err;
 }
 
 /*
@@ -759,29 +762,34 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val)
  * Sysfs stuff
  */
 
-static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
-			  char *buf)
+static int read_temp8(struct device *dev, int index)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
 
 	if (data->kind == adt7461 || data->kind == tmp451)
-		temp = temp_from_u8_adt7461(data, data->temp8[attr->index]);
+		temp = temp_from_u8_adt7461(data, data->temp8[index]);
 	else if (data->kind == max6646)
-		temp = temp_from_u8(data->temp8[attr->index]);
+		temp = temp_from_u8(data->temp8[index]);
 	else
-		temp = temp_from_s8(data->temp8[attr->index]);
+		temp = temp_from_s8(data->temp8[index]);
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind == lm99 && attr->index == 3)
+	if (data->kind == lm99 && index == 3)
 		temp += 16000;
 
-	return sprintf(buf, "%d\n", temp);
+	return temp;
 }
 
-static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
-			 const char *buf, size_t count)
+static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
+			  char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+	return sprintf(buf, "%d\n", read_temp8(dev, attr->index));
+}
+
+static int write_temp8(struct device *dev, int index, long val)
 {
 	static const u8 reg[TEMP8_REG_NUM] = {
 		LM90_REG_W_LOCAL_LOW,
@@ -794,60 +802,79 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
 		MAX6659_REG_W_REMOTE_EMERG,
 	};
 
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct lm90_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
-	int nr = attr->index;
-	long val;
 	int err;
 
-	err = kstrtol(buf, 10, &val);
-	if (err < 0)
-		return err;
-
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind == lm99 && attr->index == 3)
+	if (data->kind == lm99 && index == 3)
 		val -= 16000;
 
 	mutex_lock(&data->update_lock);
 	if (data->kind == adt7461 || data->kind == tmp451)
-		data->temp8[nr] = temp_to_u8_adt7461(data, val);
+		data->temp8[index] = temp_to_u8_adt7461(data, val);
 	else if (data->kind == max6646)
-		data->temp8[nr] = temp_to_u8(val);
+		data->temp8[index] = temp_to_u8(val);
 	else
-		data->temp8[nr] = temp_to_s8(val);
-
-	lm90_select_remote_channel(client, data, nr >= 6);
-	i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
-	lm90_select_remote_channel(client, data, 0);
+		data->temp8[index] = temp_to_s8(val);
 
+	if ((err = lm90_select_remote_channel(client, data, index >= 6)) ||
+	    (err = i2c_smbus_write_byte_data(client, reg[index],
+					     data->temp8[index])) ||
+	    (err = lm90_select_remote_channel(client, data, 0)))
+		dev_err(dev, "write_temp8 failed, err %d\n", err);
 	mutex_unlock(&data->update_lock);
+
+	return err;
+}
+
+static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
+			 const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	int index = attr->index;
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	err = write_temp8(dev, index, val);
+	if (err < 0)
+		return err;
+
 	return count;
 }
 
-static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
-			   char *buf)
+static int read_temp11(struct device *dev, int index)
 {
-	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
 	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
 
 	if (data->kind == adt7461 || data->kind == tmp451)
-		temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
+		temp = temp_from_u16_adt7461(data, data->temp11[index]);
 	else if (data->kind == max6646)
-		temp = temp_from_u16(data->temp11[attr->index]);
+		temp = temp_from_u16(data->temp11[index]);
 	else
-		temp = temp_from_s16(data->temp11[attr->index]);
+		temp = temp_from_s16(data->temp11[index]);
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind == lm99 &&  attr->index <= 2)
+	if (data->kind == lm99 && index <= 2)
 		temp += 16000;
 
-	return sprintf(buf, "%d\n", temp);
+	return temp;
 }
 
-static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
-			  const char *buf, size_t count)
+static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
+			   char *buf)
+{
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+
+	return sprintf(buf, "%d\n", read_temp11(dev, attr->index));
+}
+
+static int write_temp11(struct device *dev, int nr, int index, long val)
 {
 	struct {
 		u8 high;
@@ -861,18 +888,10 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
 	};
 
-	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
 	struct lm90_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
-	int nr = attr->nr;
-	int index = attr->index;
-	long val;
 	int err;
 
-	err = kstrtol(buf, 10, &val);
-	if (err < 0)
-		return err;
-
 	/* +16 degrees offset for temp2 for the LM99 */
 	if (data->kind == lm99 && index <= 2)
 		val -= 16000;
@@ -887,15 +906,50 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 	else
 		data->temp11[index] = temp_to_s8(val) << 8;
 
-	lm90_select_remote_channel(client, data, reg[nr].channel);
-	i2c_smbus_write_byte_data(client, reg[nr].high,
-				  data->temp11[index] >> 8);
-	if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
-		i2c_smbus_write_byte_data(client, reg[nr].low,
-					  data->temp11[index] & 0xff);
-	lm90_select_remote_channel(client, data, 0);
+	err = lm90_select_remote_channel(client, data, reg[nr].channel);
+	if (err)
+		goto error;
+
+	err = i2c_smbus_write_byte_data(client, reg[nr].high,
+					data->temp11[index] >> 8);
+	if (err)
+		goto error;
+
+	if (data->flags & LM90_HAVE_REM_LIMIT_EXT) {
+		err = i2c_smbus_write_byte_data(client, reg[nr].low,
+						data->temp11[index] & 0xff);
+		if (err)
+			goto error;
+	}
+
+	err = lm90_select_remote_channel(client, data, 0);
+
+error:
+	if (err)
+		dev_err(dev, "write_temp11 failed, err %d\n", err);
 
 	mutex_unlock(&data->update_lock);
+
+	return err;
+}
+
+static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
+			  const char *buf, size_t count)
+{
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+	int nr = attr->nr;
+	int index = attr->index;
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	err = write_temp11(dev, nr, index, val);
+	if (err < 0)
+		return err;
+
 	return count;
 }
 
-- 
1.7.9.5