[PATCH v4 0/3] Lm90 Enhancements

[PATCH v4 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 v4, previous version patches:
[RFC]: http://thread.gmane.org/gmane.linux.power-management.general/31056
[v1]: http://thread.gmane.org/gmane.linux.ports.tegra/11710/
[v2]: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg465555.html
[v3]: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg466772.html

Changes from v3:
1. remove the patch "hwmon: (lm90) split set&show temp as common codes",
and sent it as a separated one.
2. fix the bug of second read on STATUS register.
3. fix some code style issue according to Jean's comments.

Changes from v2:
1. update the defines for status bit, and go into a separate patch.
2. introduce the new lm90_is_tripped() for lm90_irq_thread and lm90_alert.

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) Define status bits
  hwmon: (lm90) add support to handle IRQ
  hwmon: (lm90) use enums for the indexes of temp8 and temp11

 drivers/hwmon/lm90.c |  252 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 163 insertions(+), 89 deletions(-)

-- 
1.7.9.5

[PATCH v4 1/3] hwmon: (lm90) Define status bits

[Wei Ni]

Add bit defines for the status register. And add a function
lm90_is_tripped() which will read status register and return
tripped or not, then lm90_alert can call it directly, and in the
future the IRQ thread also can use it.

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

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index cdff742..1da2eff 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -179,6 +179,18 @@ 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_STATUS_LTHRM	(1 << 0) /* local THERM limit tripped */
+#define LM90_STATUS_RTHRM	(1 << 1) /* remote THERM limit tripped */
+#define LM90_STATUS_OPEN	(1 << 2) /* remote is an open circuit */
+#define LM90_STATUS_RLOW	(1 << 3) /* remote low temp limit tripped */
+#define LM90_STATUS_RHIGH	(1 << 4) /* remote high temp limit tripped */
+#define LM90_STATUS_LLOW	(1 << 5) /* local low temp limit tripped */
+#define LM90_STATUS_LHIGH	(1 << 6) /* local high temp limit tripped */
+
+#define MAX6696_STATUS2_RLOW	(1 << 3) /* remote2 low temp limit tripped */
+#define MAX6696_STATUS2_RHIGH	(1 << 4) /* remote2 high temp limit tripped */
+
 /*
  * Driver data (common to all clients)
  */
@@ -1391,6 +1403,36 @@ static void lm90_init_client(struct i2c_client *client)
 		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
 }
 
+static bool lm90_is_tripped(struct i2c_client *client)
+{
+	struct lm90_data *data = i2c_get_clientdata(client);
+	u8 status, status2 = 0;
+
+	lm90_read_reg(client, LM90_REG_R_STATUS, &status);
+
+	if (data->kind == max6696)
+		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &status2);
+
+	if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0)
+		return false;
+
+	if (status & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM))
+		dev_warn(&client->dev,
+			 "temp%d out of range, please check!\n", 1);
+	if (status & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM))
+		dev_warn(&client->dev,
+			 "temp%d out of range, please check!\n", 2);
+	if (status & LM90_STATUS_OPEN)
+		dev_warn(&client->dev,
+			 "temp%d diode open, please check!\n", 2);
+
+	if (status2 & (MAX6696_STATUS2_RLOW | MAX6696_STATUS2_RHIGH))
+		dev_warn(&client->dev,
+			 "temp%d out of range, please check!\n", 3);
+
+	return true;
+}
+
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id)
 {
@@ -1489,36 +1531,17 @@ static int lm90_remove(struct i2c_client *client)
 
 static void lm90_alert(struct i2c_client *client, unsigned int flag)
 {
-	struct lm90_data *data = i2c_get_clientdata(client);
-	u8 config, alarms, alarms2 = 0;
-
-	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
-
-	if (data->kind == max6696)
-		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms2);
-
-	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);
-
+	if (lm90_is_tripped(client)) {
 		/*
 		 * Disable ALERT# output, because these chips don't implement
 		 * SMBus alert correctly; they should only hold the alert line
 		 * low briefly.
 		 */
+		struct lm90_data *data = i2c_get_clientdata(client);
+		u8 config, alarms;
+
+		lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
+
 		if ((data->flags & LM90_HAVE_BROKEN_ALERT)
 		 && (alarms & data->alert_alarms)) {
 			dev_dbg(&client->dev, "Disabling ALERT#\n");
@@ -1526,6 +1549,8 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
 			i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
 						  config | 0x80);
 		}
+	} else {
+		dev_info(&client->dev, "Everything OK\n");
 	}
 }
 
-- 
1.7.9.5

Re: [PATCH v4 1/3] hwmon: (lm90) Define status bits

[Jean Delvare]

Hi Wei,

