[PATCH 1/2] iio: add INT_TIME (integration time) channel info attribute

[PATCH 1/2] iio: add INT_TIME (integration time) channel info attribute

[Peter Meerwald]

integration time is in seconds; it controls the measurement
time and influences the gain of a sensor

used by adjd_s311, tsl4531
the following drivers have similar controls:
* tsl2563 (integration time is controlled via CALIBSCALE among other things)
* tsl2583 (has integration_time device_attr, but driver doesn't use channels yet)
* tsl2x7x (has integration_time attr)

Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
Cc: Jon Brenner <jon.brenner@ams.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 12 ++++++++++++
 drivers/iio/industrialio-core.c         |  1 +
 include/linux/iio/iio.h                 |  1 +
 include/linux/iio/sysfs.h               | 15 +++++++++++++++
 4 files changed, 29 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 5caaab9..d75147c 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -800,3 +800,15 @@ Description:
 		Writing '1' stores the current device configuration into
 		on-chip EEPROM. After power-up or chip reset the device will
 		automatically load the saved configuration.
+
+What:		/sys/.../iio:deviceX/in_intensity_red_integration_time
+What:		/sys/.../iio:deviceX/in_intensity_green_integration_time
+What:		/sys/.../iio:deviceX/in_intensity_blue_integration_time
+What:		/sys/.../iio:deviceX/in_intensity_clear_integration_time
+What:		/sys/.../iio:deviceX/in_illuminance_integration_time
+KernelVersion:	3.12
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute is used to get/set the integration time in
+		seconds.
+
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 97f0297..2cb4841 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -101,6 +101,7 @@ static const char * const iio_chan_info_postfix[] = {
 	[IIO_CHAN_INFO_PHASE] = "phase",
 	[IIO_CHAN_INFO_HARDWAREGAIN] = "hardwaregain",
 	[IIO_CHAN_INFO_HYSTERESIS] = "hysteresis",
+	[IIO_CHAN_INFO_INT_TIME] = "integration_time",
 };
 
 const struct iio_chan_spec
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 09ebe0a..45a99ef 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -36,6 +36,7 @@ enum iio_chan_info_enum {
 	IIO_CHAN_INFO_PHASE,
 	IIO_CHAN_INFO_HARDWAREGAIN,
 	IIO_CHAN_INFO_HYSTERESIS,
+	IIO_CHAN_INFO_INT_TIME,
 };
 
 enum iio_endian {
diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
index 2958c96..8a1d186 100644
--- a/include/linux/iio/sysfs.h
+++ b/include/linux/iio/sysfs.h
@@ -100,6 +100,21 @@ struct iio_const_attr {
 #define IIO_CONST_ATTR_SAMP_FREQ_AVAIL(_string)			\
 	IIO_CONST_ATTR(sampling_frequency_available, _string)
 
+/**
+ * IIO_DEV_ATTR_INT_TIME_AVAIL - list available integration times
+ * @_show: output method for the attribute
+ **/
+#define IIO_DEV_ATTR_INT_TIME_AVAIL(_show)		\
+	IIO_DEVICE_ATTR(integration_time_available, S_IRUGO, _show, NULL, 0)
+/**
+ * IIO_CONST_ATTR_INT_TIME_AVAIL - list available integration times
+ * @_string: frequency string for the attribute
+ *
+ * Constant version
+ **/
+#define IIO_CONST_ATTR_INT_TIME_AVAIL(_string)		\
+	IIO_CONST_ATTR(integration_time_available, _string)
+
 #define IIO_DEV_ATTR_TEMP_RAW(_show)			\
 	IIO_DEVICE_ATTR(in_temp_raw, S_IRUGO, _show, NULL, 0)
 
-- 
1.8.3.4

[PATCH 2/2] iio: adjd_s311: Use INT_TIME channel info

[Peter Meerwald]

Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
---
 drivers/iio/light/adjd_s311.c | 72 +++++++++++++------------------------------
 1 file changed, 21 insertions(+), 51 deletions(-)

diff --git a/drivers/iio/light/adjd_s311.c b/drivers/iio/light/adjd_s311.c
index 23cff79..8853251 100644
--- a/drivers/iio/light/adjd_s311.c
+++ b/drivers/iio/light/adjd_s311.c
@@ -114,43 +114,6 @@ static int adjd_s311_read_data(struct iio_dev *indio_dev, u8 reg, int *val)
 	return 0;
 }
 
-static ssize_t adjd_s311_read_int_time(struct iio_dev *indio_dev,
-	uintptr_t private, const struct iio_chan_spec *chan, char *buf)
-{
-	struct adjd_s311_data *data = iio_priv(indio_dev);
-	s32 ret;
-
-	ret = i2c_smbus_read_word_data(data->client,
-		ADJD_S311_INT_REG(chan->address));
-	if (ret < 0)
-		return ret;
-
-	return sprintf(buf, "%d\n", ret & ADJD_S311_INT_MASK);
-}
-
-static ssize_t adjd_s311_write_int_time(struct iio_dev *indio_dev,
-	 uintptr_t private, const struct iio_chan_spec *chan, const char *buf,
-	 size_t len)
-{
-	struct adjd_s311_data *data = iio_priv(indio_dev);
-	unsigned long int_time;
-	int ret;
-
-	ret = kstrtoul(buf, 10, &int_time);
-	if (ret)
-		return ret;
-
-	if (int_time > ADJD_S311_INT_MASK)
-		return -EINVAL;
-
-	ret = i2c_smbus_write_word_data(data->client,
-		ADJD_S311_INT_REG(chan->address), int_time);
-	if (ret < 0)
-		return ret;
-
-	return len;
-}
-
 static irqreturn_t adjd_s311_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -186,25 +149,16 @@ done:
 	return IRQ_HANDLED;
 }
 
-static const struct iio_chan_spec_ext_info adjd_s311_ext_info[] = {
-	{
-		.name = "integration_time",
-		.read = adjd_s311_read_int_time,
-		.write = adjd_s311_write_int_time,
-	},
-	{ }
-};
-
 #define ADJD_S311_CHANNEL(_color, _scan_idx) { \
 	.type = IIO_INTENSITY, \
 	.modified = 1, \
 	.address = (IDX_##_color), \
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
-		BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
+		BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \
+		BIT(IIO_CHAN_INFO_INT_TIME), \
 	.channel2 = (IIO_MOD_LIGHT_##_color), \
 	.scan_index = (_scan_idx), \
 	.scan_type = IIO_ST('u', 10, 16, 0), \
-	.ext_info = adjd_s311_ext_info, \
 }
 
 static const struct iio_chan_spec adjd_s311_channels[] = {
@@ -236,6 +190,18 @@ static int adjd_s311_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		*val = ret & ADJD_S311_CAP_MASK;
 		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = i2c_smbus_read_word_data(data->client,
+			ADJD_S311_INT_REG(chan->address));
+		if (ret < 0)
+			return ret;
+		*val = 0;
+		/*
+		 * not documented, based on measurement:
+		 * 4095 LSBs correspond to roughly 4 ms
+		 */
+		*val2 = ret & ADJD_S311_INT_MASK;
+		return IIO_VAL_INT_PLUS_MICRO;
 	}
 	return -EINVAL;
 }
@@ -245,16 +211,20 @@ static int adjd_s311_write_raw(struct iio_dev *indio_dev,
 			       int val, int val2, long mask)
 {
 	struct adjd_s311_data *data = iio_priv(indio_dev);
-	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_HARDWAREGAIN:
 		if (val < 0 || val > ADJD_S311_CAP_MASK)
 			return -EINVAL;
 
-		ret = i2c_smbus_write_byte_data(data->client,
+		return i2c_smbus_write_byte_data(data->client,
 			ADJD_S311_CAP_REG(chan->address), val);
-		return ret;
+	case IIO_CHAN_INFO_INT_TIME:
+		if (val != 0 || val2 < 0 || val2 > ADJD_S311_INT_MASK)
+			return -EINVAL;
+
+		return i2c_smbus_write_word_data(data->client,
+			ADJD_S311_INT_REG(chan->address), val2);
 	}
 	return -EINVAL;
 }
-- 
1.8.3.4

Re: [PATCH 1/2] iio: add INT_TIME (integration time) channel info attribute

[Jonathan Cameron]

On 08/18/13 14:00, Peter Meerwald wrote:
> integration time is in seconds; it controls the measurement
> time and influences the gain of a sensor
> 
> used by adjd_s311, tsl4531
> the following drivers have similar controls:
> * tsl2563 (integration time is controlled via CALIBSCALE among other things)
> * tsl2583 (has integration_time device_attr, but driver doesn't use channels yet)
> * tsl2x7x (has integration_time attr)

Hi Peter,

I know this has come up before (and I'm not actually expressing oposition
to this going into info_mask) but could you sumarise the arguments for why
this property cannot be covered by calibscale or scale?

I'd like to have a little stronger argument for the patch description in Git.

Let me have a quick bash at it (may be completely wrong).

There are two typical ways that scaling is implemented in a device:
1) input amplifier
2) reference to the ADC is changed.
These both result in the accuracy of the ADC varying (by applying it's
sampling over ta more relevant range).

