[PATCH v1 0/3] iio: afe: rescale: Add INFO_PROCESSED support

[PATCH v1 0/3] iio: afe: rescale: Add INFO_PROCESSED support

[Mylène Josserand]

Hello everyone,

You will find a small series that add the support of processed values
for iio-rescale driver.
Thanks to that, it is possible to read processed values in sysfs instead
of getting only raw and scale values.

Here is an example for a 3v3 voltage reading:
# cat /sys/bus/iio/devices/iio\:device1/in_voltage0_scale
3.791015625
# cat /sys/bus/iio/devices/iio\:device1/in_voltage0_raw
879
# cat /sys/bus/iio/devices/iio\:device1/in_voltage0_input
3332

It is also possible to read directly the processed values using iio-hwmon
driver (see example in patch03):

# cat /sys/class/hwmon/hwmon0/in1_input
3328

I seperated my series in 3 patches:
   - Patch01: Move the scale conversion into a function to prepare the
   support of IIO_CHAN_INFO_PROCESSED.
   - Patch02: Add the support of IIO_CHAN_INFO_PROCESSED.
   - Patch03: Add an example of the use of hwmon and voltage-divider nodes
   in device-tree.

If you have any feedbacks on it, I will be pleased to read them!

Best regards,
Mylène

Mylène Josserand (3):
  iio: afe: rescale: Move scale conversion to new function
  iio: afe: rescale: Add support of CHAN_INFO_PROCESSED
  dt-bindings: iio: afe: Add hwmon example

 .../bindings/iio/afe/voltage-divider.txt           | 24 ++++++
 drivers/iio/afe/iio-rescale.c                      | 96 ++++++++++++++++------
 2 files changed, 96 insertions(+), 24 deletions(-)

-- 
2.11.0

[PATCH v1 1/3] iio: afe: rescale: Move scale conversion to new function

[Mylène Josserand]

To prepare the support of processed value, create a function
to convert the scale according to the voltage-divider node
used in the device-tree.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 drivers/iio/afe/iio-rescale.c | 54 +++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index e9ceee66d1e7..3e689d6eb501 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -33,12 +33,41 @@ struct rescale {
 	s32 denominator;
 };
 