On Wed, 7 Aug 2013 14:18:24 +0800, Wei Ni wrote:
> Add bit defines for the status register. And add a function
> lm90_is_tripped() which will read status register and return
> tripped or not, then lm90_alert can call it directly, and in the
> future the IRQ thread also can use it.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  drivers/hwmon/lm90.c |   75 +++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 50 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index cdff742..1da2eff 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -179,6 +179,18 @@ 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_STATUS_LTHRM	(1 << 0) /* local THERM limit tripped */
> +#define LM90_STATUS_RTHRM	(1 << 1) /* remote THERM limit tripped */
> +#define LM90_STATUS_OPEN	(1 << 2) /* remote is an open circuit */
> +#define LM90_STATUS_RLOW	(1 << 3) /* remote low temp limit tripped */
> +#define LM90_STATUS_RHIGH	(1 << 4) /* remote high temp limit tripped */
> +#define LM90_STATUS_LLOW	(1 << 5) /* local low temp limit tripped */
> +#define LM90_STATUS_LHIGH	(1 << 6) /* local high temp limit tripped */
> +
> +#define MAX6696_STATUS2_RLOW	(1 << 3) /* remote2 low temp limit tripped */
> +#define MAX6696_STATUS2_RHIGH	(1 << 4) /* remote2 high temp limit tripped */
> +
>  /*
>   * Driver data (common to all clients)
>   */
> @@ -1391,6 +1403,36 @@ static void lm90_init_client(struct i2c_client *client)
>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>  }
>  
> +static bool lm90_is_tripped(struct i2c_client *client)
> +{
> +	struct lm90_data *data = i2c_get_clientdata(client);
> +	u8 status, status2 = 0;
> +
> +	lm90_read_reg(client, LM90_REG_R_STATUS, &status);
> +
> +	if (data->kind == max6696)
> +		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &status2);
> +
> +	if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0)
> +		return false;
> +
> +	if (status & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM))
> +		dev_warn(&client->dev,
> +			 "temp%d out of range, please check!\n", 1);
> +	if (status & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM))
> +		dev_warn(&client->dev,
> +			 "temp%d out of range, please check!\n", 2);
> +	if (status & LM90_STATUS_OPEN)
> +		dev_warn(&client->dev,
> +			 "temp%d diode open, please check!\n", 2);
> +
> +	if (status2 & (MAX6696_STATUS2_RLOW | MAX6696_STATUS2_RHIGH))
> +		dev_warn(&client->dev,
> +			 "temp%d out of range, please check!\n", 3);
> +
> +	return true;
> +}
> +
>  static int lm90_probe(struct i2c_client *client,
>  		      const struct i2c_device_id *id)
>  {
> @@ -1489,36 +1531,17 @@ static int lm90_remove(struct i2c_client *client)
>  
>  static void lm90_alert(struct i2c_client *client, unsigned int flag)
>  {
> -	struct lm90_data *data = i2c_get_clientdata(client);
> -	u8 config, alarms, alarms2 = 0;
> -
> -	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> -
> -	if (data->kind == max6696)
> -		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms2);
> -
> -	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);
> -
> +	if (lm90_is_tripped(client)) {

You are reading LM90_REG_R_STATUS here...

>  		/*
>  		 * Disable ALERT# output, because these chips don't implement
>  		 * SMBus alert correctly; they should only hold the alert line
>  		 * low briefly.
>  		 */
> +		struct lm90_data *data = i2c_get_clientdata(client);
> +		u8 config, alarms;
> +
> +		lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);

... and here again. I already complained about this in my previous
review of this patch, and you were supposed to address it, but you did
not. As a result I am still not happy with this patch and I can't apply
it, sorry.

> +
>  		if ((data->flags & LM90_HAVE_BROKEN_ALERT)
>  		 && (alarms & data->alert_alarms)) {
>  			dev_dbg(&client->dev, "Disabling ALERT#\n");
> @@ -1526,6 +1549,8 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
>  			i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
>  						  config | 0x80);
>  		}
> +	} else {
> +		dev_info(&client->dev, "Everything OK\n");
>  	}
>  }
>  

-- 
Jean Delvare

Re: [PATCH v4 1/3] hwmon: (lm90) Define status bits

[Guenter Roeck]