Integration time is a way of dealing with noise inherent in the analog
sensor itself.  In this case a mixture of Photon noise and device specific
noise.  Photon noise is dealt with by either improving the efficiency of
the sensor, (more photons actually captured) which is not easily varied dynamically,
or by integrating the measurement over a longer time period.  Note that this
can also be thought of as an averaging of a number of individual samples
and is infact sometimes implemented this way.

Hence it makes sense to distinguish between integration time and simple
scale. In some devices both types of control are present and whilst they
will have similar effects on the amplitude of the reading, their effect
on the noise on the measurements will differ considerably.

Something like that?

Thanks,

Jonathan
> 
> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
> Cc: Jon Brenner <jon.brenner@ams.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 12 ++++++++++++
>  drivers/iio/industrialio-core.c         |  1 +
>  include/linux/iio/iio.h                 |  1 +
>  include/linux/iio/sysfs.h               | 15 +++++++++++++++
>  4 files changed, 29 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 5caaab9..d75147c 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -800,3 +800,15 @@ Description:
>  		Writing '1' stores the current device configuration into
>  		on-chip EEPROM. After power-up or chip reset the device will
>  		automatically load the saved configuration.
> +
> +What:		/sys/.../iio:deviceX/in_intensity_red_integration_time
> +What:		/sys/.../iio:deviceX/in_intensity_green_integration_time
> +What:		/sys/.../iio:deviceX/in_intensity_blue_integration_time
> +What:		/sys/.../iio:deviceX/in_intensity_clear_integration_time
> +What:		/sys/.../iio:deviceX/in_illuminance_integration_time
> +KernelVersion:	3.12
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute is used to get/set the integration time in
> +		seconds.
> +
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 97f0297..2cb4841 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -101,6 +101,7 @@ static const char * const iio_chan_info_postfix[] = {
>  	[IIO_CHAN_INFO_PHASE] = "phase",
>  	[IIO_CHAN_INFO_HARDWAREGAIN] = "hardwaregain",
>  	[IIO_CHAN_INFO_HYSTERESIS] = "hysteresis",
> +	[IIO_CHAN_INFO_INT_TIME] = "integration_time",
>  };
>  
>  const struct iio_chan_spec
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 09ebe0a..45a99ef 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -36,6 +36,7 @@ enum iio_chan_info_enum {
>  	IIO_CHAN_INFO_PHASE,
>  	IIO_CHAN_INFO_HARDWAREGAIN,
>  	IIO_CHAN_INFO_HYSTERESIS,
> +	IIO_CHAN_INFO_INT_TIME,
>  };
>  
>  enum iio_endian {
> diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
> index 2958c96..8a1d186 100644
> --- a/include/linux/iio/sysfs.h
> +++ b/include/linux/iio/sysfs.h
> @@ -100,6 +100,21 @@ struct iio_const_attr {
>  #define IIO_CONST_ATTR_SAMP_FREQ_AVAIL(_string)			\
>  	IIO_CONST_ATTR(sampling_frequency_available, _string)
>  
> +/**
> + * IIO_DEV_ATTR_INT_TIME_AVAIL - list available integration times
> + * @_show: output method for the attribute
> + **/
> +#define IIO_DEV_ATTR_INT_TIME_AVAIL(_show)		\
> +	IIO_DEVICE_ATTR(integration_time_available, S_IRUGO, _show, NULL, 0)
> +/**
> + * IIO_CONST_ATTR_INT_TIME_AVAIL - list available integration times
> + * @_string: frequency string for the attribute
> + *
> + * Constant version
> + **/
> +#define IIO_CONST_ATTR_INT_TIME_AVAIL(_string)		\
> +	IIO_CONST_ATTR(integration_time_available, _string)
> +
>  #define IIO_DEV_ATTR_TEMP_RAW(_show)			\
>  	IIO_DEVICE_ATTR(in_temp_raw, S_IRUGO, _show, NULL, 0)
>  
> 

Re: [PATCH 1/2] iio: add INT_TIME (integration time) channel info attribute

[Peter Meerwald]

Hello,

> > integration time is in seconds; it controls the measurement
> > time and influences the gain of a sensor
> > 
> > used by adjd_s311, tsl4531
> > the following drivers have similar controls:
> > * tsl2563 (integration time is controlled via CALIBSCALE among other things)
> > * tsl2583 (has integration_time device_attr, but driver doesn't use channels yet)
> > * tsl2x7x (has integration_time attr)

> I know this has come up before (and I'm not actually expressing oposition
> to this going into info_mask) but could you sumarise the arguments for why
> this property cannot be covered by calibscale or scale?
> I'd like to have a little stronger argument for the patch description in Git.

sounds good; I'd add:

> There are two typical ways that scaling is implemented in a device:
> 1) input amplifier
> 2) reference to the ADC is changed.
> These both result in the accuracy of the ADC varying (by applying it's
> sampling over ta more relevant range).
> 
> Integration time is a way of dealing with noise inherent in the analog
> sensor itself.  In this case a mixture of Photon noise and device specific
> noise.  Photon noise is dealt with by either improving the efficiency of
> the sensor, (more photons actually captured) which is not easily varied dynamically,
> or by integrating the measurement over a longer time period.  Note that this
> can also be thought of as an averaging of a number of individual samples
> and is infact sometimes implemented this way.

altering integration time implies that the duration of a 
measurement changes and the user of the device may be interested
 
> Hence it makes sense to distinguish between integration time and simple
> scale. In some devices both types of control are present and whilst they
> will have similar effects on the amplitude of the reading, their effect
> on the noise on the measurements will differ considerably.

integration time may or may not have an impact on the range of values
measured

both characteristics cannot be expressed with scale/calibscale in a 
satisfying way

let's see if further comments come up, and I'll repost

please note that there is a fix for the adjd_s311 driver posted which you 
may want to fast-feed to Greg

thanks, regards, p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH 1/2] iio: add INT_TIME (integration time) channel info attribute