+static int rescale_convert_scale(struct rescale *rescale, int *val, int *val2)
+{
+	unsigned long long tmp;
+	int ret;
+
+	ret = iio_read_channel_scale(rescale->source, val, val2);
+	switch (ret) {
+	case IIO_VAL_FRACTIONAL:
+		*val *= rescale->numerator;
+		*val2 *= rescale->denominator;
+		return ret;
+	case IIO_VAL_INT:
+		*val *= rescale->numerator;
+		if (rescale->denominator == 1)
+			return ret;
+		*val2 = rescale->denominator;
+		return IIO_VAL_FRACTIONAL;
+	case IIO_VAL_FRACTIONAL_LOG2:
+		tmp = *val * 1000000000LL;
+		do_div(tmp, rescale->denominator);
+		tmp *= rescale->numerator;
+		do_div(tmp, 1000000000LL);
+		*val = tmp;
+
+		return ret;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int rescale_read_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;
 	int ret;
 
 	switch (mask) {
@@ -46,28 +75,7 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
 		return iio_read_channel_raw(rescale->source, val);
 
 	case IIO_CHAN_INFO_SCALE:
-		ret = iio_read_channel_scale(rescale->source, val, val2);
-		switch (ret) {
-		case IIO_VAL_FRACTIONAL:
-			*val *= rescale->numerator;
-			*val2 *= rescale->denominator;
-			return ret;
-		case IIO_VAL_INT:
-			*val *= rescale->numerator;
-			if (rescale->denominator == 1)
-				return ret;
-			*val2 = rescale->denominator;
-			return IIO_VAL_FRACTIONAL;
-		case IIO_VAL_FRACTIONAL_LOG2:
-			tmp = *val * 1000000000LL;
-			do_div(tmp, rescale->denominator);
-			tmp *= rescale->numerator;
-			do_div(tmp, 1000000000LL);
-			*val = tmp;
-			return ret;
-		default:
-			return -EOPNOTSUPP;
-		}
+		return rescale_convert_scale(rescale, val, val2);
 	default:
 		return -EINVAL;
 	}
-- 
2.11.0

Re: [PATCH v1 1/3] iio: afe: rescale: Move scale conversion to new function

[Peter Rosin]

On 2019-06-11 11:56, Mylène Josserand wrote:
> To prepare the support of processed value, create a function
> to convert the scale according to the voltage-divider node
> used in the device-tree.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> ---
>  drivers/iio/afe/iio-rescale.c | 54 +++++++++++++++++++++++++------------------
>  1 file changed, 31 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index e9ceee66d1e7..3e689d6eb501 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -33,12 +33,41 @@ struct rescale {
>  	s32 denominator;
>  };
>  
> +static int rescale_convert_scale(struct rescale *rescale, int *val, int *val2)
> +{
> +	unsigned long long tmp;
> +	int ret;
> +
> +	ret = iio_read_channel_scale(rescale->source, val, val2);
> +	switch (ret) {
> +	case IIO_VAL_FRACTIONAL:
> +		*val *= rescale->numerator;
> +		*val2 *= rescale->denominator;
> +		return ret;
> +	case IIO_VAL_INT:
> +		*val *= rescale->numerator;
> +		if (rescale->denominator == 1)
> +			return ret;
> +		*val2 = rescale->denominator;
> +		return IIO_VAL_FRACTIONAL;
> +	case IIO_VAL_FRACTIONAL_LOG2:
> +		tmp = *val * 1000000000LL;
> +		do_div(tmp, rescale->denominator);
> +		tmp *= rescale->numerator;
> +		do_div(tmp, 1000000000LL);
> +		*val = tmp;
> +

This blank line is in conflict with the style of the surrounding code.

Cheers,
Peter

> +		return ret;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
>  static int rescale_read_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;
>  	int ret;
>  
>  	switch (mask) {
> @@ -46,28 +75,7 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
>  		return iio_read_channel_raw(rescale->source, val);
>  
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = iio_read_channel_scale(rescale->source, val, val2);
> -		switch (ret) {
> -		case IIO_VAL_FRACTIONAL:
> -			*val *= rescale->numerator;
> -			*val2 *= rescale->denominator;
> -			return ret;
> -		case IIO_VAL_INT:
> -			*val *= rescale->numerator;
> -			if (rescale->denominator == 1)
> -				return ret;
> -			*val2 = rescale->denominator;
> -			return IIO_VAL_FRACTIONAL;
> -		case IIO_VAL_FRACTIONAL_LOG2:
> -			tmp = *val * 1000000000LL;
> -			do_div(tmp, rescale->denominator);
> -			tmp *= rescale->numerator;
> -			do_div(tmp, 1000000000LL);
> -			*val = tmp;
> -			return ret;
> -		default:
> -			return -EOPNOTSUPP;
> -		}
> +		return rescale_convert_scale(rescale, val, val2);
>  	default:
>  		return -EINVAL;
>  	}
> 

[PATCH v1 2/3] iio: afe: rescale: Add support of CHAN_INFO_PROCESSED

[Mylène Josserand]

Add the support of the CHAN_INFO_PROCESSED to have directly
the processed value (raw * scale). It will be exported as
in_voltage0_input in sysfs.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 drivers/iio/afe/iio-rescale.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 3e689d6eb501..2275571fff64 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -63,14 +63,54 @@ static int rescale_convert_scale(struct rescale *rescale, int *val, int *val2)
 	}
 }
 
+static int rescale_convert_processed(struct rescale *rescale, int raw,
+				     int *val, int *val2)
+{
+	unsigned long long tmp, scaled;
+	int ret;
+
+	ret = rescale_convert_scale(rescale, val, val2);
+	switch (ret) {
+	case IIO_VAL_FRACTIONAL:
+		tmp = div_s64((s64)*val * 1000000000LL, *val2);
+		scaled = tmp * raw;
+		*val = (int)div_s64_rem(scaled, 1000000000, val2);
+		return ret;
+	case IIO_VAL_INT:
+		return IIO_VAL_FRACTIONAL;
+	case IIO_VAL_FRACTIONAL_LOG2:
+		tmp = shift_right((s64)*val * 1000000000LL, *val2);
+		scaled = tmp * raw;
+		*val = (int)div_s64_rem(scaled, 1000000000LL, val2);
+		return ret;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int rescale_read_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 int raw;
 	int ret;
 
 	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		/* Read the raw value and the scale */
+		ret = iio_read_channel_raw(rescale->source, &raw);
+		if (!ret)
+			return ret;
+		ret = iio_read_channel_scale(rescale->source, val, val2);
+		if (!ret)
+			return ret;
+		/* Process the correct value with raw * scale */
+		ret = rescale_convert_processed(rescale, raw, val, val2);
+		if (!ret)
+			return ret;
+		return IIO_VAL_INT;
+
 	case IIO_CHAN_INFO_RAW:
 		return iio_read_channel_raw(rescale->source, val);
 
@@ -145,7 +185,7 @@ static int rescale_configure_channel(struct device *dev,
 	}
 
 	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-		BIT(IIO_CHAN_INFO_SCALE);
+		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_PROCESSED);
 
 	if (iio_channel_has_available(schan, IIO_CHAN_INFO_RAW))
 		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
-- 
2.11.0

[PATCH v1 3/3] dt-bindings: iio: afe: Add hwmon example

[Mylène Josserand]

With the support of CHAN_INFO_PROCESSED in voltage-divider,
it is possible to read the processed values directly from iio's
sysfs entries or by using iio-hwmon. Add an example for this last
use case.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 .../bindings/iio/afe/voltage-divider.txt           | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt b/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
index b452a8406107..f7e1c7cb2744 100644
--- a/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
+++ b/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
@@ -51,3 +51,27 @@ sysv {
 		spi-max-frequency = <1000000>;
 	};
 };
+
+It is also possible to retrieve the processed values using hwmon node:
+
+div0: div0 {
+	compatible = "voltage-divider";
+	io-channels = <&adc0 0>; /* Channel 0 of the ADC */
+	output-ohms = <47>; /* R2 */
+	full-ohms = <73>; /* R1 (26) + R2 (47) */
+	#io-channel-cells = <1>;
+};
+
+div1: div1 {
+	compatible = "voltage-divider";
+	io-channels = <&adc0 1>; /* Channel 1 of the ADC */
+	output-ohms = <47>; /* R2 */
+	full-ohms = <115>; /* R1 (68) + R2 (47) */
+	#io-channel-cells = <1>;
+};
+
+iio-hwmon {
+	compatible = "iio-hwmon";
+	io-channels = <&div0 0>, <&div1 0>;
+	io-channel-names = "3v3", "usb";
+};
-- 
2.11.0

Re: [PATCH v1 3/3] dt-bindings: iio: afe: Add hwmon example

[Peter Rosin]

On 2019-06-11 11:56, Mylène Josserand wrote:
> With the support of CHAN_INFO_PROCESSED in voltage-divider,
> it is possible to read the processed values directly from iio's
> sysfs entries or by using iio-hwmon. Add an example for this last
> use case.

As I wrote in response to the cover letter, I think iio-hwmon
could "consume" voltage dividers just fine before adding the
processed channel, and while more examples might be good,
there is really no specific relation between iio-hwmon and
voltage dividers. Adding iio-hwmon examples to each and every
iio binding seems pointless. So, I see little reason to add
such examples here.

But if everyone else wants it, don't let me stand in the way...

Cheers,
Peter

> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> ---
>  .../bindings/iio/afe/voltage-divider.txt           | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt b/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> index b452a8406107..f7e1c7cb2744 100644
> --- a/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> +++ b/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> @@ -51,3 +51,27 @@ sysv {
>  		spi-max-frequency = <1000000>;
>  	};
>  };
> +
> +It is also possible to retrieve the processed values using hwmon node:
> +
> +div0: div0 {
> +	compatible = "voltage-divider";
> +	io-channels = <&adc0 0>; /* Channel 0 of the ADC */
> +	output-ohms = <47>; /* R2 */
> +	full-ohms = <73>; /* R1 (26) + R2 (47) */
> +	#io-channel-cells = <1>;
> +};
> +
> +div1: div1 {
> +	compatible = "voltage-divider";
> +	io-channels = <&adc0 1>; /* Channel 1 of the ADC */
> +	output-ohms = <47>; /* R2 */
> +	full-ohms = <115>; /* R1 (68) + R2 (47) */
> +	#io-channel-cells = <1>;
> +};
> +
> +iio-hwmon {
> +	compatible = "iio-hwmon";
> +	io-channels = <&div0 0>, <&div1 0>;
> +	io-channel-names = "3v3", "usb";
> +};
> 

Re: [PATCH v1 0/3] iio: afe: rescale: Add INFO_PROCESSED support

[Peter Rosin]

On 2019-06-11 11:56, Mylène Josserand wrote:
> Hello everyone,
> 
> You will find a small series that add the support of processed values
> for iio-rescale driver.
> Thanks to that, it is possible to read processed values in sysfs instead
> of getting only raw and scale values.
> 
> Here is an example for a 3v3 voltage reading:
> # cat /sys/bus/iio/devices/iio\:device1/in_voltage0_scale
> 3.791015625
> # cat /sys/bus/iio/devices/iio\:device1/in_voltage0_raw
> 879
> # cat /sys/bus/iio/devices/iio\:device1/in_voltage0_input
> 3332
> 
> It is also possible to read directly the processed values using iio-hwmon
> driver (see example in patch03):
> 
> # cat /sys/class/hwmon/hwmon0/in1_input
> 3328
> 
> I seperated my series in 3 patches:
>    - Patch01: Move the scale conversion into a function to prepare the
>    support of IIO_CHAN_INFO_PROCESSED.
>    - Patch02: Add the support of IIO_CHAN_INFO_PROCESSED.
>    - Patch03: Add an example of the use of hwmon and voltage-divider nodes
>    in device-tree.
> 
> If you have any feedbacks on it, I will be pleased to read them!


The last patch about hwmon has nothing to do with this series, and
should be possible as-is without any code changes. No? AFAICT,
iio_hwmon calls iio_read_channel_processed, which calls
iio_convert_raw_to_processed_unlocked in case IIO_CHAN_INFO_PROCESSES
is not handled by the driver. Is that not working?

There is also libiio in userspace that provides the scale as a double
and makes the conversion to a processed value trivial, so the series
is really mostly about the convenience of having a human directly
seeing the processed value in sysfs. Right?

If that is deemed valuable, then I think it should be fixed in a
central location, and not individually for each and every driver.

Anyway, not my call, just stating my opinion, but those are the
reasons for me not adding a processed channel from the very beginning.

Cheers,
Peter

> Best regards,
> Mylène
> 
> Mylène Josserand (3):
>   iio: afe: rescale: Move scale conversion to new function
>   iio: afe: rescale: Add support of CHAN_INFO_PROCESSED
>   dt-bindings: iio: afe: Add hwmon example
> 
>  .../bindings/iio/afe/voltage-divider.txt           | 24 ++++++
>  drivers/iio/afe/iio-rescale.c                      | 96 ++++++++++++++++------
>  2 files changed, 96 insertions(+), 24 deletions(-)
> 

Re: [PATCH v1 0/3] iio: afe: rescale: Add INFO_PROCESSED support

[Jonathan Cameron]

On Tue, 11 Jun 2019 11:02:14 +0000
Peter Rosin <peda@axentia.se> wrote:

> On 2019-06-11 11:56, Mylène Josserand wrote:
> > Hello everyone,
> > 
> > You will find a small series that add the support of processed values
> > for iio-rescale driver.
> > Thanks to that, it is possible to read processed values in sysfs instead
> > of getting only raw and scale values.
> > 
> > Here is an example for a 3v3 voltage reading:
> > # cat /sys/bus/iio/devices/iio\:device1/in_voltage0_scale
> > 3.791015625
> > # cat /sys/bus/iio/devices/iio\:device1/in_voltage0_raw
> > 879
> > # cat /sys/bus/iio/devices/iio\:device1/in_voltage0_input
> > 3332
> > 
> > It is also possible to read directly the processed values using iio-hwmon
> > driver (see example in patch03):
> > 
> > # cat /sys/class/hwmon/hwmon0/in1_input
> > 3328
> > 
> > I seperated my series in 3 patches:
> >    - Patch01: Move the scale conversion into a function to prepare the
> >    support of IIO_CHAN_INFO_PROCESSED.
> >    - Patch02: Add the support of IIO_CHAN_INFO_PROCESSED.
> >    - Patch03: Add an example of the use of hwmon and voltage-divider nodes
> >    in device-tree.
> > 
> > If you have any feedbacks on it, I will be pleased to read them!  
> 
> 
> The last patch about hwmon has nothing to do with this series, and
> should be possible as-is without any code changes. No? AFAICT,
> iio_hwmon calls iio_read_channel_processed, which calls
> iio_convert_raw_to_processed_unlocked in case IIO_CHAN_INFO_PROCESSES
> is not handled by the driver. Is that not working?
> 
> There is also libiio in userspace that provides the scale as a double
> and makes the conversion to a processed value trivial, so the series
> is really mostly about the convenience of having a human directly
> seeing the processed value in sysfs. Right?
> 
> If that is deemed valuable, then I think it should be fixed in a
> central location, and not individually for each and every driver.
> 
> Anyway, not my call, just stating my opinion, but those are the
> reasons for me not adding a processed channel from the very beginning.

I definitely want to fully understand the reasoning behind this proposal.

My gut feeling is that it doesn't make sense a sit ends up with two
interfaces to the same thing in userspace, which we generally want to
avoid.

It's really trivial to do the maths in userspace and often doing it in
kernel is less accurate, or much more complex.

Jonathan
> 
> Cheers,
> Peter
> 
> > Best regards,
> > Mylène
> > 
> > Mylène Josserand (3):
> >   iio: afe: rescale: Move scale conversion to new function
> >   iio: afe: rescale: Add support of CHAN_INFO_PROCESSED
> >   dt-bindings: iio: afe: Add hwmon example
> > 
> >  .../bindings/iio/afe/voltage-divider.txt           | 24 ++++++
> >  drivers/iio/afe/iio-rescale.c                      | 96 ++++++++++++++++------
> >  2 files changed, 96 insertions(+), 24 deletions(-)
> >   
>