On Wed, Oct 30, 2013 at 04:41:13PM +0100, Jean Delvare wrote:
> Hi Wei,
> 
> On Wed, 7 Aug 2013 14:18:24 +0800, Wei Ni wrote:
> > Add bit defines for the status register. And add a function
> > lm90_is_tripped() which will read status register and return
> > tripped or not, then lm90_alert can call it directly, and in the
> > future the IRQ thread also can use it.
> > 
> > Signed-off-by: Wei Ni <wni@nvidia.com>
> > ---
> >  drivers/hwmon/lm90.c |   75 +++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 50 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index cdff742..1da2eff 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -179,6 +179,18 @@ 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_STATUS_LTHRM	(1 << 0) /* local THERM limit tripped */
> > +#define LM90_STATUS_RTHRM	(1 << 1) /* remote THERM limit tripped */
> > +#define LM90_STATUS_OPEN	(1 << 2) /* remote is an open circuit */
> > +#define LM90_STATUS_RLOW	(1 << 3) /* remote low temp limit tripped */
> > +#define LM90_STATUS_RHIGH	(1 << 4) /* remote high temp limit tripped */
> > +#define LM90_STATUS_LLOW	(1 << 5) /* local low temp limit tripped */
> > +#define LM90_STATUS_LHIGH	(1 << 6) /* local high temp limit tripped */
> > +
> > +#define MAX6696_STATUS2_RLOW	(1 << 3) /* remote2 low temp limit tripped */
> > +#define MAX6696_STATUS2_RHIGH	(1 << 4) /* remote2 high temp limit tripped */
> > +
> >  /*
> >   * Driver data (common to all clients)
> >   */
> > @@ -1391,6 +1403,36 @@ static void lm90_init_client(struct i2c_client *client)
> >  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> >  }
> >  
> > +static bool lm90_is_tripped(struct i2c_client *client)
> > +{
> > +	struct lm90_data *data = i2c_get_clientdata(client);
> > +	u8 status, status2 = 0;
> > +
> > +	lm90_read_reg(client, LM90_REG_R_STATUS, &status);
> > +
> > +	if (data->kind == max6696)
> > +		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &status2);
> > +
> > +	if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0)
> > +		return false;
> > +
> > +	if (status & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM))
> > +		dev_warn(&client->dev,
> > +			 "temp%d out of range, please check!\n", 1);
> > +	if (status & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM))
> > +		dev_warn(&client->dev,
> > +			 "temp%d out of range, please check!\n", 2);
> > +	if (status & LM90_STATUS_OPEN)
> > +		dev_warn(&client->dev,
> > +			 "temp%d diode open, please check!\n", 2);
> > +
> > +	if (status2 & (MAX6696_STATUS2_RLOW | MAX6696_STATUS2_RHIGH))
> > +		dev_warn(&client->dev,
> > +			 "temp%d out of range, please check!\n", 3);
> > +

I am also a bit concerned about the misleading function name. 
I would expect something like "is_tripped" to return true or false,
not to dump log messages to the console.

Guenter

> > +	return true;
> > +}
> > +
> >  static int lm90_probe(struct i2c_client *client,
> >  		      const struct i2c_device_id *id)
> >  {
> > @@ -1489,36 +1531,17 @@ static int lm90_remove(struct i2c_client *client)
> >  
> >  static void lm90_alert(struct i2c_client *client, unsigned int flag)
> >  {
> > -	struct lm90_data *data = i2c_get_clientdata(client);
> > -	u8 config, alarms, alarms2 = 0;
> > -
> > -	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> > -
> > -	if (data->kind == max6696)
> > -		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms2);
> > -
> > -	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);
> > -
> > +	if (lm90_is_tripped(client)) {
> 
> You are reading LM90_REG_R_STATUS here...
> 
> >  		/*
> >  		 * Disable ALERT# output, because these chips don't implement
> >  		 * SMBus alert correctly; they should only hold the alert line
> >  		 * low briefly.
> >  		 */
> > +		struct lm90_data *data = i2c_get_clientdata(client);
> > +		u8 config, alarms;
> > +
> > +		lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> 
> ... and here again. I already complained about this in my previous
> review of this patch, and you were supposed to address it, but you did
> not. As a result I am still not happy with this patch and I can't apply
> it, sorry.
> 
> > +
> >  		if ((data->flags & LM90_HAVE_BROKEN_ALERT)
> >  		 && (alarms & data->alert_alarms)) {
> >  			dev_dbg(&client->dev, "Disabling ALERT#\n");
> > @@ -1526,6 +1549,8 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
> >  			i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> >  						  config | 0x80);
> >  		}
> > +	} else {
> > +		dev_info(&client->dev, "Everything OK\n");
> >  	}
> >  }
> >  
> 
> -- 
> Jean Delvare
> 

Re: [PATCH v4 1/3] hwmon: (lm90) Define status bits

[Wei Ni]