[Jonathan Cameron]

On 08/18/13 22:30, Peter Meerwald wrote:
> Hello,
> 
>>> integration time is in seconds; it controls the measurement
>>> time and influences the gain of a sensor
>>>
>>> used by adjd_s311, tsl4531
>>> the following drivers have similar controls:
>>> * tsl2563 (integration time is controlled via CALIBSCALE among other things)
>>> * tsl2583 (has integration_time device_attr, but driver doesn't use channels yet)
>>> * tsl2x7x (has integration_time attr)
> 
>> I know this has come up before (and I'm not actually expressing oposition
>> to this going into info_mask) but could you sumarise the arguments for why
>> this property cannot be covered by calibscale or scale?
>> I'd like to have a little stronger argument for the patch description in Git.
> 
> sounds good; I'd add:
> 
>> There are two typical ways that scaling is implemented in a device:
>> 1) input amplifier
>> 2) reference to the ADC is changed.
>> These both result in the accuracy of the ADC varying (by applying it's
>> sampling over ta more relevant range).
>>
>> Integration time is a way of dealing with noise inherent in the analog
>> sensor itself.  In this case a mixture of Photon noise and device specific
>> noise.  Photon noise is dealt with by either improving the efficiency of
>> the sensor, (more photons actually captured) which is not easily varied dynamically,
>> or by integrating the measurement over a longer time period.  Note that this
>> can also be thought of as an averaging of a number of individual samples
>> and is infact sometimes implemented this way.
> 
> altering integration time implies that the duration of a 
> measurement changes and the user of the device may be interested
>  
>> Hence it makes sense to distinguish between integration time and simple
>> scale. In some devices both types of control are present and whilst they
>> will have similar effects on the amplitude of the reading, their effect
>> on the noise on the measurements will differ considerably.
> 
> integration time may or may not have an impact on the range of values
> measured
> 
> both characteristics cannot be expressed with scale/calibscale in a 
> satisfying way
> 
> let's see if further comments come up, and I'll repost
Not much come out of the wood work so something like what you propose will do nicely!

