[RFC 1/4] iio: Introduce a new fractional value type

[RFC 1/4] iio: Introduce a new fractional value type

[Lars-Peter Clausen]

This series is just a request for comments right now and at this point it is
barley tested.

Currently IIO uses a decimal fixed point representations for real type numbers.
This patch introduces a new representation for rational type numbers. The number
will be expressed by specifying a numerator and denominator. For converting a
raw value to a processed value multiply it by the numerator and divide it by the
denominator.

The reasoning for introducing this new type is that for a lot of devices the
scale can be represented easily by a fractional number, but it is not possible
to represent it as fixed point number without rounding.  E.g. for a simple DAC
the scale is often the reference voltage divided by the number of possible
values (Usually 2**n_bits - 1). Each driver currently implements the conversion
of this fraction to a fixed point number on its own.

Also when it comes to the in-kernel interface this allows to directly use the
fractional factors to convert a raw value to a processed value. This should on
one hand require less instructions and on the other hand increase the
precession.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/industrialio-core.c |    9 +++++++++
 include/linux/iio/types.h       |    1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 2ec266e..69e60c5 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -365,6 +365,7 @@ static ssize_t iio_read_channel_info(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	unsigned long long tmp;
 	int val, val2;
 	bool scale_db = false;
 	int ret = indio_dev->info->read_raw(indio_dev, this_attr->c,
@@ -390,6 +391,14 @@ static ssize_t iio_read_channel_info(struct device *dev,
 			return sprintf(buf, "-%d.%09u\n", val, -val2);
 		else
 			return sprintf(buf, "%d.%09u\n", val, val2);
+	case IIO_VAL_FRACTIONAL:
+		tmp = div_s64((s64)val * 1000000000LL, val2);
+		val2 = do_div(tmp, 1000000000LL);
+		val = tmp;
+		return sprintf(buf, "%d.%09u\n", val, val2);
+
+		/* Or maybe we could do something like this:
+		return sprintf(buf, "%d/%d\n", val, val2); */
 	default:
 		return 0;
 	}
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index 44e3977..8ef5fde 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -57,5 +57,6 @@ enum iio_modifier {
 #define IIO_VAL_INT_PLUS_MICRO 2
 #define IIO_VAL_INT_PLUS_NANO 3
 #define IIO_VAL_INT_PLUS_MICRO_DB 4
+#define IIO_VAL_FRACTIONAL 5
 
 #endif /* _IIO_TYPES_H_ */
-- 
1.7.10.4

[RFC 2/4] iio:inkern: Add function to read the processed value

[Lars-Peter Clausen]

Add a function to read a processed value from a channel. The function will first
attempt to read the IIO_CHAN_INFO_PROCESSED attribute. If that fails it will
read the IIO_CHAN_INFO_RAW attribute and convert the result from a raw value to
a processed value.

The patch also introduces a function to convert raw value to a processed value
and exports it, in case a user needs or wants to do the conversion by itself.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

---
Documentation is still missing.
---
 drivers/iio/inkern.c         |  118 ++++++++++++++++++++++++++++++++++++++++--
 include/linux/iio/consumer.h |    4 ++
 2 files changed, 118 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index b5afc2f..9bb3391 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -248,6 +248,118 @@ err_unlock:
 }
 EXPORT_SYMBOL_GPL(iio_read_channel_raw);
 
+static int iio_read_channel_scale_unlocked(struct iio_channel *chan, int *val,
+	int *val2)
+{
+	return chan->indio_dev->info->read_raw(chan->indio_dev,
+					      chan->channel,
+					      val, val2,
+					      IIO_CHAN_INFO_SCALE);
+}
+
+static int iio_read_channel_offset_unlocked(struct iio_channel *chan, int *val)
+{
+	int val2;
+
+	return chan->indio_dev->info->read_raw(chan->indio_dev,
+					      chan->channel,
+					      val, &val2,
+					      IIO_CHAN_INFO_OFFSET);
+}
+
+/* It may make sense to a add a extra scale attribute here to avoid precession
+ * loss if the user wants a more fine grained value than the IIO default, e.g.
+ * micro instead of milli volt. */
+static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
+	int raw, int *processed)
+{
+	int scale_type, scale_val, scale_val2, offset;
+	s64 raw64 = raw;
+	int ret;
+
+
+	ret = iio_read_channel_offset_unlocked(chan, &offset);
+	if (ret == 0)
+		raw64 += offset;
+
+	scale_type = iio_read_channel_scale_unlocked(chan, &scale_val, &scale_val2);
+	if (scale_type < 0)
+		return scale_type;
+
+	switch (scale_type) {
+	case IIO_VAL_INT:
+		*processed = raw * scale_val;
+		break;
+	case IIO_VAL_INT_PLUS_MICRO:
+		if (scale_val2 < 0)
+			*processed = -raw * scale_val;
+		else
+			*processed = raw * scale_val;
+		*processed += div_s64(raw * (s64)scale_val2, 1000000LL);
+		break;
+	case IIO_VAL_INT_PLUS_NANO:
+		if (scale_val2 < 0)
+			*processed = -raw * scale_val;
+		else
+			*processed = raw * scale_val;
+		*processed += div_s64(raw * (s64)scale_val2, 1000000000LL);
+		break;
+	case IIO_VAL_FRACTIONAL:
+		*processed = div_s64(raw * (s64)scale_val, scale_val2);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+
+int iio_convert_raw_to_processed(struct iio_channel *chan, int raw, int *processed)
+{
+	int ret;
+
+	mutex_lock(&chan->indio_dev->info_exist_lock);
+	if (chan->indio_dev->info == NULL) {
+		ret = -ENODEV;
+		goto err_unlock;
+	}
+
+	ret = iio_convert_raw_to_processed_unlocked(chan, raw, processed);
+
+err_unlock:
+	mutex_unlock(&chan->indio_dev->info_exist_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iio_convert_raw_to_processed);
+
+int iio_read_channel_processed(struct iio_channel *chan, int *val)
+{
+	int val2, ret;
+
+	mutex_lock(&chan->indio_dev->info_exist_lock);
+	if (chan->indio_dev->info == NULL) {
+		ret = -ENODEV;
+		goto err_unlock;
+	}
+
+	ret = chan->indio_dev->info->read_raw(chan->indio_dev, chan->channel,
+					      val, &val2, IIO_CHAN_INFO_PROCESSED);
+	if (ret < 0) {
+		ret = chan->indio_dev->info->read_raw(chan->indio_dev, chan->channel,
+					      val, &val2, IIO_CHAN_INFO_RAW);
+		if (ret < 0)
+			goto err_unlock;
+		ret = iio_convert_raw_to_processed_unlocked(chan, *val, val);
+	}
+err_unlock:
+	mutex_unlock(&chan->indio_dev->info_exist_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iio_read_channel_processed);
+
 int iio_read_channel_scale(struct iio_channel *chan, int *val, int *val2)
 {
 	int ret;
@@ -258,10 +370,8 @@ int iio_read_channel_scale(struct iio_channel *chan, int *val, int *val2)
 		goto err_unlock;
 	}
 
-	ret = chan->indio_dev->info->read_raw(chan->indio_dev,
-					      chan->channel,
-					      val, val2,
-					      IIO_CHAN_INFO_SCALE);
+	ret = iio_read_channel_scale_unlocked(chan, val, val2);
+
 err_unlock:
 	mutex_unlock(&chan->indio_dev->info_exist_lock);
 
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index e2657e6..ef99ceb 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -93,4 +93,8 @@ int iio_get_channel_type(struct iio_channel *channel,
 int iio_read_channel_scale(struct iio_channel *chan, int *val,
 			   int *val2);
 
+int iio_read_channel_processed(struct iio_channel *chan, int *val);
+int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
+	int *processed);
+
 #endif
-- 
1.7.10.4

[RFC 3/4] iio:ad5064: Report scale as fractional value

[Lars-Peter Clausen]

Report the scale as a fractional value. This avoids having to do the conversion
from fractional to fixed point manually in the driver.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/dac/ad5064.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/dac/ad5064.c b/drivers/iio/dac/ad5064.c
index eb281a2..a163e86 100644
--- a/drivers/iio/dac/ad5064.c
+++ b/drivers/iio/dac/ad5064.c
@@ -240,10 +240,9 @@ static int ad5064_read_raw(struct iio_dev *indio_dev,
 		if (scale_uv < 0)
 			return scale_uv;
 
-		scale_uv = (scale_uv * 100) >> chan->scan_type.realbits;
-		*val =  scale_uv / 100000;
-		*val2 = (scale_uv % 100000) * 10;
-		return IIO_VAL_INT_PLUS_MICRO;
+		*val = scale_uv;
+		*val2 = ((1 << chan->scan_type.realbits) - 1) * 1000;
+		return IIO_VAL_FRACTIONAL;
 	default:
 		break;
 	}
-- 
1.7.10.4

[RFC 4/4] staging:iio:hwmon bridge: Use iio_read_channel_processed

[Lars-Peter Clausen]

Use the iio_read_channel_processed function to read the sample value in the
proper unit instead of using iio_read_channel_raw and iio_read_channel_scale and
doing the unit conversion manually.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/iio_hwmon.c |   32 ++++----------------------------
 1 file changed, 4 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/iio/iio_hwmon.c b/drivers/staging/iio/iio_hwmon.c
index 27d27ec..1f221eb 100644
--- a/drivers/staging/iio/iio_hwmon.c
+++ b/drivers/staging/iio/iio_hwmon.c
@@ -42,40 +42,16 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
 				  struct device_attribute *attr,
 				  char *buf)
 {
-	long result;
-	int val, ret, scaleint, scalepart;
+	int result;
+	int ret;
 	struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
 	struct iio_hwmon_state *state = dev_get_drvdata(dev);
 
-	/*
-	 * No locking between this pair, so theoretically possible
-	 * the scale has changed.
-	 */
-	ret = iio_read_channel_raw(&state->channels[sattr->index],
-				      &val);
+	ret = iio_read_channel_processed(&state->channels[sattr->index], &result);
 	if (ret < 0)
 		return ret;
 
-	ret = iio_read_channel_scale(&state->channels[sattr->index],
-					&scaleint, &scalepart);
-	if (ret < 0)
-		return ret;
-	switch (ret) {
-	case IIO_VAL_INT:
-		result = val * scaleint;
-		break;
-	case IIO_VAL_INT_PLUS_MICRO:
-		result = (s64)val * (s64)scaleint +
-			div_s64((s64)val * (s64)scalepart, 1000000LL);
-		break;
-	case IIO_VAL_INT_PLUS_NANO:
-		result = (s64)val * (s64)scaleint +
-			div_s64((s64)val * (s64)scalepart, 1000000000LL);
-		break;
-	default:
-		return -EINVAL;
-	}
-	return sprintf(buf, "%ld\n", result);
+	return sprintf(buf, "%d\n", result);
 }
 
 static void iio_hwmon_free_attrs(struct iio_hwmon_state *st)
-- 
1.7.10.4

Re: [RFC 1/4] iio: Introduce a new fractional value type

[Jonathan Cameron]

On 08/16/2012 10:18 AM, Lars-Peter Clausen wrote:
> This series is just a request for comments right now and at this point it is
> barley tested.
> 
> Currently IIO uses a decimal fixed point representations for real type numbers.
> This patch introduces a new representation for rational type numbers. The number
> will be expressed by specifying a numerator and denominator. For converting a
> raw value to a processed value multiply it by the numerator and divide it by the
> denominator.
> 
> The reasoning for introducing this new type is that for a lot of devices the
> scale can be represented easily by a fractional number, but it is not possible
> to represent it as fixed point number without rounding.  E.g. for a simple DAC
> the scale is often the reference voltage divided by the number of possible
> values (Usually 2**n_bits - 1). Each driver currently implements the conversion
> of this fraction to a fixed point number on its own.
> 
> Also when it comes to the in-kernel interface this allows to directly use the
> fractional factors to convert a raw value to a processed value. This should on
> one hand require less instructions and on the other hand increase the
> precession.
Hmm.. Is the range of this going to be large enough?  Or do we need to have
variants of the fractional value?

Otherwise this looks like a reasonable idea to me. Few comments inline.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/iio/industrialio-core.c |    9 +++++++++
>  include/linux/iio/types.h       |    1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 2ec266e..69e60c5 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -365,6 +365,7 @@ static ssize_t iio_read_channel_info(struct device *dev,
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	unsigned long long tmp;
>  	int val, val2;
>  	bool scale_db = false;
>  	int ret = indio_dev->info->read_raw(indio_dev, this_attr->c,
> @@ -390,6 +391,14 @@ static ssize_t iio_read_channel_info(struct device *dev,
>  			return sprintf(buf, "-%d.%09u\n", val, -val2);
>  		else
>  			return sprintf(buf, "%d.%09u\n", val, val2);
> +	case IIO_VAL_FRACTIONAL:
> +		tmp = div_s64((s64)val * 1000000000LL, val2);
> +		val2 = do_div(tmp, 1000000000LL);
> +		val = tmp;
> +		return sprintf(buf, "%d.%09u\n", val, val2);
> +
> +		/* Or maybe we could do something like this:
> +		return sprintf(buf, "%d/%d\n", val, val2); */
Not sure on this one...  Will wait for anyone elses comments.
So far we have pretty printed eveything the same. Hence this will
add a fair bit of complexity to userspace code (and will cause
issues for any existing code!)

>  	default:
>  		return 0;
>  	}
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index 44e3977..8ef5fde 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -57,5 +57,6 @@ enum iio_modifier {
>  #define IIO_VAL_INT_PLUS_MICRO 2
>  #define IIO_VAL_INT_PLUS_NANO 3
>  #define IIO_VAL_INT_PLUS_MICRO_DB 4
> +#define IIO_VAL_FRACTIONAL 5
I wonder if it is worth leaving a gap here for
any more of the int plus form?
>  
>  #endif /* _IIO_TYPES_H_ */
> 

Re: [RFC 2/4] iio:inkern: Add function to read the processed value

[Jonathan Cameron]

On 08/16/2012 10:18 AM, Lars-Peter Clausen wrote:
> Add a function to read a processed value from a channel. The function will first
> attempt to read the IIO_CHAN_INFO_PROCESSED attribute. If that fails it will
> read the IIO_CHAN_INFO_RAW attribute and convert the result from a raw value to
> a processed value.
>
> The patch also introduces a function to convert raw value to a processed value
> and exports it, in case a user needs or wants to do the conversion by itself.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>
All looks sensible.  Needs a bit of tidying up, but otherwise fine.
> ---
> Documentation is still missing.

> ---
>  drivers/iio/inkern.c         |  118 ++++++++++++++++++++++++++++++++++++++++--
>  include/linux/iio/consumer.h |    4 ++
>  2 files changed, 118 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index b5afc2f..9bb3391 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -248,6 +248,118 @@ err_unlock:
>  }
>  EXPORT_SYMBOL_GPL(iio_read_channel_raw);
>
> +static int iio_read_channel_scale_unlocked(struct iio_channel *chan, int *val,
> +	int *val2)
> +{
> +	return chan->indio_dev->info->read_raw(chan->indio_dev,
> +					      chan->channel,
> +					      val, val2,
> +					      IIO_CHAN_INFO_SCALE);
> +}
Not entirely sure these wrappers add much. Still don't do much harm
either. Perhaps unify the two? Maybe we should add null handling
to the core code for when people don't care about the non integer
bit?
> +
> +static int iio_read_channel_offset_unlocked(struct iio_channel *chan, int *val)
> +{
> +	int val2;
> +
> +	return chan->indio_dev->info->read_raw(chan->indio_dev,
> +					      chan->channel,
> +					      val, &val2,
> +					      IIO_CHAN_INFO_OFFSET);
> +}
> +
> +/* It may make sense to a add a extra scale attribute here to avoid precession
> + * loss if the user wants a more fine grained value than the IIO default, e.g.
> + * micro instead of milli volt. */
perhaps...
> +static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
> +	int raw, int *processed)
> +{
> +	int scale_type, scale_val, scale_val2, offset;
> +	s64 raw64 = raw;
> +	int ret;
> +
> +
> +	ret = iio_read_channel_offset_unlocked(chan, &offset);
> +	if (ret == 0)
> +		raw64 += offset;
> +
> +	scale_type = iio_read_channel_scale_unlocked(chan, &scale_val, &scale_val2);
> +	if (scale_type < 0)
> +		return scale_type;
> +
> +	switch (scale_type) {
> +	case IIO_VAL_INT:
> +		*processed = raw * scale_val;
> +		break;
> +	case IIO_VAL_INT_PLUS_MICRO:
> +		if (scale_val2 < 0)
> +			*processed = -raw * scale_val;
> +		else
> +			*processed = raw * scale_val;
> +		*processed += div_s64(raw * (s64)scale_val2, 1000000LL);
> +		break;
> +	case IIO_VAL_INT_PLUS_NANO:
> +		if (scale_val2 < 0)
> +			*processed = -raw * scale_val;
> +		else
> +			*processed = raw * scale_val;
> +		*processed += div_s64(raw * (s64)scale_val2, 1000000000LL);
> +		break;
> +	case IIO_VAL_FRACTIONAL:
> +		*processed = div_s64(raw * (s64)scale_val, scale_val2);
Hmm.. was about to ask why this didn't need the sign test ;)
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +
May seem odd but convert processed to raw would be another useful
utility function (handy for threshold interrupts etc)  Guess we can
add that when needed.
> +int iio_convert_raw_to_processed(struct iio_channel *chan, int raw, int *processed)
> +{
> +	int ret;
> +
> +	mutex_lock(&chan->indio_dev->info_exist_lock);
> +	if (chan->indio_dev->info == NULL) {
> +		ret = -ENODEV;
> +		goto err_unlock;
> +	}
> +
> +	ret = iio_convert_raw_to_processed_unlocked(chan, raw, processed);
> +
> +err_unlock:
> +	mutex_unlock(&chan->indio_dev->info_exist_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iio_convert_raw_to_processed);
> +
> +int iio_read_channel_processed(struct iio_channel *chan, int *val)
> +{
> +	int val2, ret;
> +
> +	mutex_lock(&chan->indio_dev->info_exist_lock);
> +	if (chan->indio_dev->info == NULL) {
> +		ret = -ENODEV;
> +		goto err_unlock;
> +	}
> +
> +	ret = chan->indio_dev->info->read_raw(chan->indio_dev, chan->channel,
> +					      val, &val2, IIO_CHAN_INFO_PROCESSED);
> +	if (ret < 0) {
> +		ret = chan->indio_dev->info->read_raw(chan->indio_dev, chan->channel,
> +					      val, &val2, IIO_CHAN_INFO_RAW);
> +		if (ret < 0)
> +			goto err_unlock;
> +		ret = iio_convert_raw_to_processed_unlocked(chan, *val, val);
> +	}
> +err_unlock:
> +	mutex_unlock(&chan->indio_dev->info_exist_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iio_read_channel_processed);
> +
>  int iio_read_channel_scale(struct iio_channel *chan, int *val, int *val2)
>  {
>  	int ret;
> @@ -258,10 +370,8 @@ int iio_read_channel_scale(struct iio_channel *chan, int *val, int *val2)
>  		goto err_unlock;
>  	}
>
> -	ret = chan->indio_dev->info->read_raw(chan->indio_dev,
> -					      chan->channel,
> -					      val, val2,
> -					      IIO_CHAN_INFO_SCALE);
> +	ret = iio_read_channel_scale_unlocked(chan, val, val2);
> +
>  err_unlock:
>  	mutex_unlock(&chan->indio_dev->info_exist_lock);
>
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index e2657e6..ef99ceb 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -93,4 +93,8 @@ int iio_get_channel_type(struct iio_channel *channel,
>  int iio_read_channel_scale(struct iio_channel *chan, int *val,
>  			   int *val2);
>
> +int iio_read_channel_processed(struct iio_channel *chan, int *val);
> +int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
> +	int *processed);
> +
>  #endif
>

RE: [RFC 2/4] iio:inkern: Add function to read the processed value

[Kim, Milo]

> Add a function to read a processed value from a channel. The function
> will first
> attempt to read the IIO_CHAN_INFO_PROCESSED attribute. If that fails it
> will
> read the IIO_CHAN_INFO_RAW attribute and convert the result from a raw
> value to
> a processed value.

Point of view of the iio consumer driver, this patch is quite useful.
I added some comments below.

> +static int iio_read_channel_offset_unlocked(struct iio_channel *chan,
> int *val)
> +{
> +	int val2;
	Int unused;
> +
> +	return chan->indio_dev->info->read_raw(chan->indio_dev,
> +					      chan->channel,
> +					      val, &val2,
						Val, &unused,
> +					      IIO_CHAN_INFO_OFFSET);
> +}

I'd replace 'val2' with 'unused' because val2 is meaningless variable
in this function.

> +int iio_read_channel_processed(struct iio_channel *chan, int *val)
> +{
> +	int val2, ret;
> +
> +	mutex_lock(&chan->indio_dev->info_exist_lock);
> +	if (chan->indio_dev->info =3D=3D NULL) {
> +		ret =3D -ENODEV;
> +		goto err_unlock;
> +	}
> +
> +	ret =3D chan->indio_dev->info->read_raw(chan->indio_dev, chan-
> >channel,
> +					      val, &val2,
> IIO_CHAN_INFO_PROCESSED);
> +	if (ret < 0) {
> +		ret =3D chan->indio_dev->info->read_raw(chan->indio_dev,
> chan->channel,
> +					      val, &val2, IIO_CHAN_INFO_RAW);
> +		if (ret < 0)
> +			goto err_unlock;
> +		ret =3D iio_convert_raw_to_processed_unlocked(chan, *val,
> val);
> +	}

What about checking info_mask rather than return value from
read_raw() with IIO_CHAN_INFO_PROCESSED?
This way is more understandable for me.
(if PROCESSED is not set, then try to get raw value and convert)

#define IS_IIO_CHAN_INFO_PROCESSED_SET(chan)		\
	chan->channel->info_mask & IIO_CHAN_INFO_PROCESSED_SEPARATE_BIT

int iio_read_channel_processed(struct iio_channel *chan, int *val)
{

...

	if (IS_IIO_CHAN_INFO_PROCESSED_SET(chan)) {
		ret =3D chan->indio_dev->info->read_raw(chan->indio_dev,
						chan->channel,
						val, &val2,
						IIO_CHAN_INFO_PROCESSED);
	} else {
		ret =3D chan->indio_dev->info->read_raw(chan->indio_dev,
						chan->channel,
						val, &val2,
						IIO_CHAN_INFO_RAW);
		if (ret < 0)
			goto err_unlock;

		ret =3D iio_convert_raw_to_processed_unlocked(chan, *val, val);
	}
...

Here also, it's better to replace 'val2' with 'unused'.

Thank you.

Best Regards,
Milo

Re: [RFC 1/4] iio: Introduce a new fractional value type

[Lars-Peter Clausen]

On 08/16/2012 07:17 PM, Jonathan Cameron wrote:
> On 08/16/2012 10:18 AM, Lars-Peter Clausen wrote:
>> This series is just a request for comments right now and at this point it is
>> barley tested.
>>
>> Currently IIO uses a decimal fixed point representations for real type numbers.
>> This patch introduces a new representation for rational type numbers. The number
>> will be expressed by specifying a numerator and denominator. For converting a
>> raw value to a processed value multiply it by the numerator and divide it by the
>> denominator.
>>
>> The reasoning for introducing this new type is that for a lot of devices the
>> scale can be represented easily by a fractional number, but it is not possible
>> to represent it as fixed point number without rounding.  E.g. for a simple DAC
>> the scale is often the reference voltage divided by the number of possible
>> values (Usually 2**n_bits - 1). Each driver currently implements the conversion
>> of this fraction to a fixed point number on its own.
>>
>> Also when it comes to the in-kernel interface this allows to directly use the
>> fractional factors to convert a raw value to a processed value. This should on
>> one hand require less instructions and on the other hand increase the
>> precession.
> Hmm.. Is the range of this going to be large enough?  Or do we need to have
> variants of the fractional value?

Well the range is from +-2147483648 to +-0.0000000005, but of course you can
have any value in between. E.g 2147483647.0000000005 would not be possible,
but it shouldn't be needed. Another advantage of this encoding is that the
smaller your factor is the more effective decimal places you get.

On the other hand though do we really need a scale factor as large as
2147483648 or would 2147483.648 or even 2147.483648 be enough?

> 
> Otherwise this looks like a reasonable idea to me. Few comments inline.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>  drivers/iio/industrialio-core.c |    9 +++++++++
>>  include/linux/iio/types.h       |    1 +
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>> index 2ec266e..69e60c5 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -365,6 +365,7 @@ static ssize_t iio_read_channel_info(struct device *dev,
>>  {
>>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +	unsigned long long tmp;
>>  	int val, val2;
>>  	bool scale_db = false;
>>  	int ret = indio_dev->info->read_raw(indio_dev, this_attr->c,
>> @@ -390,6 +391,14 @@ static ssize_t iio_read_channel_info(struct device *dev,
>>  			return sprintf(buf, "-%d.%09u\n", val, -val2);
>>  		else
>>  			return sprintf(buf, "%d.%09u\n", val, val2);
>> +	case IIO_VAL_FRACTIONAL:
>> +		tmp = div_s64((s64)val * 1000000000LL, val2);
>> +		val2 = do_div(tmp, 1000000000LL);
>> +		val = tmp;
>> +		return sprintf(buf, "%d.%09u\n", val, val2);
>> +
>> +		/* Or maybe we could do something like this:
>> +		return sprintf(buf, "%d/%d\n", val, val2); */
> Not sure on this one...  Will wait for anyone elses comments.
> So far we have pretty printed eveything the same. Hence this will
> add a fair bit of complexity to userspace code (and will cause
> issues for any existing code!)
> 
>>  	default:
>>  		return 0;
>>  	}
>> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
>> index 44e3977..8ef5fde 100644
>> --- a/include/linux/iio/types.h
>> +++ b/include/linux/iio/types.h
>> @@ -57,5 +57,6 @@ enum iio_modifier {
>>  #define IIO_VAL_INT_PLUS_MICRO 2
>>  #define IIO_VAL_INT_PLUS_NANO 3
>>  #define IIO_VAL_INT_PLUS_MICRO_DB 4
>> +#define IIO_VAL_FRACTIONAL 5
> I wonder if it is worth leaving a gap here for
> any more of the int plus form?

I guess it wouldn't hurt.

>>  
>>  #endif /* _IIO_TYPES_H_ */
>>
> --
> 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