On 10/30/2013 11:41 PM, Jean Delvare wrote:
> Hi Wei,
> 
> On Wed, 7 Aug 2013 14:18:24 +0800, Wei Ni wrote:
>> Add bit defines for the status register. And add a function
>> lm90_is_tripped() which will read status register and return
>> tripped or not, then lm90_alert can call it directly, and in the
>> future the IRQ thread also can use it.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  drivers/hwmon/lm90.c |   75 +++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 50 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index cdff742..1da2eff 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -179,6 +179,18 @@ 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_STATUS_LTHRM	(1 << 0) /* local THERM limit tripped */
>> +#define LM90_STATUS_RTHRM	(1 << 1) /* remote THERM limit tripped */
>> +#define LM90_STATUS_OPEN	(1 << 2) /* remote is an open circuit */
>> +#define LM90_STATUS_RLOW	(1 << 3) /* remote low temp limit tripped */
>> +#define LM90_STATUS_RHIGH	(1 << 4) /* remote high temp limit tripped */
>> +#define LM90_STATUS_LLOW	(1 << 5) /* local low temp limit tripped */
>> +#define LM90_STATUS_LHIGH	(1 << 6) /* local high temp limit tripped */
>> +
>> +#define MAX6696_STATUS2_RLOW	(1 << 3) /* remote2 low temp limit tripped */
>> +#define MAX6696_STATUS2_RHIGH	(1 << 4) /* remote2 high temp limit tripped */
>> +
>>  /*
>>   * Driver data (common to all clients)
>>   */
>> @@ -1391,6 +1403,36 @@ static void lm90_init_client(struct i2c_client *client)
>>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>>  }
>>  
>> +static bool lm90_is_tripped(struct i2c_client *client)
>> +{
>> +	struct lm90_data *data = i2c_get_clientdata(client);
>> +	u8 status, status2 = 0;
>> +
>> +	lm90_read_reg(client, LM90_REG_R_STATUS, &status);
>> +
>> +	if (data->kind == max6696)
>> +		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &status2);
>> +
>> +	if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0)
>> +		return false;
>> +
>> +	if (status & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM))
>> +		dev_warn(&client->dev,
>> +			 "temp%d out of range, please check!\n", 1);
>> +	if (status & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM))
>> +		dev_warn(&client->dev,
>> +			 "temp%d out of range, please check!\n", 2);
>> +	if (status & LM90_STATUS_OPEN)
>> +		dev_warn(&client->dev,
>> +			 "temp%d diode open, please check!\n", 2);
>> +
>> +	if (status2 & (MAX6696_STATUS2_RLOW | MAX6696_STATUS2_RHIGH))
>> +		dev_warn(&client->dev,
>> +			 "temp%d out of range, please check!\n", 3);
>> +
>> +	return true;
>> +}
>> +
>>  static int lm90_probe(struct i2c_client *client,
>>  		      const struct i2c_device_id *id)
>>  {
>> @@ -1489,36 +1531,17 @@ static int lm90_remove(struct i2c_client *client)
>>  
>>  static void lm90_alert(struct i2c_client *client, unsigned int flag)
>>  {
>> -	struct lm90_data *data = i2c_get_clientdata(client);
>> -	u8 config, alarms, alarms2 = 0;
>> -
>> -	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
>> -
>> -	if (data->kind == max6696)
>> -		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms2);
>> -
>> -	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);
>> -
>> +	if (lm90_is_tripped(client)) {
> 
> You are reading LM90_REG_R_STATUS here...
> 
>>  		/*
>>  		 * Disable ALERT# output, because these chips don't implement
>>  		 * SMBus alert correctly; they should only hold the alert line
>>  		 * low briefly.
>>  		 */
>> +		struct lm90_data *data = i2c_get_clientdata(client);
>> +		u8 config, alarms;
>> +
>> +		lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> 
> ... and here again. I already complained about this in my previous
> review of this patch, and you were supposed to address it, but you did
> not. As a result I am still not happy with this patch and I can't apply
> it, sorry.

It's so sorry, I made a mistake, I sent a old patch for this. I will
sent the right one right now.
Sorry again.

Wei.

> 
>> +
>>  		if ((data->flags & LM90_HAVE_BROKEN_ALERT)
>>  		 && (alarms & data->alert_alarms)) {
>>  			dev_dbg(&client->dev, "Disabling ALERT#\n");
>> @@ -1526,6 +1549,8 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
>>  			i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
>>  						  config | 0x80);
>>  		}
>> +	} else {
>> +		dev_info(&client->dev, "Everything OK\n");
>>  	}
>>  }
>>  
> 

Re: [PATCH v4 1/3] hwmon: (lm90) Define status bits

[Wei Ni]

