[PATCH v2 0/3] iio: light: opt3001: Enable operation w/o IRQ

[PATCH v2 0/3] iio: light: opt3001: Enable operation w/o IRQ

[Alexander Koch]

This patch series aims at enabling IRQ-less operation for the TI OPT3001 light
sensor.

The current version of the driver requires an interrupt line to be connected to
the INT pin of the sensor, through which the IIO framework gets notified about
readout values being ready. In the datasheet this is described as optional (sec.
8.1.1).

In my use case of the OPT3001 I do not have any interrupt lines or GPIOs
available to connect the INT pin to, so I have implemented an interrupt-less
operation mode that simply sleeps for the specified worst-case readout time
instead of waiting for the interrupt.

This change is transparent as interrupt-less operation mode is done only when no
valid interrupt no. is configured via device tree.

Patches are taken against linux-next/master, tested by compilation,
checkpatch.pl and by running on an embedded developer board (both IRQ-enabled
and IRQ-less mode).


Changes from v1:

 - use type bool for introduced member 'use_irq'
 - include trivial refactoring step that changes the types of two other members
   to bool, in order to match 'use_irq'


Alexander Koch (3):
  iio: light: opt3001: extract int. time constants
  iio: light: opt3001: trivial type refactoring
  iio: light: opt3001: enable operation w/o IRQ

 drivers/iio/light/opt3001.c | 156 ++++++++++++++++++++++++++++++--------------
 1 file changed, 106 insertions(+), 50 deletions(-)

-- 
2.7.0

[PATCH v2 1/3] iio: light: opt3001: extract int. time constants

[Alexander Koch]

Extract integration times as #define constants. This prepares using them
for delay/timeout length determination.

Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
Signed-off-by: Michael Hornung <mhornung.linux@gmail.com>
---
 drivers/iio/light/opt3001.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index 01e111e..aefbd79 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -65,6 +65,9 @@
 #define OPT3001_REG_EXPONENT(n)		((n) >> 12)
 #define OPT3001_REG_MANTISSA(n)		((n) & 0xfff)
 
+#define OPT3001_INT_TIME_LONG		800000
+#define OPT3001_INT_TIME_SHORT		100000
+
 /*
  * Time to wait for conversion result to be ready. The device datasheet
  * worst-case max value is 880ms. Add some slack to be on the safe side.
@@ -325,13 +328,13 @@ static int opt3001_set_int_time(struct opt3001 *opt, int time)
 	reg = ret;
 
 	switch (time) {
-	case 100000:
+	case OPT3001_INT_TIME_SHORT:
 		reg &= ~OPT3001_CONFIGURATION_CT;
-		opt->int_time = 100000;
+		opt->int_time = OPT3001_INT_TIME_SHORT;
 		break;
-	case 800000:
+	case OPT3001_INT_TIME_LONG:
 		reg |= OPT3001_CONFIGURATION_CT;
-		opt->int_time = 800000;
+		opt->int_time = OPT3001_INT_TIME_LONG;
 		break;
 	default:
 		return -EINVAL;
@@ -597,9 +600,9 @@ static int opt3001_configure(struct opt3001 *opt)
 
 	/* Reflect status of the device's integration time setting */
 	if (reg & OPT3001_CONFIGURATION_CT)
-		opt->int_time = 800000;
+		opt->int_time = OPT3001_INT_TIME_LONG;
 	else
-		opt->int_time = 100000;
+		opt->int_time = OPT3001_INT_TIME_SHORT;
 
 	/* Ensure device is in shutdown initially */
 	opt3001_set_mode(opt, &reg, OPT3001_CONFIGURATION_M_SHUTDOWN);
-- 
2.7.0

[PATCH v2 2/3] iio: light: opt3001: trivial type refactoring

[Alexander Koch]

Change variable type of struct opt3001 members 'ok_to_ignore_lock' and
'result_ready' uint16-bitfield of length one to bool.

They are used as bool, let the compiler do the optimization.

Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
Signed-off-by: Michael Hornung <mhornung.linux@gmail.com>
---
 drivers/iio/light/opt3001.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index aefbd79..b05c484 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -79,8 +79,8 @@ struct opt3001 {
 	struct device		*dev;
 
 	struct mutex		lock;
-	u16			ok_to_ignore_lock:1;
-	u16			result_ready:1;
+	bool			ok_to_ignore_lock;
+	bool			result_ready;
 	wait_queue_head_t	result_ready_queue;
 	u16			result;
 
-- 
2.7.0

[PATCH v2 3/3] iio: light: opt3001: enable operation w/o IRQ

[Alexander Koch]

Enable operation of the TI OPT3001 light sensor without having an
interrupt line available to connect the INT pin to.

In this operation mode, we issue a conversion request and simply wait
for the conversion time available as timeout value, determined from
integration time configuration and the worst-case time given in the data
sheet (sect. 6.5, table on p. 5):

  short integration time (100ms): 110ms + 3ms = 113ms
   long integration time (800ms): 880ms + 3ms = 883ms

This change is transparent as behaviour defaults to using the interrupt
method if an interrupt no. is configured via device tree. Interrupt-less
operation mode is performed when no valid interrupt no. is given.

Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
Signed-off-by: Michael Hornung <mhornung.linux@gmail.com>
---
 drivers/iio/light/opt3001.c | 137 ++++++++++++++++++++++++++++++--------------
 1 file changed, 95 insertions(+), 42 deletions(-)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index b05c484..2a2340d 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -70,9 +70,12 @@
 
 /*
  * Time to wait for conversion result to be ready. The device datasheet
- * worst-case max value is 880ms. Add some slack to be on the safe side.
+ * sect. 6.5 states results are ready after total integration time plus 3ms.
+ * This results in worst-case max values of 113ms or 883ms, respectively.
+ * Add some slack to be on the safe side.
  */
-#define OPT3001_RESULT_READY_TIMEOUT	msecs_to_jiffies(1000)
+#define OPT3001_RESULT_READY_SHORT	150
+#define OPT3001_RESULT_READY_LONG	1000
 
 struct opt3001 {
 	struct i2c_client	*client;
@@ -92,6 +95,8 @@ struct opt3001 {
 
 	u8			high_thresh_exp;
 	u8			low_thresh_exp;
+
+	bool			use_irq;
 };
 
 struct opt3001_scale {
@@ -230,26 +235,30 @@ static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2)
 	u16 reg;
 	u8 exponent;
 	u16 value;
+	long timeout;
 
-	/*
-	 * Enable the end-of-conversion interrupt mechanism. Note that doing
-	 * so will overwrite the low-level limit value however we will restore
-	 * this value later on.
-	 */
-	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_LOW_LIMIT,
-			OPT3001_LOW_LIMIT_EOC_ENABLE);
-	if (ret < 0) {
-		dev_err(opt->dev, "failed to write register %02x\n",
-				OPT3001_LOW_LIMIT);
-		return ret;
+	if (opt->use_irq) {
+		/*
+		 * Enable the end-of-conversion interrupt mechanism. Note that
+		 * doing so will overwrite the low-level limit value however we
+		 * will restore this value later on.
+		 */
+		ret = i2c_smbus_write_word_swapped(opt->client,
+					OPT3001_LOW_LIMIT,
+					OPT3001_LOW_LIMIT_EOC_ENABLE);
+		if (ret < 0) {
+			dev_err(opt->dev, "failed to write register %02x\n",
+					OPT3001_LOW_LIMIT);
+			return ret;
+		}
+
+		/* Allow IRQ to access the device despite lock being set */
+		opt->ok_to_ignore_lock = true;
 	}
 
-	/* Reset data-ready indicator flag (will be set in the IRQ routine) */
+	/* Reset data-ready indicator flag */
 	opt->result_ready = false;
 
-	/* Allow IRQ to access the device despite lock being set */
-	opt->ok_to_ignore_lock = true;
-
 	/* Configure for single-conversion mode and start a new conversion */
 	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
 	if (ret < 0) {
@@ -269,32 +278,69 @@ static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2)
 		goto err;
 	}
 
-	/* Wait for the IRQ to indicate the conversion is complete */
-	ret = wait_event_timeout(opt->result_ready_queue, opt->result_ready,
-			OPT3001_RESULT_READY_TIMEOUT);
+	if (opt->use_irq) {
+		/* Wait for the IRQ to indicate the conversion is complete */
+		ret = wait_event_timeout(opt->result_ready_queue,
+				opt->result_ready,
+				msecs_to_jiffies(OPT3001_RESULT_READY_LONG));
+	} else {
+		/* Sleep for result ready time */
+		timeout = (opt->int_time == OPT3001_INT_TIME_SHORT) ?
+			OPT3001_RESULT_READY_SHORT : OPT3001_RESULT_READY_LONG;
+		msleep(timeout);
+
+		/* Check result ready flag */
+		ret = i2c_smbus_read_word_swapped(opt->client,
+						  OPT3001_CONFIGURATION);
+		if (ret < 0) {
+			dev_err(opt->dev, "failed to read register %02x\n",
+				OPT3001_CONFIGURATION);
+			goto err;
+		}
+
+		if (!(ret & OPT3001_CONFIGURATION_CRF)) {
+			ret = -ETIMEDOUT;
+			goto err;
+		}
+
+		/* Obtain value */
+		ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_RESULT);
+		if (ret < 0) {
+			dev_err(opt->dev, "failed to read register %02x\n",
+				OPT3001_RESULT);
+			goto err;
+		}
+		opt->result = ret;
+		opt->result_ready = true;
+	}
 
 err:
-	/* Disallow IRQ to access the device while lock is active */
-	opt->ok_to_ignore_lock = false;
+	if (opt->use_irq)
+		/* Disallow IRQ to access the device while lock is active */
+		opt->ok_to_ignore_lock = false;
 
 	if (ret == 0)
 		return -ETIMEDOUT;
 	else if (ret < 0)
 		return ret;
 
-	/*
-	 * Disable the end-of-conversion interrupt mechanism by restoring the
-	 * low-level limit value (clearing OPT3001_LOW_LIMIT_EOC_ENABLE). Note
-	 * that selectively clearing those enable bits would affect the actual
-	 * limit value due to bit-overlap and therefore can't be done.
-	 */
-	value = (opt->low_thresh_exp << 12) | opt->low_thresh_mantissa;
-	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_LOW_LIMIT,
-			value);
-	if (ret < 0) {
-		dev_err(opt->dev, "failed to write register %02x\n",
-				OPT3001_LOW_LIMIT);
-		return ret;
+	if (opt->use_irq) {
+		/*
+		 * Disable the end-of-conversion interrupt mechanism by
+		 * restoring the low-level limit value (clearing
+		 * OPT3001_LOW_LIMIT_EOC_ENABLE). Note that selectively clearing
+		 * those enable bits would affect the actual limit value due to
+		 * bit-overlap and therefore can't be done.
+		 */
+		value = (opt->low_thresh_exp << 12) | opt->low_thresh_mantissa;
+		ret = i2c_smbus_write_word_swapped(opt->client,
+						   OPT3001_LOW_LIMIT,
+						   value);
+		if (ret < 0) {
+			dev_err(opt->dev, "failed to write register %02x\n",
+					OPT3001_LOW_LIMIT);
+			return ret;
+		}
 	}
 
 	exponent = OPT3001_REG_EXPONENT(opt->result);
@@ -736,12 +782,18 @@ static int opt3001_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	ret = request_threaded_irq(irq, NULL, opt3001_irq,
-			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-			"opt3001", iio);
-	if (ret) {
-		dev_err(dev, "failed to request IRQ #%d\n", irq);
-		return ret;
+	/* Make use of INT pin only if valid IRQ no. is given */
+	if (irq > 0) {
+		ret = request_threaded_irq(irq, NULL, opt3001_irq,
+				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+				"opt3001", iio);
+		if (ret) {
+			dev_err(dev, "failed to request IRQ #%d\n", irq);
+			return ret;
+		}
+		opt->use_irq = true;
+	} else {
+		dev_info(opt->dev, "enabling interrupt-less operation\n");
 	}
 
 	return 0;
@@ -754,7 +806,8 @@ static int opt3001_remove(struct i2c_client *client)
 	int ret;
 	u16 reg;
 
-	free_irq(client->irq, iio);
+	if (opt->use_irq)
+		free_irq(client->irq, iio);
 
 	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
 	if (ret < 0) {
-- 
2.7.0

Re: [PATCH v2 3/3] iio: light: opt3001: enable operation w/o IRQ

[Martin Kepplinger]

Am 2016-01-16 um 17:14 schrieb Alexander Koch:
> Enable operation of the TI OPT3001 light sensor without having an
> interrupt line available to connect the INT pin to.
> 
> In this operation mode, we issue a conversion request and simply wait
> for the conversion time available as timeout value, determined from
> integration time configuration and the worst-case time given in the data
> sheet (sect. 6.5, table on p. 5):
> 
>   short integration time (100ms): 110ms + 3ms = 113ms
>    long integration time (800ms): 880ms + 3ms = 883ms
> 
> This change is transparent as behaviour defaults to using the interrupt
> method if an interrupt no. is configured via device tree. Interrupt-less
> operation mode is performed when no valid interrupt no. is given.
> 
> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
> Signed-off-by: Michael Hornung <mhornung.linux@gmail.com>
> ---
>  drivers/iio/light/opt3001.c | 137 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 95 insertions(+), 42 deletions(-)
> 

Looks ok to me, although I didn't verify anything. Not very important
suggestions for changes inline:

> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index b05c484..2a2340d 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -70,9 +70,12 @@
>  
>  /*
>   * Time to wait for conversion result to be ready. The device datasheet
> - * worst-case max value is 880ms. Add some slack to be on the safe side.
> + * sect. 6.5 states results are ready after total integration time plus 3ms.
> + * This results in worst-case max values of 113ms or 883ms, respectively.
> + * Add some slack to be on the safe side.
>   */
> -#define OPT3001_RESULT_READY_TIMEOUT	msecs_to_jiffies(1000)
> +#define OPT3001_RESULT_READY_SHORT	150
> +#define OPT3001_RESULT_READY_LONG	1000
>  
>  struct opt3001 {
>  	struct i2c_client	*client;
> @@ -92,6 +95,8 @@ struct opt3001 {
>  
>  	u8			high_thresh_exp;
>  	u8			low_thresh_exp;
> +
> +	bool			use_irq;
>  };
>  
>  struct opt3001_scale {
> @@ -230,26 +235,30 @@ static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2)
>  	u16 reg;
>  	u8 exponent;
>  	u16 value;
> +	long timeout;
>  
> -	/*
> -	 * Enable the end-of-conversion interrupt mechanism. Note that doing
> -	 * so will overwrite the low-level limit value however we will restore
> -	 * this value later on.
> -	 */
> -	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_LOW_LIMIT,
> -			OPT3001_LOW_LIMIT_EOC_ENABLE);
> -	if (ret < 0) {
> -		dev_err(opt->dev, "failed to write register %02x\n",
> -				OPT3001_LOW_LIMIT);
> -		return ret;
> +	if (opt->use_irq) {
> +		/*
> +		 * Enable the end-of-conversion interrupt mechanism. Note that
> +		 * doing so will overwrite the low-level limit value however we
> +		 * will restore this value later on.
> +		 */
> +		ret = i2c_smbus_write_word_swapped(opt->client,
> +					OPT3001_LOW_LIMIT,
> +					OPT3001_LOW_LIMIT_EOC_ENABLE);
> +		if (ret < 0) {
> +			dev_err(opt->dev, "failed to write register %02x\n",
> +					OPT3001_LOW_LIMIT);
> +			return ret;
> +		}
> +
> +		/* Allow IRQ to access the device despite lock being set */
> +		opt->ok_to_ignore_lock = true;
>  	}
>  
> -	/* Reset data-ready indicator flag (will be set in the IRQ routine) */
> +	/* Reset data-ready indicator flag */
>  	opt->result_ready = false;
>  
> -	/* Allow IRQ to access the device despite lock being set */
> -	opt->ok_to_ignore_lock = true;
> -
>  	/* Configure for single-conversion mode and start a new conversion */
>  	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
>  	if (ret < 0) {
> @@ -269,32 +278,69 @@ static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2)
>  		goto err;
>  	}
>  
> -	/* Wait for the IRQ to indicate the conversion is complete */
> -	ret = wait_event_timeout(opt->result_ready_queue, opt->result_ready,
> -			OPT3001_RESULT_READY_TIMEOUT);
> +	if (opt->use_irq) {
> +		/* Wait for the IRQ to indicate the conversion is complete */
> +		ret = wait_event_timeout(opt->result_ready_queue,
> +				opt->result_ready,
> +				msecs_to_jiffies(OPT3001_RESULT_READY_LONG));
> +	} else {
> +		/* Sleep for result ready time */
> +		timeout = (opt->int_time == OPT3001_INT_TIME_SHORT) ?
> +			OPT3001_RESULT_READY_SHORT : OPT3001_RESULT_READY_LONG;
> +		msleep(timeout);
> +
> +		/* Check result ready flag */
> +		ret = i2c_smbus_read_word_swapped(opt->client,
> +						  OPT3001_CONFIGURATION);
> +		if (ret < 0) {
> +			dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_CONFIGURATION);
> +			goto err;
> +		}
> +
> +		if (!(ret & OPT3001_CONFIGURATION_CRF)) {
> +			ret = -ETIMEDOUT;
> +			goto err;
> +		}
> +
> +		/* Obtain value */
> +		ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_RESULT);
> +		if (ret < 0) {
> +			dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_RESULT);
> +			goto err;
> +		}
> +		opt->result = ret;
> +		opt->result_ready = true;
> +	}
>  
>  err:
> -	/* Disallow IRQ to access the device while lock is active */
> -	opt->ok_to_ignore_lock = false;
> +	if (opt->use_irq)
> +		/* Disallow IRQ to access the device while lock is active */
> +		opt->ok_to_ignore_lock = false;
>  
>  	if (ret == 0)
>  		return -ETIMEDOUT;
>  	else if (ret < 0)
>  		return ret;
>  
> -	/*
> -	 * Disable the end-of-conversion interrupt mechanism by restoring the
> -	 * low-level limit value (clearing OPT3001_LOW_LIMIT_EOC_ENABLE). Note
> -	 * that selectively clearing those enable bits would affect the actual
> -	 * limit value due to bit-overlap and therefore can't be done.
> -	 */
> -	value = (opt->low_thresh_exp << 12) | opt->low_thresh_mantissa;
> -	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_LOW_LIMIT,
> -			value);
> -	if (ret < 0) {
> -		dev_err(opt->dev, "failed to write register %02x\n",
> -				OPT3001_LOW_LIMIT);
> -		return ret;
> +	if (opt->use_irq) {
> +		/*
> +		 * Disable the end-of-conversion interrupt mechanism by
> +		 * restoring the low-level limit value (clearing
> +		 * OPT3001_LOW_LIMIT_EOC_ENABLE). Note that selectively clearing
> +		 * those enable bits would affect the actual limit value due to
> +		 * bit-overlap and therefore can't be done.
> +		 */
> +		value = (opt->low_thresh_exp << 12) | opt->low_thresh_mantissa;
> +		ret = i2c_smbus_write_word_swapped(opt->client,
> +						   OPT3001_LOW_LIMIT,
> +						   value);
> +		if (ret < 0) {
> +			dev_err(opt->dev, "failed to write register %02x\n",
> +					OPT3001_LOW_LIMIT);
> +			return ret;
> +		}
>  	}
>  
>  	exponent = OPT3001_REG_EXPONENT(opt->result);
> @@ -736,12 +782,18 @@ static int opt3001_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	ret = request_threaded_irq(irq, NULL, opt3001_irq,
> -			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -			"opt3001", iio);
> -	if (ret) {
> -		dev_err(dev, "failed to request IRQ #%d\n", irq);
> -		return ret;
> +	/* Make use of INT pin only if valid IRQ no. is given */
> +	if (irq > 0) {
> +		ret = request_threaded_irq(irq, NULL, opt3001_irq,
> +				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +				"opt3001", iio);

You can define your driver's name once and use it at least here and in
struct i2c_driver, and struct i2c_device_id.

> +		if (ret) {
> +			dev_err(dev, "failed to request IRQ #%d\n", irq);
> +			return ret;
> +		}
> +		opt->use_irq = true;
> +	} else {
> +		dev_info(opt->dev, "enabling interrupt-less operation\n");
>  	}

I'd probably make this debug level output.

>  
>  	return 0;
> @@ -754,7 +806,8 @@ static int opt3001_remove(struct i2c_client *client)
>  	int ret;
>  	u16 reg;
>  
> -	free_irq(client->irq, iio);
> +	if (opt->use_irq)
> +		free_irq(client->irq, iio);
>  
>  	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
>  	if (ret < 0) {
> 

Re: [PATCH v2 3/3] iio: light: opt3001: enable operation w/o IRQ

[Alexander Koch]

Am 18.01.2016 um 18:07 schrieb Martin Kepplinger:
> Am 2016-01-16 um 17:14 schrieb Alexander Koch:
>> Enable operation of the TI OPT3001 light sensor without having an
>> interrupt line available to connect the INT pin to.
>>
>> In this operation mode, we issue a conversion request and simply wait
>> for the conversion time available as timeout value, determined from
>> integration time configuration and the worst-case time given in the data
>> sheet (sect. 6.5, table on p. 5):
>>
>>   short integration time (100ms): 110ms + 3ms = 113ms
>>    long integration time (800ms): 880ms + 3ms = 883ms
>>
>> This change is transparent as behaviour defaults to using the interrupt
>> method if an interrupt no. is configured via device tree. Interrupt-less
>> operation mode is performed when no valid interrupt no. is given.
>>
>> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
>> Signed-off-by: Michael Hornung <mhornung.linux@gmail.com>
>> ---
>>  drivers/iio/light/opt3001.c | 137 ++++++++++++++++++++++++++++++--------------
>>  1 file changed, 95 insertions(+), 42 deletions(-)
>>
> 
> Looks ok to me, although I didn't verify anything. Not very important
> suggestions for changes inline

Thanks! Suggestions noted but if nobody objects I'd stick with v2 as I'm
hoping to have it accepted in the merge window for 4.5.


Regards

Alex

Re: [PATCH v2 3/3] iio: light: opt3001: enable operation w/o IRQ

[Jonathan Cameron]

On 23/01/16 18:55, Alexander Koch wrote:
> Am 18.01.2016 um 18:07 schrieb Martin Kepplinger:
>> Am 2016-01-16 um 17:14 schrieb Alexander Koch:
>>> Enable operation of the TI OPT3001 light sensor without having an
>>> interrupt line available to connect the INT pin to.
>>>
>>> In this operation mode, we issue a conversion request and simply wait
>>> for the conversion time available as timeout value, determined from
>>> integration time configuration and the worst-case time given in the data
>>> sheet (sect. 6.5, table on p. 5):
>>>
>>>   short integration time (100ms): 110ms + 3ms = 113ms
>>>    long integration time (800ms): 880ms + 3ms = 883ms
>>>
>>> This change is transparent as behaviour defaults to using the interrupt
>>> method if an interrupt no. is configured via device tree. Interrupt-less
>>> operation mode is performed when no valid interrupt no. is given.
>>>
>>> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
>>> Signed-off-by: Michael Hornung <mhornung.linux@gmail.com>
>>> ---
>>>  drivers/iio/light/opt3001.c | 137 ++++++++++++++++++++++++++++++--------------
>>>  1 file changed, 95 insertions(+), 42 deletions(-)
>>>
>>
>> Looks ok to me, although I didn't verify anything. Not very important
>> suggestions for changes inline
> 
> Thanks! Suggestions noted but if nobody objects I'd stick with v2 as I'm
> hoping to have it accepted in the merge window for 4.5.
Sorry Alex,

The IIO merge window for a given cycle closes at least a week before Linus
announces the opening of the main merge window.  This is partly due to the
path that IIO patches take to Linus (via Greg KH who operates a policy of
not taking anything after Linus starts hinting that the final RC for the
previous version is out) and partly as it's good general best practice
as it leaves some time for patches to sit in linux-next and get hammered
by the autobuilders, identifying any merge conflicts etc.

So this is firmly 4.6 material now anyway so we have lots of time to
dot the 'i's and cross the 't's!

(If it had been marginal I 'might' have enacted Martin's suggestions
as I applied the patch) but no point in taking the risk I might mess
it up so early in a cycle!

Jonathan
> 
> 
> Regards
> 
> Alex
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 1/3] iio: light: opt3001: extract int. time constants

[Jonathan Cameron]

On 16/01/16 16:14, Alexander Koch wrote:
> Extract integration times as #define constants. This prepares using them
> for delay/timeout length determination.
> 
> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
> Signed-off-by: Michael Hornung <mhornung.linux@gmail.com>
Applied to the togreg branch of iio.git - initially pushed out as testing
for the autobuilders to play with it.
> ---
>  drivers/iio/light/opt3001.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index 01e111e..aefbd79 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -65,6 +65,9 @@
>  #define OPT3001_REG_EXPONENT(n)		((n) >> 12)
>  #define OPT3001_REG_MANTISSA(n)		((n) & 0xfff)
>  
> +#define OPT3001_INT_TIME_LONG		800000
> +#define OPT3001_INT_TIME_SHORT		100000
> +
>  /*
>   * Time to wait for conversion result to be ready. The device datasheet
>   * worst-case max value is 880ms. Add some slack to be on the safe side.
> @@ -325,13 +328,13 @@ static int opt3001_set_int_time(struct opt3001 *opt, int time)
>  	reg = ret;
>  
>  	switch (time) {
> -	case 100000:
> +	case OPT3001_INT_TIME_SHORT:
>  		reg &= ~OPT3001_CONFIGURATION_CT;
> -		opt->int_time = 100000;
> +		opt->int_time = OPT3001_INT_TIME_SHORT;
>  		break;
> -	case 800000:
> +	case OPT3001_INT_TIME_LONG:
>  		reg |= OPT3001_CONFIGURATION_CT;
> -		opt->int_time = 800000;
> +		opt->int_time = OPT3001_INT_TIME_LONG;
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -597,9 +600,9 @@ static int opt3001_configure(struct opt3001 *opt)
>  
>  	/* Reflect status of the device's integration time setting */
>  	if (reg & OPT3001_CONFIGURATION_CT)
> -		opt->int_time = 800000;
> +		opt->int_time = OPT3001_INT_TIME_LONG;
>  	else
> -		opt->int_time = 100000;
> +		opt->int_time = OPT3001_INT_TIME_SHORT;
>  
>  	/* Ensure device is in shutdown initially */
>  	opt3001_set_mode(opt, &reg, OPT3001_CONFIGURATION_M_SHUTDOWN);
> 

Re: [PATCH v2 2/3] iio: light: opt3001: trivial type refactoring

[Jonathan Cameron]

On 16/01/16 16:14, Alexander Koch wrote:
> Change variable type of struct opt3001 members 'ok_to_ignore_lock' and
> 'result_ready' uint16-bitfield of length one to bool.
> 
> They are used as bool, let the compiler do the optimization.
> 
> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
> Signed-off-by: Michael Hornung <mhornung.linux@gmail.com>
I find it hard to care much about this one, but consistency is
always good and the bitfield never made much sense in here.

Applied to the togreg branch of iio.git - pushed out as testing... etc.

Thanks,

Jonathan
> ---
>  drivers/iio/light/opt3001.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index aefbd79..b05c484 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -79,8 +79,8 @@ struct opt3001 {
>  	struct device		*dev;
>  
>  	struct mutex		lock;
> -	u16			ok_to_ignore_lock:1;
> -	u16			result_ready:1;
> +	bool			ok_to_ignore_lock;
> +	bool			result_ready;
>  	wait_queue_head_t	result_ready_queue;
>  	u16			result;
>  
> 

Re: [PATCH v2 3/3] iio: light: opt3001: enable operation w/o IRQ

[Jonathan Cameron]

On 18/01/16 17:07, Martin Kepplinger wrote:
> Am 2016-01-16 um 17:14 schrieb Alexander Koch:
>> Enable operation of the TI OPT3001 light sensor without having an
>> interrupt line available to connect the INT pin to.
>>
>> In this operation mode, we issue a conversion request and simply wait
>> for the conversion time available as timeout value, determined from
>> integration time configuration and the worst-case time given in the data
>> sheet (sect. 6.5, table on p. 5):
>>
>>   short integration time (100ms): 110ms + 3ms = 113ms
>>    long integration time (800ms): 880ms + 3ms = 883ms
>>
>> This change is transparent as behaviour defaults to using the interrupt
>> method if an interrupt no. is configured via device tree. Interrupt-less
>> operation mode is performed when no valid interrupt no. is given.
>>
>> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
>> Signed-off-by: Michael Hornung <mhornung.linux@gmail.com>
>> ---
>>  drivers/iio/light/opt3001.c | 137 ++++++++++++++++++++++++++++++--------------
>>  1 file changed, 95 insertions(+), 42 deletions(-)
>>
> 
> Looks ok to me, although I didn't verify anything. Not very important
> suggestions for changes inline:
Having actually read this closer now than I had when sending previous email
(all of 2 mins ago)...

1) The change to having a single define for the name would be a worthwhile
   one but is orthogonal to this patch set so should be done separately anyway.
2) The dev_info/dev_dbg is so trivial a change I doubt I can mess it up so
   will do that myself whilst applying.

Whilst we are here, the parameter indenting in this driver does really match
with consensus on indenting for parameters, so if anyone is doing a tidy up
file, through checkpatch.pl --strict at it and clean those up as well!

Jonathan
> 
>> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
>> index b05c484..2a2340d 100644
>> --- a/drivers/iio/light/opt3001.c
>> +++ b/drivers/iio/light/opt3001.c
>> @@ -70,9 +70,12 @@
>>  
>>  /*
>>   * Time to wait for conversion result to be ready. The device datasheet
>> - * worst-case max value is 880ms. Add some slack to be on the safe side.
>> + * sect. 6.5 states results are ready after total integration time plus 3ms.
>> + * This results in worst-case max values of 113ms or 883ms, respectively.
>> + * Add some slack to be on the safe side.
>>   */
>> -#define OPT3001_RESULT_READY_TIMEOUT	msecs_to_jiffies(1000)
>> +#define OPT3001_RESULT_READY_SHORT	150
>> +#define OPT3001_RESULT_READY_LONG	1000
>>  
>>  struct opt3001 {
>>  	struct i2c_client	*client;
>> @@ -92,6 +95,8 @@ struct opt3001 {
>>  
>>  	u8			high_thresh_exp;
>>  	u8			low_thresh_exp;
>> +
>> +	bool			use_irq;
>>  };
>>  
>>  struct opt3001_scale {
>> @@ -230,26 +235,30 @@ static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2)
>>  	u16 reg;
>>  	u8 exponent;
>>  	u16 value;
>> +	long timeout;
>>  
>> -	/*
>> -	 * Enable the end-of-conversion interrupt mechanism. Note that doing
>> -	 * so will overwrite the low-level limit value however we will restore
>> -	 * this value later on.
>> -	 */
>> -	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_LOW_LIMIT,
>> -			OPT3001_LOW_LIMIT_EOC_ENABLE);
>> -	if (ret < 0) {
>> -		dev_err(opt->dev, "failed to write register %02x\n",
>> -				OPT3001_LOW_LIMIT);
>> -		return ret;
>> +	if (opt->use_irq) {
>> +		/*
>> +		 * Enable the end-of-conversion interrupt mechanism. Note that
>> +		 * doing so will overwrite the low-level limit value however we
>> +		 * will restore this value later on.
>> +		 */
>> +		ret = i2c_smbus_write_word_swapped(opt->client,
>> +					OPT3001_LOW_LIMIT,
>> +					OPT3001_LOW_LIMIT_EOC_ENABLE);
>> +		if (ret < 0) {
>> +			dev_err(opt->dev, "failed to write register %02x\n",
>> +					OPT3001_LOW_LIMIT);
>> +			return ret;
>> +		}
>> +
>> +		/* Allow IRQ to access the device despite lock being set */
>> +		opt->ok_to_ignore_lock = true;
>>  	}
>>  
>> -	/* Reset data-ready indicator flag (will be set in the IRQ routine) */
>> +	/* Reset data-ready indicator flag */
>>  	opt->result_ready = false;
>>  
>> -	/* Allow IRQ to access the device despite lock being set */
>> -	opt->ok_to_ignore_lock = true;
>> -
>>  	/* Configure for single-conversion mode and start a new conversion */
>>  	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
>>  	if (ret < 0) {
>> @@ -269,32 +278,69 @@ static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2)
>>  		goto err;
>>  	}
>>  
>> -	/* Wait for the IRQ to indicate the conversion is complete */
>> -	ret = wait_event_timeout(opt->result_ready_queue, opt->result_ready,
>> -			OPT3001_RESULT_READY_TIMEOUT);
>> +	if (opt->use_irq) {
>> +		/* Wait for the IRQ to indicate the conversion is complete */
>> +		ret = wait_event_timeout(opt->result_ready_queue,
>> +				opt->result_ready,
>> +				msecs_to_jiffies(OPT3001_RESULT_READY_LONG));
>> +	} else {
>> +		/* Sleep for result ready time */
>> +		timeout = (opt->int_time == OPT3001_INT_TIME_SHORT) ?
>> +			OPT3001_RESULT_READY_SHORT : OPT3001_RESULT_READY_LONG;
>> +		msleep(timeout);
>> +
>> +		/* Check result ready flag */
>> +		ret = i2c_smbus_read_word_swapped(opt->client,
>> +						  OPT3001_CONFIGURATION);
>> +		if (ret < 0) {
>> +			dev_err(opt->dev, "failed to read register %02x\n",
>> +				OPT3001_CONFIGURATION);
>> +			goto err;
>> +		}
>> +
>> +		if (!(ret & OPT3001_CONFIGURATION_CRF)) {
>> +			ret = -ETIMEDOUT;
>> +			goto err;
>> +		}
>> +
>> +		/* Obtain value */
>> +		ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_RESULT);
>> +		if (ret < 0) {
>> +			dev_err(opt->dev, "failed to read register %02x\n",
>> +				OPT3001_RESULT);
>> +			goto err;
>> +		}
>> +		opt->result = ret;
>> +		opt->result_ready = true;
>> +	}
>>  
>>  err:
>> -	/* Disallow IRQ to access the device while lock is active */
>> -	opt->ok_to_ignore_lock = false;
>> +	if (opt->use_irq)
>> +		/* Disallow IRQ to access the device while lock is active */
>> +		opt->ok_to_ignore_lock = false;
>>  
>>  	if (ret == 0)
>>  		return -ETIMEDOUT;
>>  	else if (ret < 0)
>>  		return ret;
>>  
>> -	/*
>> -	 * Disable the end-of-conversion interrupt mechanism by restoring the
>> -	 * low-level limit value (clearing OPT3001_LOW_LIMIT_EOC_ENABLE). Note
>> -	 * that selectively clearing those enable bits would affect the actual
>> -	 * limit value due to bit-overlap and therefore can't be done.
>> -	 */
>> -	value = (opt->low_thresh_exp << 12) | opt->low_thresh_mantissa;
>> -	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_LOW_LIMIT,
>> -			value);
>> -	if (ret < 0) {
>> -		dev_err(opt->dev, "failed to write register %02x\n",
>> -				OPT3001_LOW_LIMIT);
>> -		return ret;
>> +	if (opt->use_irq) {
>> +		/*
>> +		 * Disable the end-of-conversion interrupt mechanism by
>> +		 * restoring the low-level limit value (clearing
>> +		 * OPT3001_LOW_LIMIT_EOC_ENABLE). Note that selectively clearing
>> +		 * those enable bits would affect the actual limit value due to
>> +		 * bit-overlap and therefore can't be done.
>> +		 */
>> +		value = (opt->low_thresh_exp << 12) | opt->low_thresh_mantissa;
>> +		ret = i2c_smbus_write_word_swapped(opt->client,
>> +						   OPT3001_LOW_LIMIT,
>> +						   value);
>> +		if (ret < 0) {
>> +			dev_err(opt->dev, "failed to write register %02x\n",
>> +					OPT3001_LOW_LIMIT);
>> +			return ret;
>> +		}
>>  	}
>>  
>>  	exponent = OPT3001_REG_EXPONENT(opt->result);
>> @@ -736,12 +782,18 @@ static int opt3001_probe(struct i2c_client *client,
>>  		return ret;
>>  	}
>>  
>> -	ret = request_threaded_irq(irq, NULL, opt3001_irq,
>> -			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> -			"opt3001", iio);
>> -	if (ret) {
>> -		dev_err(dev, "failed to request IRQ #%d\n", irq);
>> -		return ret;
>> +	/* Make use of INT pin only if valid IRQ no. is given */
>> +	if (irq > 0) {
>> +		ret = request_threaded_irq(irq, NULL, opt3001_irq,
>> +				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> +				"opt3001", iio);
> 
> You can define your driver's name once and use it at least here and in
> struct i2c_driver, and struct i2c_device_id.
Whilst a fair comment, it's a change that should actually be done in a
separate patch as it's not part of the functionality added here.
(this code is just indented, so ends up in the diff)
> 
>> +		if (ret) {
>> +			dev_err(dev, "failed to request IRQ #%d\n", irq);
>> +			return ret;
>> +		}
>> +		opt->use_irq = true;
>> +	} else {
>> +		dev_info(opt->dev, "enabling interrupt-less operation\n");
>>  	}
> 
> I'd probably make this debug level output.
> 
>>  
>>  	return 0;
>> @@ -754,7 +806,8 @@ static int opt3001_remove(struct i2c_client *client)
>>  	int ret;
>>  	u16 reg;
>>  
>> -	free_irq(client->irq, iio);
>> +	if (opt->use_irq)
>> +		free_irq(client->irq, iio);
>>  
>>  	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
>>  	if (ret < 0) {
>>
> 

Re: [PATCH v2 1/3] iio: light: opt3001: extract int. time constants

[Andreas Dannenberg]

On Sat, Jan 16, 2016 at 05:14:36PM +0100, Alexander Koch wrote:
> Extract integration times as #define constants. This prepares using them
> for delay/timeout length determination.
> 
> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
> Signed-off-by: Michael Hornung <mhornung.linux@gmail.com>

Tested-by: Andreas Dannenberg <dannenberg@ti.com>

Thanks for the patch series!

> ---
>  drivers/iio/light/opt3001.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index 01e111e..aefbd79 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -65,6 +65,9 @@
>  #define OPT3001_REG_EXPONENT(n)		((n) >> 12)
>  #define OPT3001_REG_MANTISSA(n)		((n) & 0xfff)
>  
> +#define OPT3001_INT_TIME_LONG		800000
> +#define OPT3001_INT_TIME_SHORT		100000
> +
>  /*
>   * Time to wait for conversion result to be ready. The device datasheet
>   * worst-case max value is 880ms. Add some slack to be on the safe side.
> @@ -325,13 +328,13 @@ static int opt3001_set_int_time(struct opt3001 *opt, int time)
>  	reg = ret;
>  
>  	switch (time) {
> -	case 100000:
> +	case OPT3001_INT_TIME_SHORT:
>  		reg &= ~OPT3001_CONFIGURATION_CT;
> -		opt->int_time = 100000;
> +		opt->int_time = OPT3001_INT_TIME_SHORT;
>  		break;
> -	case 800000:
> +	case OPT3001_INT_TIME_LONG:
>  		reg |= OPT3001_CONFIGURATION_CT;
> -		opt->int_time = 800000;
> +		opt->int_time = OPT3001_INT_TIME_LONG;
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -597,9 +600,9 @@ static int opt3001_configure(struct opt3001 *opt)
>  
>  	/* Reflect status of the device's integration time setting */
>  	if (reg & OPT3001_CONFIGURATION_CT)
> -		opt->int_time = 800000;
> +		opt->int_time = OPT3001_INT_TIME_LONG;
>  	else
> -		opt->int_time = 100000;
> +		opt->int_time = OPT3001_INT_TIME_SHORT;
>  
>  	/* Ensure device is in shutdown initially */
>  	opt3001_set_mode(opt, &reg, OPT3001_CONFIGURATION_M_SHUTDOWN);
> -- 
> 2.7.0
> 

Re: [PATCH v2 2/3] iio: light: opt3001: trivial type refactoring

[Andreas Dannenberg]

On Sat, Jan 16, 2016 at 05:14:37PM +0100, Alexander Koch wrote:
> Change variable type of struct opt3001 members 'ok_to_ignore_lock' and
> 'result_ready' uint16-bitfield of length one to bool.
> 
> They are used as bool, let the compiler do the optimization.
> 
> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
> Signed-off-by: Michael Hornung <mhornung.linux@gmail.com>

Tested-by: Andreas Dannenberg <dannenberg@ti.com>

> ---
>  drivers/iio/light/opt3001.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index aefbd79..b05c484 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -79,8 +79,8 @@ struct opt3001 {
>  	struct device		*dev;
>  
>  	struct mutex		lock;
> -	u16			ok_to_ignore_lock:1;
> -	u16			result_ready:1;
> +	bool			ok_to_ignore_lock;
> +	bool			result_ready;
>  	wait_queue_head_t	result_ready_queue;
>  	u16			result;
>  
> -- 
> 2.7.0
> 

Re: [PATCH v2 3/3] iio: light: opt3001: enable operation w/o IRQ

[Andreas Dannenberg]

On Sat, Jan 16, 2016 at 05:14:38PM +0100, Alexander Koch wrote:
> Enable operation of the TI OPT3001 light sensor without having an
> interrupt line available to connect the INT pin to.
> 
> In this operation mode, we issue a conversion request and simply wait
> for the conversion time available as timeout value, determined from
> integration time configuration and the worst-case time given in the data
> sheet (sect. 6.5, table on p. 5):
> 
>   short integration time (100ms): 110ms + 3ms = 113ms
>    long integration time (800ms): 880ms + 3ms = 883ms
> 
> This change is transparent as behaviour defaults to using the interrupt
> method if an interrupt no. is configured via device tree. Interrupt-less
> operation mode is performed when no valid interrupt no. is given.
> 
> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
> Signed-off-by: Michael Hornung <mhornung.linux@gmail.com>

Tested-by: Andreas Dannenberg <dannenberg@ti.com>

I tested both interrupt mode and non-interrupt mode using a BeagleBone
Black. Works like a champ. Also the hard-coded integration "wait" times
look good.


--
Andreas Dannenberg
Texas Instruments Inc


> ---
>  drivers/iio/light/opt3001.c | 137 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 95 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index b05c484..2a2340d 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -70,9 +70,12 @@
>  
>  /*
>   * Time to wait for conversion result to be ready. The device datasheet
> - * worst-case max value is 880ms. Add some slack to be on the safe side.
> + * sect. 6.5 states results are ready after total integration time plus 3ms.
> + * This results in worst-case max values of 113ms or 883ms, respectively.
> + * Add some slack to be on the safe side.
>   */
> -#define OPT3001_RESULT_READY_TIMEOUT	msecs_to_jiffies(1000)
> +#define OPT3001_RESULT_READY_SHORT	150
> +#define OPT3001_RESULT_READY_LONG	1000
>  
>  struct opt3001 {
>  	struct i2c_client	*client;
> @@ -92,6 +95,8 @@ struct opt3001 {
>  
>  	u8			high_thresh_exp;
>  	u8			low_thresh_exp;
> +
> +	bool			use_irq;
>  };
>  
>  struct opt3001_scale {
> @@ -230,26 +235,30 @@ static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2)
>  	u16 reg;
>  	u8 exponent;
>  	u16 value;
> +	long timeout;
>  
> -	/*
> -	 * Enable the end-of-conversion interrupt mechanism. Note that doing
> -	 * so will overwrite the low-level limit value however we will restore
> -	 * this value later on.
> -	 */
> -	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_LOW_LIMIT,
> -			OPT3001_LOW_LIMIT_EOC_ENABLE);
> -	if (ret < 0) {
> -		dev_err(opt->dev, "failed to write register %02x\n",
> -				OPT3001_LOW_LIMIT);
> -		return ret;
> +	if (opt->use_irq) {
> +		/*
> +		 * Enable the end-of-conversion interrupt mechanism. Note that
> +		 * doing so will overwrite the low-level limit value however we
> +		 * will restore this value later on.
> +		 */
> +		ret = i2c_smbus_write_word_swapped(opt->client,
> +					OPT3001_LOW_LIMIT,
> +					OPT3001_LOW_LIMIT_EOC_ENABLE);
> +		if (ret < 0) {
> +			dev_err(opt->dev, "failed to write register %02x\n",
> +					OPT3001_LOW_LIMIT);
> +			return ret;
> +		}
> +
> +		/* Allow IRQ to access the device despite lock being set */
> +		opt->ok_to_ignore_lock = true;
>  	}
>  
> -	/* Reset data-ready indicator flag (will be set in the IRQ routine) */
> +	/* Reset data-ready indicator flag */
>  	opt->result_ready = false;
>  
> -	/* Allow IRQ to access the device despite lock being set */
> -	opt->ok_to_ignore_lock = true;
> -
>  	/* Configure for single-conversion mode and start a new conversion */
>  	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
>  	if (ret < 0) {
> @@ -269,32 +278,69 @@ static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2)
>  		goto err;
>  	}
>  
> -	/* Wait for the IRQ to indicate the conversion is complete */
> -	ret = wait_event_timeout(opt->result_ready_queue, opt->result_ready,
> -			OPT3001_RESULT_READY_TIMEOUT);
> +	if (opt->use_irq) {
> +		/* Wait for the IRQ to indicate the conversion is complete */
> +		ret = wait_event_timeout(opt->result_ready_queue,
> +				opt->result_ready,
> +				msecs_to_jiffies(OPT3001_RESULT_READY_LONG));
> +	} else {
> +		/* Sleep for result ready time */
> +		timeout = (opt->int_time == OPT3001_INT_TIME_SHORT) ?
> +			OPT3001_RESULT_READY_SHORT : OPT3001_RESULT_READY_LONG;
> +		msleep(timeout);
> +
> +		/* Check result ready flag */
> +		ret = i2c_smbus_read_word_swapped(opt->client,
> +						  OPT3001_CONFIGURATION);
> +		if (ret < 0) {
> +			dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_CONFIGURATION);
> +			goto err;
> +		}
> +
> +		if (!(ret & OPT3001_CONFIGURATION_CRF)) {
> +			ret = -ETIMEDOUT;
> +			goto err;
> +		}
> +
> +		/* Obtain value */
> +		ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_RESULT);
> +		if (ret < 0) {
> +			dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_RESULT);
> +			goto err;
> +		}
> +		opt->result = ret;
> +		opt->result_ready = true;
> +	}
>  
>  err:
> -	/* Disallow IRQ to access the device while lock is active */
> -	opt->ok_to_ignore_lock = false;
> +	if (opt->use_irq)
> +		/* Disallow IRQ to access the device while lock is active */
> +		opt->ok_to_ignore_lock = false;
>  
>  	if (ret == 0)
>  		return -ETIMEDOUT;
>  	else if (ret < 0)
>  		return ret;
>  
> -	/*
> -	 * Disable the end-of-conversion interrupt mechanism by restoring the
> -	 * low-level limit value (clearing OPT3001_LOW_LIMIT_EOC_ENABLE). Note
> -	 * that selectively clearing those enable bits would affect the actual
> -	 * limit value due to bit-overlap and therefore can't be done.
> -	 */
> -	value = (opt->low_thresh_exp << 12) | opt->low_thresh_mantissa;
> -	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_LOW_LIMIT,
> -			value);
> -	if (ret < 0) {
> -		dev_err(opt->dev, "failed to write register %02x\n",
> -				OPT3001_LOW_LIMIT);
> -		return ret;
> +	if (opt->use_irq) {
> +		/*
> +		 * Disable the end-of-conversion interrupt mechanism by
> +		 * restoring the low-level limit value (clearing
> +		 * OPT3001_LOW_LIMIT_EOC_ENABLE). Note that selectively clearing
> +		 * those enable bits would affect the actual limit value due to
> +		 * bit-overlap and therefore can't be done.
> +		 */
> +		value = (opt->low_thresh_exp << 12) | opt->low_thresh_mantissa;
> +		ret = i2c_smbus_write_word_swapped(opt->client,
> +						   OPT3001_LOW_LIMIT,
> +						   value);
> +		if (ret < 0) {
> +			dev_err(opt->dev, "failed to write register %02x\n",
> +					OPT3001_LOW_LIMIT);
> +			return ret;
> +		}
>  	}
>  
>  	exponent = OPT3001_REG_EXPONENT(opt->result);
> @@ -736,12 +782,18 @@ static int opt3001_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	ret = request_threaded_irq(irq, NULL, opt3001_irq,
> -			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -			"opt3001", iio);
> -	if (ret) {
> -		dev_err(dev, "failed to request IRQ #%d\n", irq);
> -		return ret;
> +	/* Make use of INT pin only if valid IRQ no. is given */
> +	if (irq > 0) {
> +		ret = request_threaded_irq(irq, NULL, opt3001_irq,
> +				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +				"opt3001", iio);
> +		if (ret) {
> +			dev_err(dev, "failed to request IRQ #%d\n", irq);
> +			return ret;
> +		}
> +		opt->use_irq = true;
> +	} else {
> +		dev_info(opt->dev, "enabling interrupt-less operation\n");
>  	}
>  
>  	return 0;
> @@ -754,7 +806,8 @@ static int opt3001_remove(struct i2c_client *client)
>  	int ret;
>  	u16 reg;
>  
> -	free_irq(client->irq, iio);
> +	if (opt->use_irq)
> +		free_irq(client->irq, iio);
>  
>  	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
>  	if (ret < 0) {
> -- 
> 2.7.0
> 

Re: [PATCH v2 0/3] iio: light: opt3001: Enable operation w/o IRQ

[Andreas Dannenberg]

On Sat, Jan 16, 2016 at 05:14:35PM +0100, Alexander Koch wrote:
> This patch series aims at enabling IRQ-less operation for the TI OPT3001 light
> sensor.

Jon,
I was checking to see if the DT bindings have been updated to reflect
the interrupt-less operation however I can't seem to find it in the
Kernel tree. The last activity I can find is this one...

https://patchwork.ozlabs.org/patch/490819/

I guess it may have slipped through the cracks so I'm going to update
the bindings document and re-submit.

Regards,
Andreas

> 
> The current version of the driver requires an interrupt line to be connected to
> the INT pin of the sensor, through which the IIO framework gets notified about
> readout values being ready. In the datasheet this is described as optional (sec.
> 8.1.1).
> 
> In my use case of the OPT3001 I do not have any interrupt lines or GPIOs
> available to connect the INT pin to, so I have implemented an interrupt-less
> operation mode that simply sleeps for the specified worst-case readout time
> instead of waiting for the interrupt.
> 
> This change is transparent as interrupt-less operation mode is done only when no
> valid interrupt no. is configured via device tree.
> 
> Patches are taken against linux-next/master, tested by compilation,
> checkpatch.pl and by running on an embedded developer board (both IRQ-enabled
> and IRQ-less mode).
> 
> 
> Changes from v1:
> 
>  - use type bool for introduced member 'use_irq'
>  - include trivial refactoring step that changes the types of two other members
>    to bool, in order to match 'use_irq'
> 
> 
> Alexander Koch (3):
>   iio: light: opt3001: extract int. time constants
>   iio: light: opt3001: trivial type refactoring
>   iio: light: opt3001: enable operation w/o IRQ
> 
>  drivers/iio/light/opt3001.c | 156 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 106 insertions(+), 50 deletions(-)
> 
> -- 
> 2.7.0
> 

Re: [PATCH v2 3/3] iio: light: opt3001: enable operation w/o IRQ

[Jonathan Cameron]

On 29/01/16 18:01, Andreas Dannenberg wrote:
> On Sat, Jan 16, 2016 at 05:14:38PM +0100, Alexander Koch wrote:
>> Enable operation of the TI OPT3001 light sensor without having an
>> interrupt line available to connect the INT pin to.
>>
>> In this operation mode, we issue a conversion request and simply wait
>> for the conversion time available as timeout value, determined from
>> integration time configuration and the worst-case time given in the data
>> sheet (sect. 6.5, table on p. 5):
>>
>>   short integration time (100ms): 110ms + 3ms = 113ms
>>    long integration time (800ms): 880ms + 3ms = 883ms
>>
>> This change is transparent as behaviour defaults to using the interrupt
>> method if an interrupt no. is configured via device tree. Interrupt-less
>> operation mode is performed when no valid interrupt no. is given.
>>
>> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
>> Signed-off-by: Michael Hornung <mhornung.linux@gmail.com>
> 
> Tested-by: Andreas Dannenberg <dannenberg@ti.com>
> 
> I tested both interrupt mode and non-interrupt mode using a BeagleBone
> Black. Works like a champ. Also the hard-coded integration "wait" times
> look good.
> 
Thanks, I have added your tested-by to the patches.

Jonathan
> 
> --
> Andreas Dannenberg
> Texas Instruments Inc
> 
> 
>> ---
>>  drivers/iio/light/opt3001.c | 137 ++++++++++++++++++++++++++++++--------------
>>  1 file changed, 95 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
>> index b05c484..2a2340d 100644
>> --- a/drivers/iio/light/opt3001.c
>> +++ b/drivers/iio/light/opt3001.c
>> @@ -70,9 +70,12 @@
>>  
>>  /*
>>   * Time to wait for conversion result to be ready. The device datasheet
>> - * worst-case max value is 880ms. Add some slack to be on the safe side.
>> + * sect. 6.5 states results are ready after total integration time plus 3ms.
>> + * This results in worst-case max values of 113ms or 883ms, respectively.
>> + * Add some slack to be on the safe side.
>>   */
>> -#define OPT3001_RESULT_READY_TIMEOUT	msecs_to_jiffies(1000)
>> +#define OPT3001_RESULT_READY_SHORT	150
>> +#define OPT3001_RESULT_READY_LONG	1000
>>  
>>  struct opt3001 {
>>  	struct i2c_client	*client;
>> @@ -92,6 +95,8 @@ struct opt3001 {
>>  
>>  	u8			high_thresh_exp;
>>  	u8			low_thresh_exp;
>> +
>> +	bool			use_irq;
>>  };
>>  
>>  struct opt3001_scale {
>> @@ -230,26 +235,30 @@ static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2)
>>  	u16 reg;
>>  	u8 exponent;
>>  	u16 value;
>> +	long timeout;
>>  
>> -	/*
>> -	 * Enable the end-of-conversion interrupt mechanism. Note that doing
>> -	 * so will overwrite the low-level limit value however we will restore
>> -	 * this value later on.
>> -	 */
>> -	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_LOW_LIMIT,
>> -			OPT3001_LOW_LIMIT_EOC_ENABLE);
>> -	if (ret < 0) {
>> -		dev_err(opt->dev, "failed to write register %02x\n",
>> -				OPT3001_LOW_LIMIT);
>> -		return ret;
>> +	if (opt->use_irq) {
>> +		/*
>> +		 * Enable the end-of-conversion interrupt mechanism. Note that
>> +		 * doing so will overwrite the low-level limit value however we
>> +		 * will restore this value later on.
>> +		 */
>> +		ret = i2c_smbus_write_word_swapped(opt->client,
>> +					OPT3001_LOW_LIMIT,
>> +					OPT3001_LOW_LIMIT_EOC_ENABLE);
>> +		if (ret < 0) {
>> +			dev_err(opt->dev, "failed to write register %02x\n",
>> +					OPT3001_LOW_LIMIT);
>> +			return ret;
>> +		}
>> +
>> +		/* Allow IRQ to access the device despite lock being set */
>> +		opt->ok_to_ignore_lock = true;
>>  	}
>>  
>> -	/* Reset data-ready indicator flag (will be set in the IRQ routine) */
>> +	/* Reset data-ready indicator flag */
>>  	opt->result_ready = false;
>>  
>> -	/* Allow IRQ to access the device despite lock being set */
>> -	opt->ok_to_ignore_lock = true;
>> -
>>  	/* Configure for single-conversion mode and start a new conversion */
>>  	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
>>  	if (ret < 0) {
>> @@ -269,32 +278,69 @@ static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2)
>>  		goto err;
>>  	}
>>  
>> -	/* Wait for the IRQ to indicate the conversion is complete */
>> -	ret = wait_event_timeout(opt->result_ready_queue, opt->result_ready,
>> -			OPT3001_RESULT_READY_TIMEOUT);
>> +	if (opt->use_irq) {
>> +		/* Wait for the IRQ to indicate the conversion is complete */
>> +		ret = wait_event_timeout(opt->result_ready_queue,
>> +				opt->result_ready,
>> +				msecs_to_jiffies(OPT3001_RESULT_READY_LONG));
>> +	} else {
>> +		/* Sleep for result ready time */
>> +		timeout = (opt->int_time == OPT3001_INT_TIME_SHORT) ?
>> +			OPT3001_RESULT_READY_SHORT : OPT3001_RESULT_READY_LONG;
>> +		msleep(timeout);
>> +
>> +		/* Check result ready flag */
>> +		ret = i2c_smbus_read_word_swapped(opt->client,
>> +						  OPT3001_CONFIGURATION);
>> +		if (ret < 0) {
>> +			dev_err(opt->dev, "failed to read register %02x\n",
>> +				OPT3001_CONFIGURATION);
>> +			goto err;
>> +		}
>> +
>> +		if (!(ret & OPT3001_CONFIGURATION_CRF)) {
>> +			ret = -ETIMEDOUT;
>> +			goto err;
>> +		}
>> +
>> +		/* Obtain value */
>> +		ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_RESULT);
>> +		if (ret < 0) {
>> +			dev_err(opt->dev, "failed to read register %02x\n",
>> +				OPT3001_RESULT);
>> +			goto err;
>> +		}
>> +		opt->result = ret;
>> +		opt->result_ready = true;
>> +	}
>>  
>>  err:
>> -	/* Disallow IRQ to access the device while lock is active */
>> -	opt->ok_to_ignore_lock = false;
>> +	if (opt->use_irq)
>> +		/* Disallow IRQ to access the device while lock is active */
>> +		opt->ok_to_ignore_lock = false;
>>  
>>  	if (ret == 0)
>>  		return -ETIMEDOUT;
>>  	else if (ret < 0)
>>  		return ret;
>>  
>> -	/*
>> -	 * Disable the end-of-conversion interrupt mechanism by restoring the
>> -	 * low-level limit value (clearing OPT3001_LOW_LIMIT_EOC_ENABLE). Note
>> -	 * that selectively clearing those enable bits would affect the actual
>> -	 * limit value due to bit-overlap and therefore can't be done.
>> -	 */
>> -	value = (opt->low_thresh_exp << 12) | opt->low_thresh_mantissa;
>> -	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_LOW_LIMIT,
>> -			value);
>> -	if (ret < 0) {
>> -		dev_err(opt->dev, "failed to write register %02x\n",
>> -				OPT3001_LOW_LIMIT);
>> -		return ret;
>> +	if (opt->use_irq) {
>> +		/*
>> +		 * Disable the end-of-conversion interrupt mechanism by
>> +		 * restoring the low-level limit value (clearing
>> +		 * OPT3001_LOW_LIMIT_EOC_ENABLE). Note that selectively clearing
>> +		 * those enable bits would affect the actual limit value due to
>> +		 * bit-overlap and therefore can't be done.
>> +		 */
>> +		value = (opt->low_thresh_exp << 12) | opt->low_thresh_mantissa;
>> +		ret = i2c_smbus_write_word_swapped(opt->client,
>> +						   OPT3001_LOW_LIMIT,
>> +						   value);
>> +		if (ret < 0) {
>> +			dev_err(opt->dev, "failed to write register %02x\n",
>> +					OPT3001_LOW_LIMIT);
>> +			return ret;
>> +		}
>>  	}
>>  
>>  	exponent = OPT3001_REG_EXPONENT(opt->result);
>> @@ -736,12 +782,18 @@ static int opt3001_probe(struct i2c_client *client,
>>  		return ret;
>>  	}
>>  
>> -	ret = request_threaded_irq(irq, NULL, opt3001_irq,
>> -			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> -			"opt3001", iio);
>> -	if (ret) {
>> -		dev_err(dev, "failed to request IRQ #%d\n", irq);
>> -		return ret;
>> +	/* Make use of INT pin only if valid IRQ no. is given */
>> +	if (irq > 0) {
>> +		ret = request_threaded_irq(irq, NULL, opt3001_irq,
>> +				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> +				"opt3001", iio);
>> +		if (ret) {
>> +			dev_err(dev, "failed to request IRQ #%d\n", irq);
>> +			return ret;
>> +		}
>> +		opt->use_irq = true;
>> +	} else {
>> +		dev_info(opt->dev, "enabling interrupt-less operation\n");
>>  	}
>>  
>>  	return 0;
>> @@ -754,7 +806,8 @@ static int opt3001_remove(struct i2c_client *client)
>>  	int ret;
>>  	u16 reg;
>>  
>> -	free_irq(client->irq, iio);
>> +	if (opt->use_irq)
>> +		free_irq(client->irq, iio);
>>  
>>  	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
>>  	if (ret < 0) {
>> -- 
>> 2.7.0
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>