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

[PATCH 0/2] 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).


Alexander Koch (2):
  iio: light: opt3001: extract int. time constants
  iio: light: opt3001: enable operation w/o IRQ

 drivers/iio/light/opt3001.c | 152 ++++++++++++++++++++++++++++++--------------
 1 file changed, 104 insertions(+), 48 deletions(-)

-- 
2.7.0

[PATCH 1/2] 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 2/2] 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 aefbd79..2abc18a 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;
+
+	u16			use_irq:1;
 };
 
 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 2/2] iio: light: opt3001: enable operation w/o IRQ

[Peter Meerwald-Stadler]

On Tue, 12 Jan 2016, 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.

looks good, I'd rather use a bool for use_irq and the msecs_to_jiffies() 
call moved from the #define to the code (which is not strictly necessary 
for the patch) -- matter of taste
 
> 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 aefbd79..2abc18a 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;
> +
> +	u16			use_irq:1;
>  };
>  
>  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) {
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

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

[Alexander Koch]

Am 12.01.2016 um 20:27 schrieb Peter Meerwald-Stadler:
> On Tue, 12 Jan 2016, 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.
> 
> looks good, I'd rather use a bool for use_irq and the msecs_to_jiffies() 
> call moved from the #define to the code (which is not strictly necessary 
> for the patch) -- matter of taste

Thanks - actually this is my first patch, so positive feedback much
appreciated!

Concerning the bool for 'use_irq': I first had it that way but then
opted for the bit field of length 1 as I wasn't sure whether bool would
get optimized to the same level by the compiler.

I'm a bit irritated by your comment concerning the msecs_to_jiffies()
call, as my patch indeed moves this call from the #define to the code.
Did you mean it the other way round, then?
My reason to move it was that I need to work with microseconds for the
IRQ-less operation mode, and jiffies are only required in one place for
the IRQ mode.


Best regards

lynix

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

[Jonathan Cameron]

On 12/01/16 20:17, Alexander Koch wrote:
> Am 12.01.2016 um 20:27 schrieb Peter Meerwald-Stadler:
>> On Tue, 12 Jan 2016, 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.
>>
>> looks good, I'd rather use a bool for use_irq and the msecs_to_jiffies() 
>> call moved from the #define to the code (which is not strictly necessary 
>> for the patch) -- matter of taste
> 
> Thanks - actually this is my first patch, so positive feedback much
> appreciated!
> 
> Concerning the bool for 'use_irq': I first had it that way but then
> opted for the bit field of length 1 as I wasn't sure whether bool would
> get optimized to the same level by the compiler.
Bit fields are often less efficient as the compiler has to separate them out
using shifts and masks.  Also from a space point of view the data structure
will be considerably padded anyway for a couple of reasons:
1) It contains u32 fields so will at least be padded to a multiple of u32.
2) Memory allocations may well be a good bit larger depending on exact
sizes vs the blob levels available in the memory allocator.

Basic rule of thumb - keep things simple and let the compiler do the work.
So a bool is suitable here.

> 
> I'm a bit irritated by your comment concerning the msecs_to_jiffies()
> call, as my patch indeed moves this call from the #define to the code.
> Did you mean it the other way round, then?
Presumably ;) 
> My reason to move it was that I need to work with microseconds for the
> IRQ-less operation mode, and jiffies are only required in one place for
> the IRQ mode.
Now perhaps the 'right' way to do this would be have been a precursor patch
removing the define rather than lumping what is an an connected change (in
many ways) in here.  Overall I agree the change is worthwhile and trivial.
As Peter said, it's a matter of taste!  We both happen to disagree with him
on this point.
> 
> 
	> Best regards
> 
> lynix
> 

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

[Jonathan Cameron]