On 10/31/2013 10:47 AM, Wei Ni wrote:
> On 10/30/2013 11:41 PM, Jean Delvare wrote:
>> Hi Wei,
>>
>> On Wed, 7 Aug 2013 14:18:24 +0800, Wei Ni wrote:
>>> Add bit defines for the status register. And add a function
>>> lm90_is_tripped() which will read status register and return
>>> tripped or not, then lm90_alert can call it directly, and in the
>>> future the IRQ thread also can use it.
>>>
>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>> ---
>>>  drivers/hwmon/lm90.c |   75 +++++++++++++++++++++++++++++++++-----------------
>>>  1 file changed, 50 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>> index cdff742..1da2eff 100644
>>> --- a/drivers/hwmon/lm90.c
>>> +++ b/drivers/hwmon/lm90.c
>>> @@ -179,6 +179,18 @@ 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_STATUS_LTHRM	(1 << 0) /* local THERM limit tripped */
>>> +#define LM90_STATUS_RTHRM	(1 << 1) /* remote THERM limit tripped */
>>> +#define LM90_STATUS_OPEN	(1 << 2) /* remote is an open circuit */
>>> +#define LM90_STATUS_RLOW	(1 << 3) /* remote low temp limit tripped */
>>> +#define LM90_STATUS_RHIGH	(1 << 4) /* remote high temp limit tripped */
>>> +#define LM90_STATUS_LLOW	(1 << 5) /* local low temp limit tripped */
>>> +#define LM90_STATUS_LHIGH	(1 << 6) /* local high temp limit tripped */
>>> +
>>> +#define MAX6696_STATUS2_RLOW	(1 << 3) /* remote2 low temp limit tripped */
>>> +#define MAX6696_STATUS2_RHIGH	(1 << 4) /* remote2 high temp limit tripped */
>>> +
>>>  /*
>>>   * Driver data (common to all clients)
>>>   */
>>> @@ -1391,6 +1403,36 @@ static void lm90_init_client(struct i2c_client *client)
>>>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>>>  }
>>>  
>>> +static bool lm90_is_tripped(struct i2c_client *client)
>>> +{
>>> +	struct lm90_data *data = i2c_get_clientdata(client);
>>> +	u8 status, status2 = 0;
>>> +
>>> +	lm90_read_reg(client, LM90_REG_R_STATUS, &status);
>>> +
>>> +	if (data->kind == max6696)
>>> +		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &status2);
>>> +
>>> +	if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0)
>>> +		return false;
>>> +
>>> +	if (status & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM))
>>> +		dev_warn(&client->dev,
>>> +			 "temp%d out of range, please check!\n", 1);
>>> +	if (status & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM))
>>> +		dev_warn(&client->dev,
>>> +			 "temp%d out of range, please check!\n", 2);
>>> +	if (status & LM90_STATUS_OPEN)
>>> +		dev_warn(&client->dev,
>>> +			 "temp%d diode open, please check!\n", 2);
>>> +
>>> +	if (status2 & (MAX6696_STATUS2_RLOW | MAX6696_STATUS2_RHIGH))
>>> +		dev_warn(&client->dev,
>>> +			 "temp%d out of range, please check!\n", 3);
>>> +
>>> +	return true;
>>> +}
>>> +
>>>  static int lm90_probe(struct i2c_client *client,
>>>  		      const struct i2c_device_id *id)
>>>  {
>>> @@ -1489,36 +1531,17 @@ static int lm90_remove(struct i2c_client *client)
>>>  
>>>  static void lm90_alert(struct i2c_client *client, unsigned int flag)
>>>  {
>>> -	struct lm90_data *data = i2c_get_clientdata(client);
>>> -	u8 config, alarms, alarms2 = 0;
>>> -
>>> -	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
>>> -
>>> -	if (data->kind == max6696)
>>> -		lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms2);
>>> -
>>> -	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);
>>> -
>>> +	if (lm90_is_tripped(client)) {
>>
>> You are reading LM90_REG_R_STATUS here...
>>
>>>  		/*
>>>  		 * Disable ALERT# output, because these chips don't implement
>>>  		 * SMBus alert correctly; they should only hold the alert line
>>>  		 * low briefly.
>>>  		 */
>>> +		struct lm90_data *data = i2c_get_clientdata(client);
>>> +		u8 config, alarms;
>>> +
>>> +		lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
>>
>> ... and here again. I already complained about this in my previous
>> review of this patch, and you were supposed to address it, but you did
>> not. As a result I am still not happy with this patch and I can't apply
>> it, sorry.
> 
> It's so sorry, I made a mistake, I sent a old patch for this. I will
> sent the right one right now.
> Sorry again.

Hi, Jean

I resend this patch:
[PATCH RESEND v4 1/3] hwmon: (lm90) Define status bits
Please review it.

Thanks.
Wei.

> 
> Wei.
> 
>>
>>> +
>>>  		if ((data->flags & LM90_HAVE_BROKEN_ALERT)
>>>  		 && (alarms & data->alert_alarms)) {
>>>  			dev_dbg(&client->dev, "Disabling ALERT#\n");
>>> @@ -1526,6 +1549,8 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
>>>  			i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
>>>  						  config | 0x80);
>>>  		}
>>> +	} else {
>>> +		dev_info(&client->dev, "Everything OK\n");
>>>  	}
>>>  }
>>>  
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH v4 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 |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 1da2eff..2e68773 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
@@ -1433,6 +1434,16 @@ static bool lm90_is_tripped(struct i2c_client *client)
 	return true;
 }
 
