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

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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

On Thu, Jul 04, 2013 at 03:57:49PM +0800, Wei Ni wrote:
> Add support to handle irq. When the temperature touch

Nit: This first sentence can be dropped. It merely repeats the subject.
And another nit: "irq" -> "IRQ"

> the limit value, the driver can handle the interrupt.
> 
> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/hwmon/lm90.c |   25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 2cb7f8e..fce9dfa 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
> @@ -1423,6 +1424,18 @@ static void lm90_init_client(struct i2c_client *client)
>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>  }
>  
> +static void lm90_alert(struct i2c_client *client, unsigned int flag);

Any reason why the implementation of lm90_alert() can't be moved here.
Granted, it'll make the diff larger but at least it'll allow us to get
rid of the forward declaration.

> +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);
> +
> +	lm90_alert(client, 0);
> +
> +	return IRQ_HANDLED;
> +}

Isn't this potentially dangerous, since if the IRQ was shared, IRQ
processing will always stop after this handler. Shouldn't you check
whether the device actually triggered an interrupt (by reading the
interrupt status register)?

> +
>  static int lm90_probe(struct i2c_client *client,
>  		      const struct i2c_device_id *id)
>  {
> @@ -1499,6 +1512,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);

Nit: "irq" -> "IRQ"

Thierry

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