[PATCH 0/4] iio: afe/rescale improvements

[PATCH 0/4] iio: afe/rescale improvements

[Paul Cercueil]

Hi Jonathan,

This patchset adds support for converting the "scale avail" list that
may be provided by the source drivers. It also implements the
.write_raw() callback.

These two features will later be used by the
(drivers/power/supply/ingenic-battery.c) driver, which will pick the
best scale possible for the battery's max voltage (it's already
implemented in there, but doesn't yet support iio-rescale being in the
middle).

As you suggested after my RFC I added support for a new
IIO_AVAIL_LIST_WITH_TYPE and it's been a perfect solution, so thank you
for that.

Cheers,
-Paul

Paul Cercueil (4):
  iio: inkern: Remove useless argument to iio_channel_read_max()
  iio: core: Add support for IIO_AVAIL_LIST_WITH_TYPE
  iio: afe/rescale: Add support for converting scale avail table
  iio: afe/rescale: Implement write_raw

 drivers/iio/afe/iio-rescale.c   | 107 ++++++++++++++++++++++++++++++++
 drivers/iio/industrialio-core.c |  25 ++++++++
 drivers/iio/inkern.c            |  35 +++++++++--
 include/linux/iio/afe/rescale.h |   2 +
 include/linux/iio/consumer.h    |   6 +-
 include/linux/iio/types.h       |   1 +
 6 files changed, 168 insertions(+), 8 deletions(-)

-- 
2.35.1

[PATCH 1/4] iio: inkern: Remove useless argument to iio_channel_read_max()

[Paul Cercueil]