+static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
+{
+	struct i2c_client *client = dev_id;
+
+	if (lm90_is_tripped(client))
+		return IRQ_HANDLED;
+	else
+		return IRQ_NONE;
+}
+
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id)
 {
@@ -1509,6 +1520,18 @@ static int lm90_probe(struct i2c_client *client,
 		goto exit_remove_files;
 	}
 
+	if (client->irq) {
+		dev_dbg(dev, "IRQ: %d\n", client->irq);
+		err = devm_request_threaded_irq(dev, client->irq,
+						NULL, lm90_irq_thread,
+						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+						"lm90", client);
+		if (err < 0) {
+			dev_err(dev, "cannot request IRQ: %d\n", client->irq);
+			goto exit_remove_files;
+		}
+	}
+
 	return 0;
 
 exit_remove_files:
-- 
1.7.9.5

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

[Jean Delvare]

On Wed, 7 Aug 2013 14:18:25 +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 |   23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> (...)

This one looks good now, thanks, with just one minor change I'd do
before committing:

> +			dev_err(dev, "cannot request IRQ: %d\n", client->irq);

The ":" isn't really needed, I'll remove it.

-- 
Jean Delvare

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

[Jean Delvare]

Hi Wei,

I'm hitting a problem with that patch now...

On Wed, 7 Aug 2013 14:18:25 +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 |   23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 1da2eff..2e68773 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
> @@ -1433,6 +1434,16 @@ static bool lm90_is_tripped(struct i2c_client *client)
>  	return true;
>  }
>  
> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
> +{
> +	struct i2c_client *client = dev_id;
> +
> +	if (lm90_is_tripped(client))

Did you really send the latest version of the patch? lm90_is_tripped()
takes two arguments now, but here you only pass one.

If you have a more up-to-date version of this patch, please send it
over quickly.

> +		return IRQ_HANDLED;
> +	else
> +		return IRQ_NONE;
> +}
> +
>  static int lm90_probe(struct i2c_client *client,
>  		      const struct i2c_device_id *id)
>  {
> @@ -1509,6 +1520,18 @@ static int lm90_probe(struct i2c_client *client,
>  		goto exit_remove_files;
>  	}
>  
> +	if (client->irq) {
> +		dev_dbg(dev, "IRQ: %d\n", client->irq);
> +		err = devm_request_threaded_irq(dev, client->irq,
> +						NULL, lm90_irq_thread,
> +						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +						"lm90", client);
> +		if (err < 0) {
> +			dev_err(dev, "cannot request IRQ: %d\n", client->irq);
> +			goto exit_remove_files;
> +		}
> +	}
> +
>  	return 0;
>  
>  exit_remove_files:


-- 
Jean Delvare

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

[Wei Ni]

On 11/04/2013 05:34 PM, Jean Delvare wrote:
> Hi Wei,
> 
> I'm hitting a problem with that patch now...
> 
> On Wed, 7 Aug 2013 14:18:25 +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 |   23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index 1da2eff..2e68773 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
>> @@ -1433,6 +1434,16 @@ static bool lm90_is_tripped(struct i2c_client *client)
>>  	return true;
>>  }
>>  
>> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
>> +{
>> +	struct i2c_client *client = dev_id;
>> +
>> +	if (lm90_is_tripped(client))
> 
> Did you really send the latest version of the patch? lm90_is_tripped()
> takes two arguments now, but here you only pass one.
> 
> If you have a more up-to-date version of this patch, please send it
> over quickly.

Hi, Jean
It's so sorry. As you know I resend the "[PATCH RESEND v4 1/3] hwmon:
(lm90) Define status bits" last week, but didn't rebase this patch. I
will rebase it and send it right now.

Wei.
Thanks.

> 
>> +		return IRQ_HANDLED;
>> +	else
>> +		return IRQ_NONE;
>> +}
>> +
>>  static int lm90_probe(struct i2c_client *client,
>>  		      const struct i2c_device_id *id)
>>  {
>> @@ -1509,6 +1520,18 @@ static int lm90_probe(struct i2c_client *client,
>>  		goto exit_remove_files;
>>  	}
>>  
>> +	if (client->irq) {
>> +		dev_dbg(dev, "IRQ: %d\n", client->irq);
>> +		err = devm_request_threaded_irq(dev, client->irq,
>> +						NULL, lm90_irq_thread,
>> +						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>> +						"lm90", client);
>> +		if (err < 0) {
>> +			dev_err(dev, "cannot request IRQ: %d\n", client->irq);
>> +			goto exit_remove_files;
>> +		}
>> +	}
>> +
>>  	return 0;
>>  
>>  exit_remove_files:
> 
> 

[PATCH v4 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 |  154 +++++++++++++++++++++++++++++---------------------
 1 file changed, 90 insertions(+), 64 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 2e68773..6cc3eef 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -309,6 +309,38 @@ static const struct lm90_params lm90_params[] = {
 };
 
 /*
+ * TEMP8 register index
+ */
+enum lm90_temp8_reg_index {
+	LOCAL_LOW = 0,
+	LOCAL_HIGH,
+	LOCAL_CRIT,
+	REMOTE_CRIT,
+	LOCAL_EMERG,	/* max6659 and max6695/96 */
+	REMOTE_EMERG,	/* max6659 and max6695/96 */
+	REMOTE2_CRIT,	/* max6695/96 only */
+	REMOTE2_EMERG,	/* max6695/96 only */
+	TEMP8_REG_NUM
+};
+
+/*
+ * TEMP11 register index
+ */
+enum lm90_temp11_reg_index {
+	REMOTE_TEMP = 0,
+	REMOTE_LOW,
+	REMOTE_HIGH,
+	REMOTE_OFFSET,	/* except max6646, max6657/58/59,
+			 *  and max6695/96
+			 */
+	LOCAL_TEMP,
+	REMOTE2_TEMP,	/* max6695/96 only */
+	REMOTE2_LOW,	/* max6695/96 only */
+	REMOTE2_HIGH,	/* max6695/96 only */
+	TEMP11_REG_NUM
+};
+
+/*
  * Client data (each client gets its own)
  */
 
@@ -330,25 +362,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) */
 };