On 16/01/16 12:52, Jonathan Cameron wrote:
> On 12/01/16 20:17, Alexander Koch wrote:
>> Am 12.01.2016 um 20:27 schrieb Peter Meerwald-Stadler:
>>> On Tue, 12 Jan 2016, 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.
>>>
>>> looks good, I'd rather use a bool for use_irq and the msecs_to_jiffies() 
>>> call moved from the #define to the code (which is not strictly necessary 
>>> for the patch) -- matter of taste
>>
>> Thanks - actually this is my first patch, so positive feedback much
>> appreciated!
>>
>> Concerning the bool for 'use_irq': I first had it that way but then
>> opted for the bit field of length 1 as I wasn't sure whether bool would
>> get optimized to the same level by the compiler.
> Bit fields are often less efficient as the compiler has to separate them out
> using shifts and masks.  Also from a space point of view the data structure
> will be considerably padded anyway for a couple of reasons:
> 1) It contains u32 fields so will at least be padded to a multiple of u32.
> 2) Memory allocations may well be a good bit larger depending on exact
> sizes vs the blob levels available in the memory allocator.
> 
> Basic rule of thumb - keep things simple and let the compiler do the work.
> So a bool is suitable here.
> 
>>
>> I'm a bit irritated by your comment concerning the msecs_to_jiffies()
>> call, as my patch indeed moves this call from the #define to the code.
>> Did you mean it the other way round, then?
> Presumably ;) 
>> My reason to move it was that I need to work with microseconds for the
>> IRQ-less operation mode, and jiffies are only required in one place for
>> the IRQ mode.
> Now perhaps the 'right' way to do this would be have been a precursor patch
> removing the define rather than lumping what is an an connected change (in
> many ways) in here.  Overall I agree the change is worthwhile and trivial.
> As Peter said, it's a matter of taste!  We both happen to disagree with him
> on this point.

ps. Should have said that other than the bit field vs bool change, the patch
looks good to me.

Jonathan
>>
>>
> 	> Best regards
>>
>> lynix
>>
> 
> --
> 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 2/2] iio: light: opt3001: enable operation w/o IRQ

[Alexander Koch]

Am 16.01.2016 um 13:53 schrieb Jonathan Cameron:
> On 16/01/16 12:52, Jonathan Cameron wrote:
>> On 12/01/16 20:17, Alexander Koch wrote:
>>> Am 12.01.2016 um 20:27 schrieb Peter Meerwald-Stadler:
>>>> On Tue, 12 Jan 2016, 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.
>>>>
>>>> looks good, I'd rather use a bool for use_irq and the msecs_to_jiffies() 
>>>> call moved from the #define to the code (which is not strictly necessary 
>>>> for the patch) -- matter of taste
>>>
>>> Thanks - actually this is my first patch, so positive feedback much
>>> appreciated!
>>>
>>> Concerning the bool for 'use_irq': I first had it that way but then
>>> opted for the bit field of length 1 as I wasn't sure whether bool would
>>> get optimized to the same level by the compiler.
>> Bit fields are often less efficient as the compiler has to separate them out
>> using shifts and masks.  Also from a space point of view the data structure
>> will be considerably padded anyway for a couple of reasons:
>> 1) It contains u32 fields so will at least be padded to a multiple of u32.
>> 2) Memory allocations may well be a good bit larger depending on exact
>> sizes vs the blob levels available in the memory allocator.
>>
>> Basic rule of thumb - keep things simple and let the compiler do the work.
>> So a bool is suitable here.
>>
>>>
>>> I'm a bit irritated by your comment concerning the msecs_to_jiffies()
>>> call, as my patch indeed moves this call from the #define to the code.
>>> Did you mean it the other way round, then?
>> Presumably ;) 
>>> My reason to move it was that I need to work with microseconds for the
>>> IRQ-less operation mode, and jiffies are only required in one place for
>>> the IRQ mode.
>> Now perhaps the 'right' way to do this would be have been a precursor patch
>> removing the define rather than lumping what is an an connected change (in
>> many ways) in here.  Overall I agree the change is worthwhile and trivial.
>> As Peter said, it's a matter of taste!  We both happen to disagree with him
>> on this point.
> 
> ps. Should have said that other than the bit field vs bool change, the patch
> looks good to me.

Okay then, so will send a v2 of the patch that includes this bool
change, shortly.

While I'm at it, maybe I should include a second refactoring commit that
changes the other bitfield members of the opt3001-struct that are used
as bool as well - namely 'ok_to_ignore_lock' and 'result_ready'. I hope
this is okay.


Best regards

Alex

> 
> Jonathan
>>>
>>>
>> 	> Best regards
>>>
>>> lynix
>>>
>>
>> --
>> 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
>>
>