I would also like a short description in the documentation file so we don't have
to keep refering people back to the original patch.

Jon, you still out there?  Not heard anything from you for a while and your input
on this sort of thing is always valued!

Jonathan
> 
> please note that there is a fix for the adjd_s311 driver posted which you 
> may want to fast-feed to Greg
> 
> thanks, regards, p.
> 

RE: [PATCH 1/2] iio: add INT_TIME (integration time) channel info attribute

[Jon Brenner]

Jonathan, et al.,
Sorry guys - I've been completely snowed under.
I have some thoughts but no time to annunciate.
What I do have is confidence in all of you to do the right thing.  I'll cat=
ch up later.
Relationships between integration time and gain can be tricky due to other =
external factors (aperture, glass density, inking, etc.) and need to be bal=
anced accordingly. =20
Throw in some filtering (ie persistence) and offset calibrations and you ha=
ve a sticky wicket.
Fortunately, most of these devices provide the ability to set parameters in=
dependently.  It's a blessing and it's a curse. =20
I will be immersed in a wicked project for the next several weeks, after wh=
ich I expect to come back on-line with some really cool devices coming to I=
IO.

BR
Jon

________________________________________
From: Jonathan Cameron [jic23@kernel.org]
Sent: Sunday, September 01, 2013 8:33 PM
To: Peter Meerwald
Cc: linux-iio@vger.kernel.org; Jon Brenner; Lars-Peter Clausen
Subject: Re: [PATCH 1/2] iio: add INT_TIME (integration time) channel info =
attribute