@@ -490,37 +505,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[LOCAL_LOW]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH,
+			      &data->temp8[LOCAL_HIGH]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT,
+			      &data->temp8[LOCAL_CRIT]);
+		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
+			      &data->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[LOCAL_TEMP]);
 		} else {
 			if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
 					  &h) == 0)
-				data->temp11[4] = h << 8;
+				data->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[REMOTE_TEMP]);
 
 		if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
-			data->temp11[1] = h << 8;
+			data->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[REMOTE_LOW] |= l;
 		}
 		if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
-			data->temp11[2] = h << 8;
+			data->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[REMOTE_HIGH] |= l;
 		}
 
 		if (data->flags & LM90_HAVE_OFFSET) {
@@ -528,13 +548,13 @@ 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[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[LOCAL_EMERG]);
 			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
-				      &data->temp8[5]);
+				      &data->temp8[REMOTE_EMERG]);
 		}
 		lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
 		data->alarms = alarms;	/* save as 16 bit value */
@@ -542,15 +562,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[REMOTE2_CRIT]);
 			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
-				      &data->temp8[7]);
+				      &data->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[REMOTE2_TEMP]);
 			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h))
-				data->temp11[6] = h << 8;
+				data->temp11[REMOTE2_LOW] = h << 8;
 			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h))
-				data->temp11[7] = h << 8;
+				data->temp11[REMOTE2_HIGH] = h << 8;
 			lm90_select_remote_channel(client, data, 0);
 
 			if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2,
@@ -739,7 +760,7 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
 static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
 			 const char *buf, size_t count)
 {
-	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,
@@ -892,11 +913,11 @@ 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[LOCAL_CRIT]);
 	else if (data->kind == max6646)
-		temp = temp_from_u8(data->temp8[2]);
+		temp = temp_from_u8(data->temp8[LOCAL_CRIT]);
 	else
-		temp = temp_from_s8(data->temp8[2]);
+		temp = temp_from_s8(data->temp8[LOCAL_CRIT]);
 
 	data->temp_hyst = hyst_to_reg(temp - val);
 	i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST,
@@ -950,25 +971,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,
+	0, LOCAL_TEMP);
+static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL,
+	0, REMOTE_TEMP);
 static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 0);
+	set_temp8, LOCAL_LOW);
 static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 0, 1);
+	set_temp11, 0, REMOTE_LOW);
 static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 1);
+	set_temp8, LOCAL_HIGH);
 static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 1, 2);
+	set_temp11, 1, REMOTE_HIGH);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 2);
+	set_temp8, LOCAL_CRIT);
 static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 3);
+	set_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, LOCAL_CRIT);
+static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL,
+	REMOTE_CRIT);
 static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 2, 3);
+	set_temp11, 2, REMOTE_OFFSET);
 
 /* Individual alarm files */
 static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0);
@@ -1016,13 +1040,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, LOCAL_EMERG);
 static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 5);
+	set_temp8, REMOTE_EMERG);
 static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 4);
+			  NULL, LOCAL_EMERG);
 static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 5);