The 'type' argument was passed by the only caller of the
iio_channel_read_max() function as a pointer to return an extra value,
but the value of the variable passed by the caller was never read
afterwards.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/iio/inkern.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index df74765d33dc..e8a25852f0df 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -813,21 +813,22 @@ int iio_read_avail_channel_raw(struct iio_channel *chan,
 EXPORT_SYMBOL_GPL(iio_read_avail_channel_raw);
 
 static int iio_channel_read_max(struct iio_channel *chan,
-				int *val, int *val2, int *type,
+				int *val, int *val2,
 				enum iio_chan_info_enum info)
 {
 	int unused;
 	const int *vals;
 	int length;
 	int ret;
+	int type;
 
 	if (!val2)
 		val2 = &unused;
 
-	ret = iio_channel_read_avail(chan, &vals, type, &length, info);
+	ret = iio_channel_read_avail(chan, &vals, &type, &length, info);
 	switch (ret) {
 	case IIO_AVAIL_RANGE:
-		switch (*type) {
+		switch (type) {
 		case IIO_VAL_INT:
 			*val = vals[2];
 			break;
@@ -840,7 +841,7 @@ static int iio_channel_read_max(struct iio_channel *chan,
 	case IIO_AVAIL_LIST:
 		if (length <= 0)
 			return -EINVAL;
-		switch (*type) {
+		switch (type) {
 		case IIO_VAL_INT:
 			*val = vals[--length];
 			while (length) {
@@ -863,7 +864,6 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev);
 	int ret;
-	int type;
 
 	mutex_lock(&iio_dev_opaque->info_exist_lock);
 	if (!chan->indio_dev->info) {
@@ -871,7 +871,7 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val)
 		goto err_unlock;
 	}
 
-	ret = iio_channel_read_max(chan, val, NULL, &type, IIO_CHAN_INFO_RAW);
+	ret = iio_channel_read_max(chan, val, NULL, IIO_CHAN_INFO_RAW);
 err_unlock:
 	mutex_unlock(&iio_dev_opaque->info_exist_lock);
 
-- 
2.35.1

Re: [PATCH 1/4] iio: inkern: Remove useless argument to iio_channel_read_max()

[Jonathan Cameron]

On Thu, 21 Jul 2022 20:15:23 +0100
Paul Cercueil <paul@crapouillou.net> wrote:

> The 'type' argument was passed by the only caller of the
> iio_channel_read_max() function as a pointer to return an extra value,
> but the value of the variable passed by the caller was never read
> afterwards.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Curious. I'm playing guess the intent here and I suspect that what
we should really be doing is checking type in 
iio_read_max_channel_raw() and returning an error if it is not IIO_VAL_INT
(can only get there for IIO_AVAIL_RANGE currently as there is a FIXME for
 IIO_AVAIL_LIST for non IIO_VAL_INT types)

I'm not sure we've ever documented that _RAW can't be non integer even if
that makes relatively little sense most of the time..

Jonathan

> ---
>  drivers/iio/inkern.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index df74765d33dc..e8a25852f0df 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -813,21 +813,22 @@ int iio_read_avail_channel_raw(struct iio_channel *chan,
>  EXPORT_SYMBOL_GPL(iio_read_avail_channel_raw);
>  
>  static int iio_channel_read_max(struct iio_channel *chan,
> -				int *val, int *val2, int *type,
> +				int *val, int *val2,
>  				enum iio_chan_info_enum info)
>  {
>  	int unused;
>  	const int *vals;
>  	int length;
>  	int ret;
> +	int type;
>  
>  	if (!val2)
>  		val2 = &unused;
>  
> -	ret = iio_channel_read_avail(chan, &vals, type, &length, info);
> +	ret = iio_channel_read_avail(chan, &vals, &type, &length, info);
>  	switch (ret) {
>  	case IIO_AVAIL_RANGE:
> -		switch (*type) {
> +		switch (type) {
>  		case IIO_VAL_INT:
>  			*val = vals[2];
>  			break;
> @@ -840,7 +841,7 @@ static int iio_channel_read_max(struct iio_channel *chan,
>  	case IIO_AVAIL_LIST:
>  		if (length <= 0)
>  			return -EINVAL;
> -		switch (*type) {
> +		switch (type) {
>  		case IIO_VAL_INT:
>  			*val = vals[--length];
>  			while (length) {
> @@ -863,7 +864,6 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val)
>  {
>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev);
>  	int ret;
> -	int type;
>  
>  	mutex_lock(&iio_dev_opaque->info_exist_lock);
>  	if (!chan->indio_dev->info) {
> @@ -871,7 +871,7 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val)
>  		goto err_unlock;
>  	}
>  
> -	ret = iio_channel_read_max(chan, val, NULL, &type, IIO_CHAN_INFO_RAW);
> +	ret = iio_channel_read_max(chan, val, NULL, IIO_CHAN_INFO_RAW);
>  err_unlock:
>  	mutex_unlock(&iio_dev_opaque->info_exist_lock);
>  

[PATCH 2/4] iio: core: Add support for IIO_AVAIL_LIST_WITH_TYPE

[Paul Cercueil]

The IIO_AVAIL_LIST_WITH_TYPE specifies that the array that corresponds
to the available values is composed by cells of 3 integers, the first
two representing the value itself (similar to what you would have with
IIO_AVAIL_LIST), and the third integer representing the encoding type of
the value.

This can be used for instance when a driver's .read_avail() callback
returns values which cannot be represented with an unique encoding type.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/iio/industrialio-core.c | 25 +++++++++++++++++++++++++
 drivers/iio/inkern.c            | 23 +++++++++++++++++++++++
 include/linux/iio/consumer.h    |  6 ++++--
 include/linux/iio/types.h       |  1 +
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index adf054c7a75e..99ced9eab069 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -838,6 +838,29 @@ static ssize_t iio_format_avail_range(char *buf, const int *vals, int type)
 	return iio_format_list(buf, vals, type, 3, "[", "]");
 }
 
+static ssize_t iio_format_avail_list_with_type(char *buf, const int *vals,
+					       int length)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < length; i += 3) {
+		if (i != 0) {
+			len += sysfs_emit_at(buf, len, " ");
+			if (len >= PAGE_SIZE)
+				return -EFBIG;
+		}
+
+		len += __iio_format_value(buf, len, vals[i + 2], 2, &vals[i]);
+		if (len >= PAGE_SIZE)
+			return -EFBIG;
+	}
+
+	len += sysfs_emit_at(buf, len, "\n");
+
+	return len;
+}
+
 static ssize_t iio_read_channel_info_avail(struct device *dev,
 					   struct device_attribute *attr,
 					   char *buf)
@@ -860,6 +883,8 @@ static ssize_t iio_read_channel_info_avail(struct device *dev,
 		return iio_format_avail_list(buf, vals, type, length);
 	case IIO_AVAIL_RANGE:
 		return iio_format_avail_range(buf, vals, type);
+	case IIO_AVAIL_LIST_WITH_TYPE:
+		return iio_format_avail_list_with_type(buf, vals, length);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index e8a25852f0df..92d225f1ddd5 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -855,6 +855,29 @@ static int iio_channel_read_max(struct iio_channel *chan,
 		}
 		return 0;
 
+	case IIO_AVAIL_LIST_WITH_TYPE:
+		if (length <= 0 || length % 3 != 0)
+			return -EINVAL;
+
+		if (vals[length - 1] != IIO_VAL_INT) {
+			/* FIXME: learn about max for other iio values */
+			return -EINVAL;
+		}
+
+		*val = vals[length - 3];
+		length -= 3;
+
+		for (; length; length -= 3) {
+			if (vals[length - 1] != IIO_VAL_INT) {
+				/* FIXME: learn about max for other iio values */
+				return -EINVAL;
+			}
+
+			if (vals[length - 3] > *val)
+				*val = vals[length - 3];
+		}
+		return 0;
+
 	default:
 		return ret;
 	}
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 5fa5957586cf..99dd12e10fb6 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -309,7 +309,8 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val);
  * @vals:		Available values read back.
  * @length:		Number of entries in vals.
  *
- * Returns an error code, IIO_AVAIL_RANGE or IIO_AVAIL_LIST.
+ * Returns an error code, IIO_AVAIL_RANGE, IIO_AVAIL_LIST or
+ * IIO_AVAIL_LIST_WITH_TYPE.
  *
  * For ranges, three vals are always returned; min, step and max.
  * For lists, all the possible values are enumerated.
@@ -328,7 +329,8 @@ int iio_read_avail_channel_raw(struct iio_channel *chan,
  * @length:		Number of entries in vals.
  * @attribute:		info attribute to be read back.
  *
- * Returns an error code, IIO_AVAIL_RANGE or IIO_AVAIL_LIST.
+ * Returns an error code, IIO_AVAIL_RANGE, IIO_AVAIL_LIST or
+ * IIO_AVAIL_LIST_WITH_TYPE.
  */
 int iio_read_avail_channel_attribute(struct iio_channel *chan,
 				     const int **vals, int *type, int *length,
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index a7aa91f3a8dc..9777d1357080 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -32,6 +32,7 @@ enum iio_event_info {
 enum iio_available_type {
 	IIO_AVAIL_LIST,
 	IIO_AVAIL_RANGE,
+	IIO_AVAIL_LIST_WITH_TYPE,
 };
 
 enum iio_chan_info_enum {
-- 
2.35.1

Re: [PATCH 2/4] iio: core: Add support for IIO_AVAIL_LIST_WITH_TYPE

[Jonathan Cameron]

On Thu, 21 Jul 2022 20:15:24 +0100
Paul Cercueil <paul@crapouillou.net> wrote:

> The IIO_AVAIL_LIST_WITH_TYPE specifies that the array that corresponds
> to the available values is composed by cells of 3 integers, the first
> two representing the value itself (similar to what you would have with
> IIO_AVAIL_LIST), and the third integer representing the encoding type of
> the value.
> 
> This can be used for instance when a driver's .read_avail() callback
> returns values which cannot be represented with an unique encoding type.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Hi Paul,

Generally looks good to me, but I'm not sure the overflow checks will work
as expected.

Jonathan

> ---
>  drivers/iio/industrialio-core.c | 25 +++++++++++++++++++++++++
>  drivers/iio/inkern.c            | 23 +++++++++++++++++++++++
>  include/linux/iio/consumer.h    |  6 ++++--
>  include/linux/iio/types.h       |  1 +
>  4 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index adf054c7a75e..99ced9eab069 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -838,6 +838,29 @@ static ssize_t iio_format_avail_range(char *buf, const int *vals, int type)
>  	return iio_format_list(buf, vals, type, 3, "[", "]");
>  }
>  
> +static ssize_t iio_format_avail_list_with_type(char *buf, const int *vals,
> +					       int length)
> +{
> +	ssize_t len = 0;
> +	int i;
> +
> +	for (i = 0; i < length; i += 3) {
> +		if (i != 0) {
> +			len += sysfs_emit_at(buf, len, " ");
> +			if (len >= PAGE_SIZE)
I don't think this check will trigger if I read the sysfs_emit_at() docs right.

> +				return -EFBIG;
> +		}
> +
> +		len += __iio_format_value(buf, len, vals[i + 2], 2, &vals[i]);
> +		if (len >= PAGE_SIZE)

Can this trigger?  sysfs_emit_at() inside __iio_format_value() will only return
characters written which I think is none if there isn't space left in the page...

I think the normal thing to do is to just not handle the overflow if it occurs...



> +			return -EFBIG;
> +	}
> +
> +	len += sysfs_emit_at(buf, len, "\n");
> +
> +	return len;
> +}
> +
>  static ssize_t iio_read_channel_info_avail(struct device *dev,
>  					   struct device_attribute *attr,
>  					   char *buf)
> @@ -860,6 +883,8 @@ static ssize_t iio_read_channel_info_avail(struct device *dev,
>  		return iio_format_avail_list(buf, vals, type, length);
>  	case IIO_AVAIL_RANGE:
>  		return iio_format_avail_range(buf, vals, type);
> +	case IIO_AVAIL_LIST_WITH_TYPE:
> +		return iio_format_avail_list_with_type(buf, vals, length);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index e8a25852f0df..92d225f1ddd5 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -855,6 +855,29 @@ static int iio_channel_read_max(struct iio_channel *chan,
>  		}
>  		return 0;
>  
> +	case IIO_AVAIL_LIST_WITH_TYPE:
> +		if (length <= 0 || length % 3 != 0)
> +			return -EINVAL;
> +
> +		if (vals[length - 1] != IIO_VAL_INT) {
> +			/* FIXME: learn about max for other iio values */
> +			return -EINVAL;
> +		}
> +
> +		*val = vals[length - 3];
> +		length -= 3;
> +
> +		for (; length; length -= 3) {
> +			if (vals[length - 1] != IIO_VAL_INT) {
> +				/* FIXME: learn about max for other iio values */
> +				return -EINVAL;
> +			}
> +
> +			if (vals[length - 3] > *val)
> +				*val = vals[length - 3];
> +		}
> +		return 0;
> +
>  	default:
>  		return ret;
>  	}
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index 5fa5957586cf..99dd12e10fb6 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -309,7 +309,8 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val);
>   * @vals:		Available values read back.
>   * @length:		Number of entries in vals.
>   *
> - * Returns an error code, IIO_AVAIL_RANGE or IIO_AVAIL_LIST.
> + * Returns an error code, IIO_AVAIL_RANGE, IIO_AVAIL_LIST or
> + * IIO_AVAIL_LIST_WITH_TYPE.
>   *
>   * For ranges, three vals are always returned; min, step and max.
>   * For lists, all the possible values are enumerated.
> @@ -328,7 +329,8 @@ int iio_read_avail_channel_raw(struct iio_channel *chan,
>   * @length:		Number of entries in vals.
>   * @attribute:		info attribute to be read back.
>   *
> - * Returns an error code, IIO_AVAIL_RANGE or IIO_AVAIL_LIST.
> + * Returns an error code, IIO_AVAIL_RANGE, IIO_AVAIL_LIST or
> + * IIO_AVAIL_LIST_WITH_TYPE.
>   */
>  int iio_read_avail_channel_attribute(struct iio_channel *chan,
>  				     const int **vals, int *type, int *length,
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index a7aa91f3a8dc..9777d1357080 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -32,6 +32,7 @@ enum iio_event_info {
>  enum iio_available_type {
>  	IIO_AVAIL_LIST,
>  	IIO_AVAIL_RANGE,
> +	IIO_AVAIL_LIST_WITH_TYPE,
>  };
>  
>  enum iio_chan_info_enum {

[PATCH 3/4] iio: afe/rescale: Add support for converting scale avail table

[Paul Cercueil]

When the IIO channel has a scale_available attribute, we want the values
contained to be properly converted the same way the scale value is.

Since rescale_process_scale() may change the encoding type, we must
convert the IIO_AVAIL_LIST to a IIO_AVAIL_LIST_WITH_TYPE.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/iio/afe/iio-rescale.c   | 85 +++++++++++++++++++++++++++++++++
 include/linux/iio/afe/rescale.h |  2 +
 2 files changed, 87 insertions(+)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 6949d2151025..5c9970b93384 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -232,6 +232,18 @@ static int rescale_read_avail(struct iio_dev *indio_dev,
 		*type = IIO_VAL_INT;
 		return iio_read_avail_channel_raw(rescale->source,
 						  vals, length);
+	case IIO_CHAN_INFO_SCALE:
+		if (rescale->chan_processed) {
+			return iio_read_avail_channel_attribute(rescale->source,
+								vals, type,
+								length,
+								IIO_CHAN_INFO_SCALE);
+		} else if (rescale->scale_len) {
+			*length = rescale->scale_len;
+			*vals = rescale->scale_data;
+			return IIO_AVAIL_LIST_WITH_TYPE;
+		}
+		fallthrough;
 	default:
 		return -EINVAL;
 	}
@@ -266,11 +278,74 @@ static ssize_t rescale_write_ext_info(struct iio_dev *indio_dev,
 					  buf, len);
 }
 
+static int rescale_init_scale_avail(struct device *dev, struct rescale *rescale)
+{
+	int ret, type, length, *data;
+	const int *scale_raw;
+	unsigned int i;
+	size_t out_len;
+
+	ret = iio_read_avail_channel_attribute(rescale->source, &scale_raw,
+					       &type, &length,
+					       IIO_CHAN_INFO_SCALE);
+	if (ret < 0)
+		return ret;
+
+	switch (ret) {
+	case IIO_AVAIL_LIST_WITH_TYPE:
+		out_len = length;
+		break;
+	case IIO_AVAIL_LIST:
+		if (type == IIO_VAL_INT)
+			out_len = length * 3 / 1;
+		else
+			out_len = length * 3 / 2;
+		break;
+	default:
+		/* TODO: Support IIO_AVAIL_RANGE */
+		return -EOPNOTSUPP;
+	}
+
+	data = devm_kzalloc(dev, sizeof(*data) * out_len, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	if (ret == IIO_AVAIL_LIST_WITH_TYPE) {
+		memcpy(data, scale_raw, sizeof(*scale_raw) * length);
+	} else if (type == IIO_VAL_INT) {
+		for (i = 0; i < length; i++) {
+			data[i * 3 + 0] = scale_raw[i];
+			data[i * 3 + 2] = IIO_VAL_INT;
+		}
+	} else {
+		for (i = 0; i < length / 2; i++) {
+			data[i * 3 + 0] = scale_raw[i * 2];
+			data[i * 3 + 1] = scale_raw[i * 2 + 1];
+			data[i * 3 + 2] = type;
+		}
+	}
+
+	for (i = 0; i < out_len; i += 3) {
+		ret = rescale_process_scale(rescale, data[i + 2],
+					    &data[i], &data[i + 1]);
+		if (ret < 0)
+			return ret;
+
+		data[i + 2] = ret;
+	}
+
+	rescale->scale_len = out_len;
+	rescale->scale_data = data;
+
+	return 0;
+}
+
 static int rescale_configure_channel(struct device *dev,
 				     struct rescale *rescale)
 {
 	struct iio_chan_spec *chan = &rescale->chan;
 	struct iio_chan_spec const *schan = rescale->source->channel;
+	int ret;
 
 	chan->indexed = 1;
 	chan->output = schan->output;
@@ -303,6 +378,16 @@ static int rescale_configure_channel(struct device *dev,
 	    !rescale->chan_processed)
 		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
 
+	if (iio_channel_has_available(schan, IIO_CHAN_INFO_SCALE)) {
+		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_SCALE);
+
+		if (!rescale->chan_processed) {
+			ret = rescale_init_scale_avail(dev, rescale);
+			if (ret)
+				return ret;
+		}
+	}
+
 	return 0;
 }
 
diff --git a/include/linux/iio/afe/rescale.h b/include/linux/iio/afe/rescale.h
index 6eecb435488f..74de2962f864 100644
--- a/include/linux/iio/afe/rescale.h
+++ b/include/linux/iio/afe/rescale.h
@@ -26,6 +26,8 @@ struct rescale {
 	s32 numerator;
 	s32 denominator;
 	s32 offset;
+	int scale_len;
+	int *scale_data;
 };
 
 int rescale_process_scale(struct rescale *rescale, int scale_type,
-- 
2.35.1

Re: [PATCH 3/4] iio: afe/rescale: Add support for converting scale avail table

[Peter Rosin]

Hi!

2022-07-21 at 21:15, Paul Cercueil wrote:
> When the IIO channel has a scale_available attribute, we want the values
> contained to be properly converted the same way the scale value is.
> 
> Since rescale_process_scale() may change the encoding type, we must
> convert the IIO_AVAIL_LIST to a IIO_AVAIL_LIST_WITH_TYPE.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/iio/afe/iio-rescale.c   | 85 +++++++++++++++++++++++++++++++++
>  include/linux/iio/afe/rescale.h |  2 +
>  2 files changed, 87 insertions(+)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 6949d2151025..5c9970b93384 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -232,6 +232,18 @@ static int rescale_read_avail(struct iio_dev *indio_dev,
>  		*type = IIO_VAL_INT;
>  		return iio_read_avail_channel_raw(rescale->source,
>  						  vals, length);
> +	case IIO_CHAN_INFO_SCALE:
> +		if (rescale->chan_processed) {

I think it is wrong to simply feed the info-scale to the source channel if it
happens to be processed. It still needs the inverse rescale. But see below.

> +			return iio_read_avail_channel_attribute(rescale->source,
> +								vals, type,
> +								length,
> +								IIO_CHAN_INFO_SCALE);
> +		} else if (rescale->scale_len) {
> +			*length = rescale->scale_len;
> +			*vals = rescale->scale_data;
> +			return IIO_AVAIL_LIST_WITH_TYPE;
> +		}
> +		fallthrough;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -266,11 +278,74 @@ static ssize_t rescale_write_ext_info(struct iio_dev *indio_dev,
>  					  buf, len);
>  }
>  
> +static int rescale_init_scale_avail(struct device *dev, struct rescale *rescale)
> +{
> +	int ret, type, length, *data;
> +	const int *scale_raw;
> +	unsigned int i;
> +	size_t out_len;
> +
> +	ret = iio_read_avail_channel_attribute(rescale->source, &scale_raw,
> +					       &type, &length,
> +					       IIO_CHAN_INFO_SCALE);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (ret) {
> +	case IIO_AVAIL_LIST_WITH_TYPE:
> +		out_len = length;
> +		break;
> +	case IIO_AVAIL_LIST:
> +		if (type == IIO_VAL_INT)
> +			out_len = length * 3 / 1;
> +		else
> +			out_len = length * 3 / 2;
> +		break;
> +	default:
> +		/* TODO: Support IIO_AVAIL_RANGE */
> +		return -EOPNOTSUPP;
> +	}
> +
> +	data = devm_kzalloc(dev, sizeof(*data) * out_len, GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	if (ret == IIO_AVAIL_LIST_WITH_TYPE) {
> +		memcpy(data, scale_raw, sizeof(*scale_raw) * length);
> +	} else if (type == IIO_VAL_INT) {
> +		for (i = 0; i < length; i++) {
> +			data[i * 3 + 0] = scale_raw[i];
> +			data[i * 3 + 2] = IIO_VAL_INT;
> +		}
> +	} else {
> +		for (i = 0; i < length / 2; i++) {
> +			data[i * 3 + 0] = scale_raw[i * 2];
> +			data[i * 3 + 1] = scale_raw[i * 2 + 1];
> +			data[i * 3 + 2] = type;
> +		}
> +	}
> +
> +	for (i = 0; i < out_len; i += 3) {
> +		ret = rescale_process_scale(rescale, data[i + 2],
> +					    &data[i], &data[i + 1]);
> +		if (ret < 0)
> +			return ret;
> +
> +		data[i + 2] = ret;
> +	}
> +
> +	rescale->scale_len = out_len;
> +	rescale->scale_data = data;
> +
> +	return 0;
> +}
> +
>  static int rescale_configure_channel(struct device *dev,
>  				     struct rescale *rescale)
>  {
>  	struct iio_chan_spec *chan = &rescale->chan;
>  	struct iio_chan_spec const *schan = rescale->source->channel;
> +	int ret;
>  
>  	chan->indexed = 1;
>  	chan->output = schan->output;
> @@ -303,6 +378,16 @@ static int rescale_configure_channel(struct device *dev,
>  	    !rescale->chan_processed)
>  		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
>  
> +	if (iio_channel_has_available(schan, IIO_CHAN_INFO_SCALE)) {
> +		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_SCALE);
> +
> +		if (!rescale->chan_processed) {
> +			ret = rescale_init_scale_avail(dev, rescale);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +

Does a (sane) processed channel really have a scale? That seems a bit fringe.
Wouldn't it be better to conditionally publish availability of info-scale so
that it isn't visible for processed channels and dodge that rabbit-hole? In
either case, the above commented implementation of info-scale for rescaled
processed channels is wrong (I think...).

Cheers,
Peter

>  	return 0;
>  }
>  
> diff --git a/include/linux/iio/afe/rescale.h b/include/linux/iio/afe/rescale.h
> index 6eecb435488f..74de2962f864 100644
> --- a/include/linux/iio/afe/rescale.h
> +++ b/include/linux/iio/afe/rescale.h
> @@ -26,6 +26,8 @@ struct rescale {
>  	s32 numerator;
>  	s32 denominator;
>  	s32 offset;
> +	int scale_len;
> +	int *scale_data;
>  };
>  
>  int rescale_process_scale(struct rescale *rescale, int scale_type,

Re: [PATCH 3/4] iio: afe/rescale: Add support for converting scale avail table

[Paul Cercueil]

Hi Peter,

Le ven., juil. 22 2022 at 00:16:31 +0200, Peter Rosin <peda@axentia.se> 
a écrit :
> Hi!
> 
> 2022-07-21 at 21:15, Paul Cercueil wrote:
>>  When the IIO channel has a scale_available attribute, we want the 
>> values
>>  contained to be properly converted the same way the scale value is.
>> 
>>  Since rescale_process_scale() may change the encoding type, we must
>>  convert the IIO_AVAIL_LIST to a IIO_AVAIL_LIST_WITH_TYPE.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/iio/afe/iio-rescale.c   | 85 
>> +++++++++++++++++++++++++++++++++
>>   include/linux/iio/afe/rescale.h |  2 +
>>   2 files changed, 87 insertions(+)
>> 
>>  diff --git a/drivers/iio/afe/iio-rescale.c 
>> b/drivers/iio/afe/iio-rescale.c
>>  index 6949d2151025..5c9970b93384 100644
>>  --- a/drivers/iio/afe/iio-rescale.c
>>  +++ b/drivers/iio/afe/iio-rescale.c
>>  @@ -232,6 +232,18 @@ static int rescale_read_avail(struct iio_dev 
>> *indio_dev,
>>   		*type = IIO_VAL_INT;
>>   		return iio_read_avail_channel_raw(rescale->source,
>>   						  vals, length);
>>  +	case IIO_CHAN_INFO_SCALE:
>>  +		if (rescale->chan_processed) {
> 
> I think it is wrong to simply feed the info-scale to the source 
> channel if it
> happens to be processed. It still needs the inverse rescale. But see 
> below.

Yes, when I started working on that patchset, processed channels 
weren't a thing, and I don't think I understood what they are about.

> 
>>  +			return iio_read_avail_channel_attribute(rescale->source,
>>  +								vals, type,
>>  +								length,
>>  +								IIO_CHAN_INFO_SCALE);
>>  +		} else if (rescale->scale_len) {
>>  +			*length = rescale->scale_len;
>>  +			*vals = rescale->scale_data;
>>  +			return IIO_AVAIL_LIST_WITH_TYPE;
>>  +		}
>>  +		fallthrough;
>>   	default:
>>   		return -EINVAL;
>>   	}
>>  @@ -266,11 +278,74 @@ static ssize_t rescale_write_ext_info(struct 
>> iio_dev *indio_dev,
>>   					  buf, len);
>>   }
>> 
>>  +static int rescale_init_scale_avail(struct device *dev, struct 
>> rescale *rescale)
>>  +{
>>  +	int ret, type, length, *data;
>>  +	const int *scale_raw;
>>  +	unsigned int i;
>>  +	size_t out_len;
>>  +
>>  +	ret = iio_read_avail_channel_attribute(rescale->source, 
>> &scale_raw,
>>  +					       &type, &length,
>>  +					       IIO_CHAN_INFO_SCALE);
>>  +	if (ret < 0)
>>  +		return ret;
>>  +
>>  +	switch (ret) {
>>  +	case IIO_AVAIL_LIST_WITH_TYPE:
>>  +		out_len = length;
>>  +		break;
>>  +	case IIO_AVAIL_LIST:
>>  +		if (type == IIO_VAL_INT)
>>  +			out_len = length * 3 / 1;
>>  +		else
>>  +			out_len = length * 3 / 2;
>>  +		break;
>>  +	default:
>>  +		/* TODO: Support IIO_AVAIL_RANGE */
>>  +		return -EOPNOTSUPP;
>>  +	}
>>  +
>>  +	data = devm_kzalloc(dev, sizeof(*data) * out_len, GFP_KERNEL);
>>  +	if (!data)
>>  +		return -ENOMEM;
>>  +
>>  +	if (ret == IIO_AVAIL_LIST_WITH_TYPE) {
>>  +		memcpy(data, scale_raw, sizeof(*scale_raw) * length);
>>  +	} else if (type == IIO_VAL_INT) {
>>  +		for (i = 0; i < length; i++) {
>>  +			data[i * 3 + 0] = scale_raw[i];
>>  +			data[i * 3 + 2] = IIO_VAL_INT;
>>  +		}
>>  +	} else {
>>  +		for (i = 0; i < length / 2; i++) {
>>  +			data[i * 3 + 0] = scale_raw[i * 2];
>>  +			data[i * 3 + 1] = scale_raw[i * 2 + 1];
>>  +			data[i * 3 + 2] = type;
>>  +		}
>>  +	}
>>  +
>>  +	for (i = 0; i < out_len; i += 3) {
>>  +		ret = rescale_process_scale(rescale, data[i + 2],
>>  +					    &data[i], &data[i + 1]);
>>  +		if (ret < 0)
>>  +			return ret;
>>  +
>>  +		data[i + 2] = ret;
>>  +	}
>>  +
>>  +	rescale->scale_len = out_len;
>>  +	rescale->scale_data = data;
>>  +
>>  +	return 0;
>>  +}
>>  +
>>   static int rescale_configure_channel(struct device *dev,
>>   				     struct rescale *rescale)
>>   {
>>   	struct iio_chan_spec *chan = &rescale->chan;
>>   	struct iio_chan_spec const *schan = rescale->source->channel;
>>  +	int ret;
>> 
>>   	chan->indexed = 1;
>>   	chan->output = schan->output;
>>  @@ -303,6 +378,16 @@ static int rescale_configure_channel(struct 
>> device *dev,
>>   	    !rescale->chan_processed)
>>   		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
>> 
>>  +	if (iio_channel_has_available(schan, IIO_CHAN_INFO_SCALE)) {
>>  +		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_SCALE);
>>  +
>>  +		if (!rescale->chan_processed) {
>>  +			ret = rescale_init_scale_avail(dev, rescale);
>>  +			if (ret)
>>  +				return ret;
>>  +		}
>>  +	}
>>  +
> 
> Does a (sane) processed channel really have a scale? That seems a bit 
> fringe.
> Wouldn't it be better to conditionally publish availability of 
> info-scale so
> that it isn't visible for processed channels and dodge that 
> rabbit-hole? In
> either case, the above commented implementation of info-scale for 
> rescaled
> processed channels is wrong (I think...).

I could set the IIO_CHAN_INFO_SCALE only for non-processed channels, 
since this is what I can test with.

Cheers,
-Paul

>>   	return 0;
>>   }
>> 
>>  diff --git a/include/linux/iio/afe/rescale.h 
>> b/include/linux/iio/afe/rescale.h
>>  index 6eecb435488f..74de2962f864 100644
>>  --- a/include/linux/iio/afe/rescale.h
>>  +++ b/include/linux/iio/afe/rescale.h
>>  @@ -26,6 +26,8 @@ struct rescale {
>>   	s32 numerator;
>>   	s32 denominator;
>>   	s32 offset;
>>  +	int scale_len;
>>  +	int *scale_data;
>>   };
>> 
>>   int rescale_process_scale(struct rescale *rescale, int scale_type,

Re: [PATCH 3/4] iio: afe/rescale: Add support for converting scale avail table

[Jonathan Cameron]

On Fri, 22 Jul 2022 09:52:34 +0100
Paul Cercueil <paul@crapouillou.net> wrote:

> Hi Peter,
> 
> Le ven., juil. 22 2022 at 00:16:31 +0200, Peter Rosin <peda@axentia.se> 
> a écrit :
> > Hi!
> > 
> > 2022-07-21 at 21:15, Paul Cercueil wrote:  
> >>  When the IIO channel has a scale_available attribute, we want the 
> >> values
> >>  contained to be properly converted the same way the scale value is.
> >> 
> >>  Since rescale_process_scale() may change the encoding type, we must
> >>  convert the IIO_AVAIL_LIST to a IIO_AVAIL_LIST_WITH_TYPE.
> >> 
> >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >>  ---
> >>   drivers/iio/afe/iio-rescale.c   | 85 
> >> +++++++++++++++++++++++++++++++++
> >>   include/linux/iio/afe/rescale.h |  2 +
> >>   2 files changed, 87 insertions(+)
> >> 
> >>  diff --git a/drivers/iio/afe/iio-rescale.c 
> >> b/drivers/iio/afe/iio-rescale.c
> >>  index 6949d2151025..5c9970b93384 100644
> >>  --- a/drivers/iio/afe/iio-rescale.c
> >>  +++ b/drivers/iio/afe/iio-rescale.c
> >>  @@ -232,6 +232,18 @@ static int rescale_read_avail(struct iio_dev 
> >> *indio_dev,
> >>   		*type = IIO_VAL_INT;
> >>   		return iio_read_avail_channel_raw(rescale->source,
> >>   						  vals, length);
> >>  +	case IIO_CHAN_INFO_SCALE:
> >>  +		if (rescale->chan_processed) {  
> > 
> > I think it is wrong to simply feed the info-scale to the source 
> > channel if it
> > happens to be processed. It still needs the inverse rescale. But see 
> > below.  
> 
> Yes, when I started working on that patchset, processed channels 
> weren't a thing, and I don't think I understood what they are about.
> 
> >   
> >>  +			return iio_read_avail_channel_attribute(rescale->source,
> >>  +								vals, type,
> >>  +								length,
> >>  +								IIO_CHAN_INFO_SCALE);
> >>  +		} else if (rescale->scale_len) {
> >>  +			*length = rescale->scale_len;
> >>  +			*vals = rescale->scale_data;
> >>  +			return IIO_AVAIL_LIST_WITH_TYPE;
> >>  +		}
> >>  +		fallthrough;
> >>   	default:
> >>   		return -EINVAL;
> >>   	}
> >>  @@ -266,11 +278,74 @@ static ssize_t rescale_write_ext_info(struct 
> >> iio_dev *indio_dev,
> >>   					  buf, len);
> >>   }
> >> 
> >>  +static int rescale_init_scale_avail(struct device *dev, struct 
> >> rescale *rescale)
> >>  +{
> >>  +	int ret, type, length, *data;
> >>  +	const int *scale_raw;
> >>  +	unsigned int i;
> >>  +	size_t out_len;
> >>  +
> >>  +	ret = iio_read_avail_channel_attribute(rescale->source, 
> >> &scale_raw,
> >>  +					       &type, &length,
> >>  +					       IIO_CHAN_INFO_SCALE);
> >>  +	if (ret < 0)
> >>  +		return ret;
> >>  +
> >>  +	switch (ret) {
> >>  +	case IIO_AVAIL_LIST_WITH_TYPE:
> >>  +		out_len = length;
> >>  +		break;
> >>  +	case IIO_AVAIL_LIST:
> >>  +		if (type == IIO_VAL_INT)
> >>  +			out_len = length * 3 / 1;
> >>  +		else
> >>  +			out_len = length * 3 / 2;
> >>  +		break;
> >>  +	default:
> >>  +		/* TODO: Support IIO_AVAIL_RANGE */
> >>  +		return -EOPNOTSUPP;
> >>  +	}
> >>  +
> >>  +	data = devm_kzalloc(dev, sizeof(*data) * out_len, GFP_KERNEL);
> >>  +	if (!data)
> >>  +		return -ENOMEM;
> >>  +
> >>  +	if (ret == IIO_AVAIL_LIST_WITH_TYPE) {
> >>  +		memcpy(data, scale_raw, sizeof(*scale_raw) * length);
> >>  +	} else if (type == IIO_VAL_INT) {
> >>  +		for (i = 0; i < length; i++) {
> >>  +			data[i * 3 + 0] = scale_raw[i];
> >>  +			data[i * 3 + 2] = IIO_VAL_INT;
> >>  +		}
> >>  +	} else {
> >>  +		for (i = 0; i < length / 2; i++) {
> >>  +			data[i * 3 + 0] = scale_raw[i * 2];
> >>  +			data[i * 3 + 1] = scale_raw[i * 2 + 1];
> >>  +			data[i * 3 + 2] = type;
> >>  +		}
> >>  +	}
> >>  +
> >>  +	for (i = 0; i < out_len; i += 3) {
> >>  +		ret = rescale_process_scale(rescale, data[i + 2],
> >>  +					    &data[i], &data[i + 1]);
> >>  +		if (ret < 0)
> >>  +			return ret;
> >>  +
> >>  +		data[i + 2] = ret;
> >>  +	}
> >>  +
> >>  +	rescale->scale_len = out_len;
> >>  +	rescale->scale_data = data;
> >>  +
> >>  +	return 0;
> >>  +}
> >>  +
> >>   static int rescale_configure_channel(struct device *dev,
> >>   				     struct rescale *rescale)
> >>   {
> >>   	struct iio_chan_spec *chan = &rescale->chan;
> >>   	struct iio_chan_spec const *schan = rescale->source->channel;
> >>  +	int ret;
> >> 
> >>   	chan->indexed = 1;
> >>   	chan->output = schan->output;
> >>  @@ -303,6 +378,16 @@ static int rescale_configure_channel(struct 
> >> device *dev,
> >>   	    !rescale->chan_processed)
> >>   		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
> >> 
> >>  +	if (iio_channel_has_available(schan, IIO_CHAN_INFO_SCALE)) {
> >>  +		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_SCALE);
> >>  +
> >>  +		if (!rescale->chan_processed) {
> >>  +			ret = rescale_init_scale_avail(dev, rescale);
> >>  +			if (ret)
> >>  +				return ret;
> >>  +		}
> >>  +	}
> >>  +  
> > 
> > Does a (sane) processed channel really have a scale? That seems a bit 
> > fringe.
> > Wouldn't it be better to conditionally publish availability of 
> > info-scale so
> > that it isn't visible for processed channels and dodge that 
> > rabbit-hole? In
> > either case, the above commented implementation of info-scale for 
> > rescaled
> > processed channels is wrong (I think...).  
> 
> I could set the IIO_CHAN_INFO_SCALE only for non-processed channels, 
> since this is what I can test with.

Indeed both nonsensical for PROCESSED channels to have a scale and in the
rare corner case where it happens you shouldn't apply it anyway.

I'm struggling to think of when it might happen due to maintaining
backwards compatibility and similar (the reason we have channels with
both PROCESSED and RAW) but in those cases I don't think we'd have
SCALE because they tend to be devices with nasty non linear transfer
functions where SCALE isn't appropriate.

Jonathan


> 
> Cheers,
> -Paul
> 
> >>   	return 0;
> >>   }
> >> 
> >>  diff --git a/include/linux/iio/afe/rescale.h 
> >> b/include/linux/iio/afe/rescale.h
> >>  index 6eecb435488f..74de2962f864 100644
> >>  --- a/include/linux/iio/afe/rescale.h
> >>  +++ b/include/linux/iio/afe/rescale.h
> >>  @@ -26,6 +26,8 @@ struct rescale {
> >>   	s32 numerator;
> >>   	s32 denominator;
> >>   	s32 offset;
> >>  +	int scale_len;
> >>  +	int *scale_data;
> >>   };
> >> 
> >>   int rescale_process_scale(struct rescale *rescale, int scale_type,  
> 
> 

[PATCH 4/4] iio: afe/rescale: Implement write_raw

[Paul Cercueil]

Implement write_raw by converting the value if writing the scale, or
just calling the managed channel driver's write_raw otherwise.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/iio/afe/iio-rescale.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 5c9970b93384..0edb62ee4508 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -141,6 +141,27 @@ int rescale_process_offset(struct rescale *rescale, int scale_type,
 	}
 }
 
+static int rescale_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct rescale *rescale = iio_priv(indio_dev);
+	unsigned long long tmp;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		tmp = val * 1000000000LL;
+		do_div(tmp, rescale->numerator);
+		tmp *= rescale->denominator;
+		do_div(tmp, 1000000000LL);
+		return iio_write_channel_attribute(rescale->source, tmp, 0,
+						   IIO_CHAN_INFO_SCALE);
+	default:
+		return iio_write_channel_attribute(rescale->source,
+						   val, val2, mask);
+	}
+}
+
 static int rescale_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -250,6 +271,7 @@ static int rescale_read_avail(struct iio_dev *indio_dev,
 }
 
 static const struct iio_info rescale_info = {
+	.write_raw = rescale_write_raw,
 	.read_raw = rescale_read_raw,
 	.read_avail = rescale_read_avail,
 };
-- 
2.35.1

Re: [PATCH 4/4] iio: afe/rescale: Implement write_raw

[Peter Rosin]

Hi!

2022-07-21 at 21:15, Paul Cercueil wrote:
> Implement write_raw by converting the value if writing the scale, or
> just calling the managed channel driver's write_raw otherwise.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/iio/afe/iio-rescale.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 5c9970b93384..0edb62ee4508 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -141,6 +141,27 @@ int rescale_process_offset(struct rescale *rescale, int scale_type,
>  	}
>  }
>  
> +static int rescale_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct rescale *rescale = iio_priv(indio_dev);
> +	unsigned long long tmp;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		tmp = val * 1000000000LL;
> +		do_div(tmp, rescale->numerator);
> +		tmp *= rescale->denominator;
> +		do_div(tmp, 1000000000LL);

do_div is for unsigned operands. Can val never ever be negative?
What about the numerator and denominator, can those be negative? I
think this code should live in a new rescale_process_inverse_scale
function, or something like that (and a few tests could be added to
drivers/iio/test/iio-test-rescale.c)

> +		return iio_write_channel_attribute(rescale->source, tmp, 0,
> +						   IIO_CHAN_INFO_SCALE);
> +	default:

What if the source driver has a .write_raw_get_fmt callback? That bit
of info is silently dropped (with no comment that a shortcut has been
taken). How does inverse rescaling mix with a .write_raw_get_fmt that
returns e.g. IIO_VAL_INT_PLUS_MICRO_DB anyway? I think all cases might
get a bit hairy to support, so I think you need to do some filtering
and somehow fail the .write_raw call if the .write_raw_get_fmt of the
source returns something that gets too difficult to support.

Cheers,
Peter

> +		return iio_write_channel_attribute(rescale->source,
> +						   val, val2, mask);
> +	}
> +}
> +
>  static int rescale_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
> @@ -250,6 +271,7 @@ static int rescale_read_avail(struct iio_dev *indio_dev,
>  }
>  
>  static const struct iio_info rescale_info = {
> +	.write_raw = rescale_write_raw,
>  	.read_raw = rescale_read_raw,
>  	.read_avail = rescale_read_avail,
>  };

Re: [PATCH 4/4] iio: afe/rescale: Implement write_raw

[Paul Cercueil]

Hi Peter,

Le ven., juil. 22 2022 at 00:16:36 +0200, Peter Rosin <peda@axentia.se> 
a écrit :
> Hi!
> 
> 2022-07-21 at 21:15, Paul Cercueil wrote:
>>  Implement write_raw by converting the value if writing the scale, or
>>  just calling the managed channel driver's write_raw otherwise.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/iio/afe/iio-rescale.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>> 
>>  diff --git a/drivers/iio/afe/iio-rescale.c 
>> b/drivers/iio/afe/iio-rescale.c
>>  index 5c9970b93384..0edb62ee4508 100644
>>  --- a/drivers/iio/afe/iio-rescale.c
>>  +++ b/drivers/iio/afe/iio-rescale.c
>>  @@ -141,6 +141,27 @@ int rescale_process_offset(struct rescale 
>> *rescale, int scale_type,
>>   	}
>>   }
>> 
>>  +static int rescale_write_raw(struct iio_dev *indio_dev,
>>  +			     struct iio_chan_spec const *chan,
>>  +			     int val, int val2, long mask)
>>  +{
>>  +	struct rescale *rescale = iio_priv(indio_dev);
>>  +	unsigned long long tmp;
>>  +
>>  +	switch (mask) {
>>  +	case IIO_CHAN_INFO_SCALE:
>>  +		tmp = val * 1000000000LL;
>>  +		do_div(tmp, rescale->numerator);
>>  +		tmp *= rescale->denominator;
>>  +		do_div(tmp, 1000000000LL);
> 
> do_div is for unsigned operands. Can val never ever be negative?
> What about the numerator and denominator, can those be negative? I
> think this code should live in a new rescale_process_inverse_scale
> function, or something like that (and a few tests could be added to
> drivers/iio/test/iio-test-rescale.c)

I can do that.

> 
>>  +		return iio_write_channel_attribute(rescale->source, tmp, 0,
>>  +						   IIO_CHAN_INFO_SCALE);
>>  +	default:
> 
> What if the source driver has a .write_raw_get_fmt callback? That bit
> of info is silently dropped (with no comment that a shortcut has been
> taken). How does inverse rescaling mix with a .write_raw_get_fmt that
> returns e.g. IIO_VAL_INT_PLUS_MICRO_DB anyway? I think all cases might
> get a bit hairy to support, so I think you need to do some filtering
> and somehow fail the .write_raw call if the .write_raw_get_fmt of the
> source returns something that gets too difficult to support.

If the inverse rescale uses the same code as rescale_process_scale() 
then it becomes problematic, yes, as it likes to change the type of the 
value.

What I could try - compute the inverse of the value, then find the 
closest scale value+type that the source driver supports, and use this 
as the value+type. Then the only failure point would be if 
.write_raw_get_fmt returns something different than the formats 
returned by .read_avail, but that sounds unlikely to happen.

Cheers,
-Paul

>>  +		return iio_write_channel_attribute(rescale->source,
>>  +						   val, val2, mask);
>>  +	}
>>  +}
>>  +
>>   static int rescale_read_raw(struct iio_dev *indio_dev,
>>   			    struct iio_chan_spec const *chan,
>>   			    int *val, int *val2, long mask)
>>  @@ -250,6 +271,7 @@ static int rescale_read_avail(struct iio_dev 
>> *indio_dev,
>>   }
>> 
>>   static const struct iio_info rescale_info = {
>>  +	.write_raw = rescale_write_raw,
>>   	.read_raw = rescale_read_raw,
>>   	.read_avail = rescale_read_avail,
>>   };