On 08/18/13 22:30, Peter Meerwald wrote:
> Hello,
>
>>> integration time is in seconds; it controls the measurement
>>> time and influences the gain of a sensor
>>>
>>> used by adjd_s311, tsl4531
>>> the following drivers have similar controls:
>>> * tsl2563 (integration time is controlled via CALIBSCALE among other th=
ings)
>>> * tsl2583 (has integration_time device_attr, but driver doesn't use cha=
nnels yet)
>>> * tsl2x7x (has integration_time attr)
>
>> I know this has come up before (and I'm not actually expressing opositio=
n
>> to this going into info_mask) but could you sumarise the arguments for w=
hy
>> this property cannot be covered by calibscale or scale?
>> I'd like to have a little stronger argument for the patch description in=
 Git.
>
> sounds good; I'd add:
>
>> There are two typical ways that scaling is implemented in a device:
>> 1) input amplifier
>> 2) reference to the ADC is changed.
>> These both result in the accuracy of the ADC varying (by applying it's
>> sampling over ta more relevant range).
>>
>> Integration time is a way of dealing with noise inherent in the analog
>> sensor itself.  In this case a mixture of Photon noise and device specif=
ic
>> noise.  Photon noise is dealt with by either improving the efficiency of
>> the sensor, (more photons actually captured) which is not easily varied =
dynamically,
>> or by integrating the measurement over a longer time period.  Note that =
this
>> can also be thought of as an averaging of a number of individual samples
>> and is infact sometimes implemented this way.
>
> altering integration time implies that the duration of a
> measurement changes and the user of the device may be interested
>
>> Hence it makes sense to distinguish between integration time and simple
>> scale. In some devices both types of control are present and whilst they
>> will have similar effects on the amplitude of the reading, their effect
>> on the noise on the measurements will differ considerably.
>
> integration time may or may not have an impact on the range of values
> measured
>
> both characteristics cannot be expressed with scale/calibscale in a
> satisfying way
>
> let's see if further comments come up, and I'll repost
Not much come out of the wood work so something like what you propose will =
do nicely!

I would also like a short description in the documentation file so we don't=
 have
to keep refering people back to the original patch.

Jon, you still out there?  Not heard anything from you for a while and your=
 input
on this sort of thing is always valued!

Jonathan
>
> please note that there is a fix for the adjd_s311 driver posted which you
> may want to fast-feed to Greg
>
> thanks, regards, p.
>=