+			  NULL, REMOTE_EMERG);
 
 static struct attribute *lm90_emergency_attributes[] = {
 	&sensor_dev_attr_temp1_emergency.dev_attr.attr,
@@ -1052,18 +1076,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,
+	0, REMOTE2_TEMP);
 static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 3, 6);
+	set_temp11, 3, REMOTE2_LOW);
 static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 4, 7);
+	set_temp11, 4, 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, REMOTE2_CRIT);
+static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL,
+	REMOTE2_CRIT);
 static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 7);
+	set_temp8, REMOTE2_EMERG);
 static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 7);
+			  NULL, 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 v4 3/3] hwmon: (lm90) use enums for the indexes of temp8 and temp11

[Jean Delvare]

Hi Wei,

On Wed, 7 Aug 2013 14:18:26 +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>
> ---
>  drivers/hwmon/lm90.c |  154 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 90 insertions(+), 64 deletions(-)
> (...)

Looks much better except for one minor detail:

> +/*
> + * TEMP11 register index
> + */
> +enum lm90_temp11_reg_index {
> +	REMOTE_TEMP = 0,
> +	REMOTE_LOW,
> +	REMOTE_HIGH,
> +	REMOTE_OFFSET,	/* except max6646, max6657/58/59,
> +			 *  and max6695/96
> +			 */

This comment fits on a single line now.

> +	LOCAL_TEMP,
> +	REMOTE2_TEMP,	/* max6695/96 only */
> +	REMOTE2_LOW,	/* max6695/96 only */
> +	REMOTE2_HIGH,	/* max6695/96 only */
> +	TEMP11_REG_NUM
> +};

Other than that, it's OK, and I think I'll apply it after all.

-- 
Jean Delvare

Re: [PATCH v4 0/3] Lm90 Enhancements

[Wei Ni]

Hi, Jean
Do you have any more suggestions on this series?

Thanks.
Wei.

On 08/07/2013 02:18 PM, Wei Ni wrote:
> This patch set enhance the lm90 driver,
> it make the driver more readable and easier to use thermal framework.
> 
> This series is v4, previous version patches:
> [RFC]: http://thread.gmane.org/gmane.linux.power-management.general/31056
> [v1]: http://thread.gmane.org/gmane.linux.ports.tegra/11710/
> [v2]: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg465555.html
> [v3]: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg466772.html
> 
> Changes from v3:
> 1. remove the patch "hwmon: (lm90) split set&show temp as common codes",
> and sent it as a separated one.
> 2. fix the bug of second read on STATUS register.
> 3. fix some code style issue according to Jean's comments.
> 
> Changes from v2:
> 1. update the defines for status bit, and go into a separate patch.
> 2. introduce the new lm90_is_tripped() for lm90_irq_thread and lm90_alert.
> 
> 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) Define status bits
>   hwmon: (lm90) add support to handle IRQ
>   hwmon: (lm90) use enums for the indexes of temp8 and temp11
> 
>  drivers/hwmon/lm90.c |  252 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 163 insertions(+), 89 deletions(-)
> 

Re: [PATCH v4 0/3] Lm90 Enhancements

[Jean Delvare]

Hi Wei,

I am sorry, I see there have been many discussions about the lm90
driver while I was on vacation and these are threads I did not have the
time to catch up with yet. I'll read it all as soon as possible by my
current schedule is tight so please be patient!

Jean

On Mon, 9 Sep 2013 14:16:18 +0800, Wei Ni wrote:
> Hi, Jean
> Do you have any more suggestions on this series?
> 
> Thanks.
> Wei.
> 
> On 08/07/2013 02:18 PM, Wei Ni wrote:
> > This patch set enhance the lm90 driver,
> > it make the driver more readable and easier to use thermal framework.
> > 
> > This series is v4, previous version patches:
> > [RFC]: http://thread.gmane.org/gmane.linux.power-management.general/31056
> > [v1]: http://thread.gmane.org/gmane.linux.ports.tegra/11710/
> > [v2]: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg465555.html
> > [v3]: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg466772.html
> > 
> > Changes from v3:
> > 1. remove the patch "hwmon: (lm90) split set&show temp as common codes",
> > and sent it as a separated one.
> > 2. fix the bug of second read on STATUS register.
> > 3. fix some code style issue according to Jean's comments.
> > 
> > Changes from v2:
> > 1. update the defines for status bit, and go into a separate patch.
> > 2. introduce the new lm90_is_tripped() for lm90_irq_thread and lm90_alert.
> > 
> > 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) Define status bits
> >   hwmon: (lm90) add support to handle IRQ
> >   hwmon: (lm90) use enums for the indexes of temp8 and temp11
> > 
> >  drivers/hwmon/lm90.c |  252 ++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 163 insertions(+), 89 deletions(-)