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

[Thierry Reding]

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@xxxxxxxxxx>
> ---
>  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: signature.asc
Description: Digital signature