Linux-Devicetree Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/4] iio: adc: stm32-dfsdm: add scale and offset support
@ 2020-02-04 10:10 Olivier Moysan
  2020-02-04 10:10 ` [PATCH 1/4] dt-bindings: iio: adc: sd modulator: add vref support Olivier Moysan
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Olivier Moysan @ 2020-02-04 10:10 UTC (permalink / raw)
  To: jic23, robh+dt, olivier.moysan
  Cc: mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

Add scale and offset attributes support to STM32 DFSDM.
Also add scale support to the SD modulator, which can be used as input for the DFSDM.

Olivier Moysan (4):
  dt-bindings: iio: adc: sd modulator: add vref support
  iio: adc: sd modulator: add scale support
  iio: adc: stm32-dfsdm: use resolution define
  iio: adc: stm32-dfsdm: add scale and offset support

 .../iio/adc/sigma-delta-modulator.yaml        |   4 +
 drivers/iio/adc/sd_adc_modulator.c            | 108 ++++++++++++++++--
 drivers/iio/adc/stm32-dfsdm-adc.c             | 107 ++++++++++++++++-
 3 files changed, 207 insertions(+), 12 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/4] dt-bindings: iio: adc: sd modulator: add vref support
  2020-02-04 10:10 [PATCH 0/4] iio: adc: stm32-dfsdm: add scale and offset support Olivier Moysan
@ 2020-02-04 10:10 ` Olivier Moysan
  2020-02-06 19:08   ` Rob Herring
  2020-02-08 16:04   ` Jonathan Cameron
  2020-02-04 10:10 ` [PATCH 2/4] iio: adc: sd modulator: add scale support Olivier Moysan
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 15+ messages in thread
From: Olivier Moysan @ 2020-02-04 10:10 UTC (permalink / raw)
  To: jic23, robh+dt, olivier.moysan
  Cc: mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

Add vref supply support to sigma delta modulator.

Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 .../devicetree/bindings/iio/adc/sigma-delta-modulator.yaml    | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml b/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml
index a390343d0c2a..2afe0765e971 100644
--- a/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml
@@ -8,6 +8,7 @@ title: Device-Tree bindings for sigma delta modulator
 
 maintainers:
   - Arnaud Pouliquen <arnaud.pouliquen@st.com>
+  - Olivier Moysan <olivier.moysan@st.com>
 
 properties:
   compatible:
@@ -21,6 +22,9 @@ properties:
   '#io-channel-cells':
     const: 0
 
+  vref-supply:
+    description: Phandle to the vref input analog reference voltage.
+
 required:
   - compatible
   - '#io-channel-cells'
-- 
2.17.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/4] iio: adc: sd modulator: add scale support
  2020-02-04 10:10 [PATCH 0/4] iio: adc: stm32-dfsdm: add scale and offset support Olivier Moysan
  2020-02-04 10:10 ` [PATCH 1/4] dt-bindings: iio: adc: sd modulator: add vref support Olivier Moysan
@ 2020-02-04 10:10 ` Olivier Moysan
  2020-02-08 16:03   ` Jonathan Cameron
  2020-02-04 10:10 ` [PATCH 3/4] iio: adc: stm32-dfsdm: use resolution define Olivier Moysan
  2020-02-04 10:10 ` [PATCH 4/4] iio: adc: stm32-dfsdm: add scale and offset support Olivier Moysan
  3 siblings, 1 reply; 15+ messages in thread
From: Olivier Moysan @ 2020-02-04 10:10 UTC (permalink / raw)
  To: jic23, robh+dt, olivier.moysan
  Cc: mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

Add scale support to sigma delta modulator.

Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 drivers/iio/adc/sd_adc_modulator.c | 108 ++++++++++++++++++++++++++---
 1 file changed, 100 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/sd_adc_modulator.c b/drivers/iio/adc/sd_adc_modulator.c
index 560d8c7d9d86..a83f35832050 100644
--- a/drivers/iio/adc/sd_adc_modulator.c
+++ b/drivers/iio/adc/sd_adc_modulator.c
@@ -10,8 +10,7 @@
 #include <linux/iio/triggered_buffer.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
-
-static const struct iio_info iio_sd_mod_iio_info;
+#include <linux/regulator/consumer.h>
 
 static const struct iio_chan_spec iio_sd_mod_ch = {
 	.type = IIO_VOLTAGE,
@@ -21,34 +20,126 @@ static const struct iio_chan_spec iio_sd_mod_ch = {
 		.realbits = 1,
 		.shift = 0,
 	},
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+};
+
+static const struct iio_chan_spec iio_sd_mod_ch_ads = {
+	.type = IIO_VOLTAGE,
+	.indexed = 1,
+	.scan_type = {
+		.sign = 'u',
+		.realbits = 1,
+		.shift = 0,
+	},
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+	.differential = 1,
+};
+
+struct iio_sd_mod_priv {
+	struct regulator *vref;
+	int vref_mv;
+};
+
+static int iio_sd_mod_read_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan, int *val,
+			       int *val2, long mask)
+{
+	struct iio_sd_mod_priv *priv = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*val = priv->vref_mv;
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info iio_sd_mod_iio_info = {
+	.read_raw = iio_sd_mod_read_raw,
 };
 
 static int iio_sd_mod_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct iio_sd_mod_priv *priv;
 	struct iio_dev *iio;
+	int ret;
 
-	iio = devm_iio_device_alloc(dev, 0);
+	iio = devm_iio_device_alloc(dev, sizeof(*priv));
 	if (!iio)
 		return -ENOMEM;
 
+	iio->channels = (const struct iio_chan_spec *)
+					of_device_get_match_data(&pdev->dev);
+
+	priv = iio_priv(iio);
+
 	iio->dev.parent = dev;
 	iio->dev.of_node = dev->of_node;
 	iio->name = dev_name(dev);
 	iio->info = &iio_sd_mod_iio_info;
 	iio->modes = INDIO_BUFFER_HARDWARE;
-
 	iio->num_channels = 1;
-	iio->channels = &iio_sd_mod_ch;
 
 	platform_set_drvdata(pdev, iio);
 
-	return devm_iio_device_register(&pdev->dev, iio);
+	priv->vref = devm_regulator_get_optional(dev, "vref");
+	if (IS_ERR(priv->vref)) {
+		ret = PTR_ERR(priv->vref);
+		if (ret != -ENODEV) {
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "vref get failed, %d\n", ret);
+			return ret;
+		}
+	}
+
+	if (!IS_ERR(priv->vref)) {
+		ret = regulator_enable(priv->vref);
+		if (ret < 0) {
+			dev_err(dev, "vref enable failed %d\n", ret);
+			return ret;
+		}
+
+		ret = regulator_get_voltage(priv->vref);
+		if (ret < 0) {
+			dev_err(dev, "vref get failed, %d\n", ret);
+			goto err_regulator_disable;
+		}
+
+		priv->vref_mv = ret / 1000;
+		dev_dbg(dev, "vref+=%dmV\n", priv->vref_mv);
+	}
+
+	ret = devm_iio_device_register(&pdev->dev, iio);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register sd-modulator, %d\n", ret);
+		goto err_regulator_disable;
+	}
+
+	return 0;
+
+err_regulator_disable:
+	regulator_disable(priv->vref);
+
+	return ret;
+}
+
+static int iio_sd_mod_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct iio_sd_mod_priv *priv = iio_priv(indio_dev);
+
+	if (priv->vref)
+		return regulator_disable(priv->vref);
+
+	return 0;
 }
 
 static const struct of_device_id sd_adc_of_match[] = {
-	{ .compatible = "sd-modulator" },
-	{ .compatible = "ads1201" },
+	{ .compatible = "sd-modulator", .data = &iio_sd_mod_ch },
+	{ .compatible = "ads1201", .data = &iio_sd_mod_ch_ads },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sd_adc_of_match);
@@ -59,6 +150,7 @@ static struct platform_driver iio_sd_mod_adc = {
 		.of_match_table = of_match_ptr(sd_adc_of_match),
 	},
 	.probe = iio_sd_mod_probe,
+	.remove = iio_sd_mod_remove,
 };
 
 module_platform_driver(iio_sd_mod_adc);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 3/4] iio: adc: stm32-dfsdm: use resolution define
  2020-02-04 10:10 [PATCH 0/4] iio: adc: stm32-dfsdm: add scale and offset support Olivier Moysan
  2020-02-04 10:10 ` [PATCH 1/4] dt-bindings: iio: adc: sd modulator: add vref support Olivier Moysan
  2020-02-04 10:10 ` [PATCH 2/4] iio: adc: sd modulator: add scale support Olivier Moysan
@ 2020-02-04 10:10 ` Olivier Moysan
  2020-02-04 10:10 ` [PATCH 4/4] iio: adc: stm32-dfsdm: add scale and offset support Olivier Moysan
  3 siblings, 0 replies; 15+ messages in thread
From: Olivier Moysan @ 2020-02-04 10:10 UTC (permalink / raw)
  To: jic23, robh+dt, olivier.moysan
  Cc: mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

Use resolution define instead of hard coded value.

Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 drivers/iio/adc/stm32-dfsdm-adc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 2aad2cda6943..07b9dfdf8e76 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -1440,7 +1440,7 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
 		ch->scan_type.shift = 8;
 	}
 	ch->scan_type.sign = 's';
-	ch->scan_type.realbits = 24;
+	ch->scan_type.realbits = DFSDM_DATA_RES;
 	ch->scan_type.storagebits = 32;
 
 	return stm32_dfsdm_chan_configure(adc->dfsdm,
-- 
2.17.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 4/4] iio: adc: stm32-dfsdm: add scale and offset support
  2020-02-04 10:10 [PATCH 0/4] iio: adc: stm32-dfsdm: add scale and offset support Olivier Moysan
                   ` (2 preceding siblings ...)
  2020-02-04 10:10 ` [PATCH 3/4] iio: adc: stm32-dfsdm: use resolution define Olivier Moysan
@ 2020-02-04 10:10 ` Olivier Moysan
  2020-02-08 16:18   ` Jonathan Cameron
  3 siblings, 1 reply; 15+ messages in thread
From: Olivier Moysan @ 2020-02-04 10:10 UTC (permalink / raw)
  To: jic23, robh+dt, olivier.moysan
  Cc: mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

Add scale and offset attributes support to STM32 DFSDM.

Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 drivers/iio/adc/stm32-dfsdm-adc.c | 105 +++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 07b9dfdf8e76..b85fd3e90496 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -10,6 +10,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/iio/adc/stm32-dfsdm-adc.h>
 #include <linux/iio/buffer.h>
+#include <linux/iio/consumer.h>
 #include <linux/iio/hw-consumer.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/timer/stm32-lptim-trigger.h>
@@ -67,6 +68,13 @@ struct stm32_dfsdm_dev_data {
 	const struct regmap_config *regmap_cfg;
 };
 
+struct stm32_dfsdm_sd_chan_info {
+	int scale_val;
+	int scale_val2;
+	int offset;
+	unsigned int differential;
+};
+
 struct stm32_dfsdm_adc {
 	struct stm32_dfsdm *dfsdm;
 	const struct stm32_dfsdm_dev_data *dev_data;
@@ -79,6 +87,7 @@ struct stm32_dfsdm_adc {
 	struct iio_hw_consumer *hwc;
 	struct completion completion;
 	u32 *buffer;
+	struct stm32_dfsdm_sd_chan_info *sd_chan;
 
 	/* Audio specific */
 	unsigned int spi_freq;  /* SPI bus clock frequency */
@@ -1271,7 +1280,10 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
 				int *val2, long mask)
 {
 	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
-	int ret;
+	struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
+	struct stm32_dfsdm_filter_osr *flo = &fl->flo[fl->fast];
+	u32 max = flo->max << (flo->lshift - chan->scan_type.shift);
+	int ret, idx = chan->scan_index;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -1307,6 +1319,41 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
 		*val = adc->sample_freq;
 
 		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		/*
+		 * Scale is expressed in mV.
+		 * When fast mode is disabled, actual resolution may be lower
+		 * than 2^n, where n=realbits-1.
+		 * This leads to underestimating input voltage. To
+		 * compensate this deviation, the voltage reference can be
+		 * corrected with a factor = realbits resolution / actual max
+		 */
+		*val = div_u64((u64)adc->sd_chan[idx].scale_val *
+			       (u64)BIT(DFSDM_DATA_RES - 1), max);
+		*val2 = chan->scan_type.realbits;
+		if (adc->sd_chan[idx].differential)
+			*val *= 2;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	case IIO_CHAN_INFO_OFFSET:
+		/*
+		 * DFSDM output data are in the range [-2^n,2^n-1],
+		 * with n=realbits-1.
+		 * - Differential modulator:
+		 * Offset correspond to SD modulator offset.
+		 * - Single ended modulator:
+		 * Input is in [0V,Vref] range, where 0V corresponds to -2^n.
+		 * Add 2^n to offset. (i.e. middle of input range)
+		 * offset = offset(sd) * vref / res(sd) * max / vref.
+		 */
+		*val = div_u64((u64)max * adc->sd_chan[idx].offset,
+			       BIT(adc->sd_chan[idx].scale_val2 - 1));
+		if (!adc->sd_chan[idx].differential)
+			*val += max;
+
+		return IIO_VAL_INT;
 	}
 
 	return -EINVAL;
@@ -1430,7 +1477,9 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
 	 * IIO_CHAN_INFO_RAW: used to compute regular conversion
 	 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
 	 */
-	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				 BIT(IIO_CHAN_INFO_SCALE) |
+				 BIT(IIO_CHAN_INFO_OFFSET);
 	ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
 					BIT(IIO_CHAN_INFO_SAMP_FREQ);
 
@@ -1481,8 +1530,10 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
 {
 	struct iio_chan_spec *ch;
 	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
+	struct iio_channel *channels, *chan;
+	struct stm32_dfsdm_sd_chan_info *sd_chan;
 	int num_ch;
-	int ret, chan_idx;
+	int ret, chan_idx, val2;
 
 	adc->oversamp = DFSDM_DEFAULT_OVERSAMPLING;
 	ret = stm32_dfsdm_compute_all_osrs(indio_dev, adc->oversamp);
@@ -1506,6 +1557,22 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
 	if (!ch)
 		return -ENOMEM;
 
+	/* Get SD modulator channels */
+	channels = iio_channel_get_all(&indio_dev->dev);
+	if (IS_ERR(channels)) {
+		dev_err(&indio_dev->dev, "Failed to get channel %ld\n",
+			PTR_ERR(channels));
+		return PTR_ERR(channels);
+	}
+	chan = &channels[0];
+
+	adc->sd_chan = devm_kzalloc(&indio_dev->dev,
+				    sizeof(*adc->sd_chan) * num_ch, GFP_KERNEL);
+	if (!adc->sd_chan)
+		return -ENOMEM;
+
+	sd_chan = adc->sd_chan;
+
 	for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
 		ch[chan_idx].scan_index = chan_idx;
 		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &ch[chan_idx]);
@@ -1513,6 +1580,38 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
 			dev_err(&indio_dev->dev, "Channels init failed\n");
 			return ret;
 		}
+
+		if (!chan->indio_dev)
+			return -EINVAL;
+
+		ret = iio_read_channel_scale(chan, &sd_chan->scale_val,
+					     &sd_chan->scale_val2);
+		if (ret < 0) {
+			dev_err(&indio_dev->dev,
+				"Failed to get channel %d scale\n", chan_idx);
+			return ret;
+		}
+
+		if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_OFFSET)) {
+			ret = iio_read_channel_offset(chan, &sd_chan->offset,
+						      &val2);
+			if (ret < 0) {
+				dev_err(&indio_dev->dev,
+					"Failed to get channel %d offset\n",
+					chan_idx);
+				return ret;
+			}
+		}
+
+		sd_chan->differential = chan->channel->differential;
+
+		dev_dbg(&indio_dev->dev, "Channel %d %s scale ref=%d offset=%d",
+			chan_idx, chan->channel->differential ?
+			"differential" : "single-ended",
+			sd_chan->scale_val, sd_chan->offset);
+
+		chan++;
+		sd_chan++;
 	}
 
 	indio_dev->num_channels = num_ch;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] dt-bindings: iio: adc: sd modulator: add vref support
  2020-02-04 10:10 ` [PATCH 1/4] dt-bindings: iio: adc: sd modulator: add vref support Olivier Moysan
@ 2020-02-06 19:08   ` Rob Herring
  2020-02-08 16:04   ` Jonathan Cameron
  1 sibling, 0 replies; 15+ messages in thread
From: Rob Herring @ 2020-02-06 19:08 UTC (permalink / raw)
  To: Olivier Moysan
  Cc: jic23, robh+dt, olivier.moysan, mark.rutland, knaack.h, lars,
	devicetree, linux-iio, linux-kernel, pmeerw, linux-stm32,
	linux-arm-kernel

On Tue, 4 Feb 2020 11:10:05 +0100, Olivier Moysan wrote:
> Add vref supply support to sigma delta modulator.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
> ---
>  .../devicetree/bindings/iio/adc/sigma-delta-modulator.yaml    | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] iio: adc: sd modulator: add scale support
  2020-02-04 10:10 ` [PATCH 2/4] iio: adc: sd modulator: add scale support Olivier Moysan
@ 2020-02-08 16:03   ` Jonathan Cameron
  2020-02-11 15:22     ` Olivier MOYSAN
  0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2020-02-08 16:03 UTC (permalink / raw)
  To: Olivier Moysan
  Cc: robh+dt, mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

On Tue, 4 Feb 2020 11:10:06 +0100
Olivier Moysan <olivier.moysan@st.com> wrote:

> Add scale support to sigma delta modulator.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@st.com>

I note below that there are probably some complexities around
whether vref is used as you have it here or not.

A few other bits inline around a race condition introduced in probe / remove.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/sd_adc_modulator.c | 108 ++++++++++++++++++++++++++---
>  1 file changed, 100 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/sd_adc_modulator.c b/drivers/iio/adc/sd_adc_modulator.c
> index 560d8c7d9d86..a83f35832050 100644
> --- a/drivers/iio/adc/sd_adc_modulator.c
> +++ b/drivers/iio/adc/sd_adc_modulator.c
> @@ -10,8 +10,7 @@
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> -
> -static const struct iio_info iio_sd_mod_iio_info;
> +#include <linux/regulator/consumer.h>
>  
>  static const struct iio_chan_spec iio_sd_mod_ch = {
>  	.type = IIO_VOLTAGE,
> @@ -21,34 +20,126 @@ static const struct iio_chan_spec iio_sd_mod_ch = {
>  		.realbits = 1,
>  		.shift = 0,
>  	},
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),

This relies on generic sigma delta modulators using an external vref.
They might have an internal always on regulator...
I would suggest we only support scale for devices with explicitly
defined compatibles where we can know what regulators are used etc.

For many devices, there will be a single powersupply called vdd-supply
or similar in DT.  It may also provide a reference voltage.

> +};
> +
> +static const struct iio_chan_spec iio_sd_mod_ch_ads = {
> +	.type = IIO_VOLTAGE,
> +	.indexed = 1,
> +	.scan_type = {
> +		.sign = 'u',
> +		.realbits = 1,
> +		.shift = 0,
> +	},
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +	.differential = 1,
> +};
> +
> +struct iio_sd_mod_priv {
> +	struct regulator *vref;
> +	int vref_mv;
> +};
> +
> +static int iio_sd_mod_read_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan, int *val,
> +			       int *val2, long mask)
> +{
> +	struct iio_sd_mod_priv *priv = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = priv->vref_mv;
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info iio_sd_mod_iio_info = {
> +	.read_raw = iio_sd_mod_read_raw,
>  };
>  
>  static int iio_sd_mod_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct iio_sd_mod_priv *priv;
>  	struct iio_dev *iio;
> +	int ret;
>  
> -	iio = devm_iio_device_alloc(dev, 0);
> +	iio = devm_iio_device_alloc(dev, sizeof(*priv));
>  	if (!iio)
>  		return -ENOMEM;
>  
> +	iio->channels = (const struct iio_chan_spec *)
> +					of_device_get_match_data(&pdev->dev);
> +
> +	priv = iio_priv(iio);
> +
>  	iio->dev.parent = dev;
>  	iio->dev.of_node = dev->of_node;
>  	iio->name = dev_name(dev);
>  	iio->info = &iio_sd_mod_iio_info;
>  	iio->modes = INDIO_BUFFER_HARDWARE;
> -
>  	iio->num_channels = 1;
> -	iio->channels = &iio_sd_mod_ch;
>  
>  	platform_set_drvdata(pdev, iio);
>  
> -	return devm_iio_device_register(&pdev->dev, iio);
> +	priv->vref = devm_regulator_get_optional(dev, "vref");
> +	if (IS_ERR(priv->vref)) {
> +		ret = PTR_ERR(priv->vref);
> +		if (ret != -ENODEV) {
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "vref get failed, %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	if (!IS_ERR(priv->vref)) {
> +		ret = regulator_enable(priv->vref);
> +		if (ret < 0) {
> +			dev_err(dev, "vref enable failed %d\n", ret);
> +			return ret;
> +		}
> +
> +		ret = regulator_get_voltage(priv->vref);
> +		if (ret < 0) {
> +			dev_err(dev, "vref get failed, %d\n", ret);
> +			goto err_regulator_disable;
> +		}
> +
> +		priv->vref_mv = ret / 1000;
> +		dev_dbg(dev, "vref+=%dmV\n", priv->vref_mv);
> +	}
> +
> +	ret = devm_iio_device_register(&pdev->dev, iio);
This exposes the userspace and in kernel interfaces.  Those
are partly dependent on the regulator enable you have above.

Using devm_ version fo this interface leaves you with a race in remove.
The regulator is disabled before you have remove the interfaces that
will only work if we assume it is still on.

Hence, you should either use devm_add_action_or_reset magic
to ensure we still do everything in the right order, or do it
manually by using iio_device_register and iio_device_unregister.


> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register sd-modulator, %d\n", ret);
> +		goto err_regulator_disable;
> +	}
> +
> +	return 0;
> +
> +err_regulator_disable:
> +	regulator_disable(priv->vref);
> +
> +	return ret;
> +}
> +
> +static int iio_sd_mod_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct iio_sd_mod_priv *priv = iio_priv(indio_dev);
> +
> +	if (priv->vref)
> +		return regulator_disable(priv->vref);
> +
> +	return 0;
>  }
>  
>  static const struct of_device_id sd_adc_of_match[] = {
> -	{ .compatible = "sd-modulator" },
> -	{ .compatible = "ads1201" },
> +	{ .compatible = "sd-modulator", .data = &iio_sd_mod_ch },
> +	{ .compatible = "ads1201", .data = &iio_sd_mod_ch_ads },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, sd_adc_of_match);
> @@ -59,6 +150,7 @@ static struct platform_driver iio_sd_mod_adc = {
>  		.of_match_table = of_match_ptr(sd_adc_of_match),
>  	},
>  	.probe = iio_sd_mod_probe,
> +	.remove = iio_sd_mod_remove,
>  };
>  
>  module_platform_driver(iio_sd_mod_adc);


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] dt-bindings: iio: adc: sd modulator: add vref support
  2020-02-04 10:10 ` [PATCH 1/4] dt-bindings: iio: adc: sd modulator: add vref support Olivier Moysan
  2020-02-06 19:08   ` Rob Herring
@ 2020-02-08 16:04   ` Jonathan Cameron
  2020-02-11 15:24     ` Olivier MOYSAN
  1 sibling, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2020-02-08 16:04 UTC (permalink / raw)
  To: Olivier Moysan
  Cc: robh+dt, mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

On Tue, 4 Feb 2020 11:10:05 +0100
Olivier Moysan <olivier.moysan@st.com> wrote:

> Add vref supply support to sigma delta modulator.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
> ---
>  .../devicetree/bindings/iio/adc/sigma-delta-modulator.yaml    | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml b/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml
> index a390343d0c2a..2afe0765e971 100644
> --- a/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml
> @@ -8,6 +8,7 @@ title: Device-Tree bindings for sigma delta modulator
>  
>  maintainers:
>    - Arnaud Pouliquen <arnaud.pouliquen@st.com>
> +  - Olivier Moysan <olivier.moysan@st.com>
>  
>  properties:
>    compatible:
> @@ -21,6 +22,9 @@ properties:
>    '#io-channel-cells':
>      const: 0
>  
> +  vref-supply:
> +    description: Phandle to the vref input analog reference voltage.
I note this in review of patch 2 but in general I'm not sure we should
be introducing this for generic devices.   It's fine if we have an
explicit compatible but there is no reason to assume a generic sd-modulator
uses an external reference.

Jonathan

> +
>  required:
>    - compatible
>    - '#io-channel-cells'


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] iio: adc: stm32-dfsdm: add scale and offset support
  2020-02-04 10:10 ` [PATCH 4/4] iio: adc: stm32-dfsdm: add scale and offset support Olivier Moysan
@ 2020-02-08 16:18   ` Jonathan Cameron
  2020-02-11 15:19     ` Olivier MOYSAN
  0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2020-02-08 16:18 UTC (permalink / raw)
  To: Olivier Moysan
  Cc: robh+dt, mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

On Tue, 4 Feb 2020 11:10:08 +0100
Olivier Moysan <olivier.moysan@st.com> wrote:

> Add scale and offset attributes support to STM32 DFSDM.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@st.com>

Hmm. I can't remember this history of this but we've kind of
ended up backwards wrt to other consumer drivers.

In some sense this is similar to the analog gyroscopes.  In those
the consumer driver is the gyroscope which is consuming the raw
readings from an ADC connected to the channel.  This results
in us getting readings reported by the gyroscope driver.

Here we have a sigma delta convertor consuming the pulse train
from a sigma delta device.  So the channels are reported by
the sigma delta receiver, whereas i think the nearest equivalent
to the analog voltage outputing gyroscopes would have been if
we had reported the channel values at the sigma delta converter.

This wasn't really an issue when the only values available were
raw, but if we are adding scale and offset, they are things that
belong to the ad1201 for example, not the upstream stm32-dfsdm unit.

Thinking of it another way, we don't report an SPI ADC output in
the driver for the SPI master.

Could we flip it around without breaking anything?

Jonathan

> ---
>  drivers/iio/adc/stm32-dfsdm-adc.c | 105 +++++++++++++++++++++++++++++-
>  1 file changed, 102 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index 07b9dfdf8e76..b85fd3e90496 100644
> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> @@ -10,6 +10,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/iio/adc/stm32-dfsdm-adc.h>
>  #include <linux/iio/buffer.h>
> +#include <linux/iio/consumer.h>
>  #include <linux/iio/hw-consumer.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/timer/stm32-lptim-trigger.h>
> @@ -67,6 +68,13 @@ struct stm32_dfsdm_dev_data {
>  	const struct regmap_config *regmap_cfg;
>  };
>  
> +struct stm32_dfsdm_sd_chan_info {
> +	int scale_val;
> +	int scale_val2;
> +	int offset;
> +	unsigned int differential;
> +};
> +
>  struct stm32_dfsdm_adc {
>  	struct stm32_dfsdm *dfsdm;
>  	const struct stm32_dfsdm_dev_data *dev_data;
> @@ -79,6 +87,7 @@ struct stm32_dfsdm_adc {
>  	struct iio_hw_consumer *hwc;
>  	struct completion completion;
>  	u32 *buffer;
> +	struct stm32_dfsdm_sd_chan_info *sd_chan;
>  
>  	/* Audio specific */
>  	unsigned int spi_freq;  /* SPI bus clock frequency */
> @@ -1271,7 +1280,10 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>  				int *val2, long mask)
>  {
>  	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> -	int ret;
> +	struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
> +	struct stm32_dfsdm_filter_osr *flo = &fl->flo[fl->fast];
> +	u32 max = flo->max << (flo->lshift - chan->scan_type.shift);
> +	int ret, idx = chan->scan_index;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -1307,6 +1319,41 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>  		*val = adc->sample_freq;
>  
>  		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		/*
> +		 * Scale is expressed in mV.
> +		 * When fast mode is disabled, actual resolution may be lower
> +		 * than 2^n, where n=realbits-1.
> +		 * This leads to underestimating input voltage. To
> +		 * compensate this deviation, the voltage reference can be
> +		 * corrected with a factor = realbits resolution / actual max
> +		 */
> +		*val = div_u64((u64)adc->sd_chan[idx].scale_val *
> +			       (u64)BIT(DFSDM_DATA_RES - 1), max);
> +		*val2 = chan->scan_type.realbits;
> +		if (adc->sd_chan[idx].differential)
> +			*val *= 2;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	case IIO_CHAN_INFO_OFFSET:
> +		/*
> +		 * DFSDM output data are in the range [-2^n,2^n-1],
> +		 * with n=realbits-1.
> +		 * - Differential modulator:
> +		 * Offset correspond to SD modulator offset.
> +		 * - Single ended modulator:
> +		 * Input is in [0V,Vref] range, where 0V corresponds to -2^n.
> +		 * Add 2^n to offset. (i.e. middle of input range)
> +		 * offset = offset(sd) * vref / res(sd) * max / vref.
> +		 */
> +		*val = div_u64((u64)max * adc->sd_chan[idx].offset,
> +			       BIT(adc->sd_chan[idx].scale_val2 - 1));
> +		if (!adc->sd_chan[idx].differential)
> +			*val += max;
> +
> +		return IIO_VAL_INT;
>  	}
>  
>  	return -EINVAL;
> @@ -1430,7 +1477,9 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
>  	 * IIO_CHAN_INFO_RAW: used to compute regular conversion
>  	 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
>  	 */
> -	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				 BIT(IIO_CHAN_INFO_SCALE) |
> +				 BIT(IIO_CHAN_INFO_OFFSET);
>  	ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
>  					BIT(IIO_CHAN_INFO_SAMP_FREQ);
>  
> @@ -1481,8 +1530,10 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
>  {
>  	struct iio_chan_spec *ch;
>  	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	struct iio_channel *channels, *chan;
> +	struct stm32_dfsdm_sd_chan_info *sd_chan;
>  	int num_ch;
> -	int ret, chan_idx;
> +	int ret, chan_idx, val2;
>  
>  	adc->oversamp = DFSDM_DEFAULT_OVERSAMPLING;
>  	ret = stm32_dfsdm_compute_all_osrs(indio_dev, adc->oversamp);
> @@ -1506,6 +1557,22 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
>  	if (!ch)
>  		return -ENOMEM;
>  
> +	/* Get SD modulator channels */
> +	channels = iio_channel_get_all(&indio_dev->dev);
> +	if (IS_ERR(channels)) {
> +		dev_err(&indio_dev->dev, "Failed to get channel %ld\n",
> +			PTR_ERR(channels));
> +		return PTR_ERR(channels);
> +	}
> +	chan = &channels[0];
> +
> +	adc->sd_chan = devm_kzalloc(&indio_dev->dev,
> +				    sizeof(*adc->sd_chan) * num_ch, GFP_KERNEL);
> +	if (!adc->sd_chan)
> +		return -ENOMEM;
> +
> +	sd_chan = adc->sd_chan;
> +
>  	for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
>  		ch[chan_idx].scan_index = chan_idx;
>  		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &ch[chan_idx]);
> @@ -1513,6 +1580,38 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
>  			dev_err(&indio_dev->dev, "Channels init failed\n");
>  			return ret;
>  		}
> +
> +		if (!chan->indio_dev)
> +			return -EINVAL;
> +
> +		ret = iio_read_channel_scale(chan, &sd_chan->scale_val,
> +					     &sd_chan->scale_val2);
> +		if (ret < 0) {
> +			dev_err(&indio_dev->dev,
> +				"Failed to get channel %d scale\n", chan_idx);
> +			return ret;
> +		}
> +
> +		if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_OFFSET)) {
> +			ret = iio_read_channel_offset(chan, &sd_chan->offset,
> +						      &val2);
> +			if (ret < 0) {
> +				dev_err(&indio_dev->dev,
> +					"Failed to get channel %d offset\n",
> +					chan_idx);
> +				return ret;
> +			}
> +		}
> +
> +		sd_chan->differential = chan->channel->differential;
> +
> +		dev_dbg(&indio_dev->dev, "Channel %d %s scale ref=%d offset=%d",
> +			chan_idx, chan->channel->differential ?
> +			"differential" : "single-ended",
> +			sd_chan->scale_val, sd_chan->offset);
> +
> +		chan++;
> +		sd_chan++;
>  	}
>  
>  	indio_dev->num_channels = num_ch;


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] iio: adc: stm32-dfsdm: add scale and offset support
  2020-02-08 16:18   ` Jonathan Cameron
@ 2020-02-11 15:19     ` Olivier MOYSAN
  2020-02-14 13:11       ` Jonathan Cameron
  0 siblings, 1 reply; 15+ messages in thread
From: Olivier MOYSAN @ 2020-02-11 15:19 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: robh+dt, mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

Hi Jonathan,

On 2/8/20 5:18 PM, Jonathan Cameron wrote:
> On Tue, 4 Feb 2020 11:10:08 +0100
> Olivier Moysan <olivier.moysan@st.com> wrote:
>
>> Add scale and offset attributes support to STM32 DFSDM.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
> Hmm. I can't remember this history of this but we've kind of
> ended up backwards wrt to other consumer drivers.
>
> In some sense this is similar to the analog gyroscopes.  In those
> the consumer driver is the gyroscope which is consuming the raw
> readings from an ADC connected to the channel.  This results
> in us getting readings reported by the gyroscope driver.
>
> Here we have a sigma delta convertor consuming the pulse train
> from a sigma delta device.  So the channels are reported by
> the sigma delta receiver, whereas i think the nearest equivalent
> to the analog voltage outputing gyroscopes would have been if
> we had reported the channel values at the sigma delta converter.
The DFSDM driver is currently used as a consumer of the sd modulator.
The scale and offset values of the channels are already computed by
the DFSDM driver, and provided by this driver to the IIO ABI.
However, the DFSDM has no voltage reference, so it has to retrieve
it from sd-modulator channels, for the scale factor computation.

                                     scale  offset
                                       ^      ^
                                       |      |       IIO ABI
+-------------------------------------------------------------+
          +---------------+          +-------------+
          |sd driver      |          |DFSDM driver |
          +---------------+          +-------------+
+-------------------------------------------------------------+
                                                          HW
          +---------------+          +-------------+
+------->+ sd-modulator  +--------->+ DFSDM +-------->
analog   +------+--------+          +-------------+ output
input           ^
                 | vref
                 +


Is it the topology your are expecting ?
If not, I probably missedsomething. Could you please clarify this point ?

Regards
Olivier
> This wasn't really an issue when the only values available were
> raw, but if we are adding scale and offset, they are things that
> belong to the ad1201 for example, not the upstream stm32-dfsdm unit.
>
> Thinking of it another way, we don't report an SPI ADC output in
> the driver for the SPI master.
>
> Could we flip it around without breaking anything?
>
> Jonathan
>
>> ---
>>   drivers/iio/adc/stm32-dfsdm-adc.c | 105 +++++++++++++++++++++++++++++-
>>   1 file changed, 102 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
>> index 07b9dfdf8e76..b85fd3e90496 100644
>> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
>> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/dma-mapping.h>
>>   #include <linux/iio/adc/stm32-dfsdm-adc.h>
>>   #include <linux/iio/buffer.h>
>> +#include <linux/iio/consumer.h>
>>   #include <linux/iio/hw-consumer.h>
>>   #include <linux/iio/sysfs.h>
>>   #include <linux/iio/timer/stm32-lptim-trigger.h>
>> @@ -67,6 +68,13 @@ struct stm32_dfsdm_dev_data {
>>   	const struct regmap_config *regmap_cfg;
>>   };
>>   
>> +struct stm32_dfsdm_sd_chan_info {
>> +	int scale_val;
>> +	int scale_val2;
>> +	int offset;
>> +	unsigned int differential;
>> +};
>> +
>>   struct stm32_dfsdm_adc {
>>   	struct stm32_dfsdm *dfsdm;
>>   	const struct stm32_dfsdm_dev_data *dev_data;
>> @@ -79,6 +87,7 @@ struct stm32_dfsdm_adc {
>>   	struct iio_hw_consumer *hwc;
>>   	struct completion completion;
>>   	u32 *buffer;
>> +	struct stm32_dfsdm_sd_chan_info *sd_chan;
>>   
>>   	/* Audio specific */
>>   	unsigned int spi_freq;  /* SPI bus clock frequency */
>> @@ -1271,7 +1280,10 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>>   				int *val2, long mask)
>>   {
>>   	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> -	int ret;
>> +	struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
>> +	struct stm32_dfsdm_filter_osr *flo = &fl->flo[fl->fast];
>> +	u32 max = flo->max << (flo->lshift - chan->scan_type.shift);
>> +	int ret, idx = chan->scan_index;
>>   
>>   	switch (mask) {
>>   	case IIO_CHAN_INFO_RAW:
>> @@ -1307,6 +1319,41 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>>   		*val = adc->sample_freq;
>>   
>>   		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		/*
>> +		 * Scale is expressed in mV.
>> +		 * When fast mode is disabled, actual resolution may be lower
>> +		 * than 2^n, where n=realbits-1.
>> +		 * This leads to underestimating input voltage. To
>> +		 * compensate this deviation, the voltage reference can be
>> +		 * corrected with a factor = realbits resolution / actual max
>> +		 */
>> +		*val = div_u64((u64)adc->sd_chan[idx].scale_val *
>> +			       (u64)BIT(DFSDM_DATA_RES - 1), max);
>> +		*val2 = chan->scan_type.realbits;
>> +		if (adc->sd_chan[idx].differential)
>> +			*val *= 2;
>> +
>> +		return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		/*
>> +		 * DFSDM output data are in the range [-2^n,2^n-1],
>> +		 * with n=realbits-1.
>> +		 * - Differential modulator:
>> +		 * Offset correspond to SD modulator offset.
>> +		 * - Single ended modulator:
>> +		 * Input is in [0V,Vref] range, where 0V corresponds to -2^n.
>> +		 * Add 2^n to offset. (i.e. middle of input range)
>> +		 * offset = offset(sd) * vref / res(sd) * max / vref.
>> +		 */
>> +		*val = div_u64((u64)max * adc->sd_chan[idx].offset,
>> +			       BIT(adc->sd_chan[idx].scale_val2 - 1));
>> +		if (!adc->sd_chan[idx].differential)
>> +			*val += max;
>> +
>> +		return IIO_VAL_INT;
>>   	}
>>   
>>   	return -EINVAL;
>> @@ -1430,7 +1477,9 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
>>   	 * IIO_CHAN_INFO_RAW: used to compute regular conversion
>>   	 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
>>   	 */
>> -	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> +	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +				 BIT(IIO_CHAN_INFO_SCALE) |
>> +				 BIT(IIO_CHAN_INFO_OFFSET);
>>   	ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
>>   					BIT(IIO_CHAN_INFO_SAMP_FREQ);
>>   
>> @@ -1481,8 +1530,10 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
>>   {
>>   	struct iio_chan_spec *ch;
>>   	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> +	struct iio_channel *channels, *chan;
>> +	struct stm32_dfsdm_sd_chan_info *sd_chan;
>>   	int num_ch;
>> -	int ret, chan_idx;
>> +	int ret, chan_idx, val2;
>>   
>>   	adc->oversamp = DFSDM_DEFAULT_OVERSAMPLING;
>>   	ret = stm32_dfsdm_compute_all_osrs(indio_dev, adc->oversamp);
>> @@ -1506,6 +1557,22 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
>>   	if (!ch)
>>   		return -ENOMEM;
>>   
>> +	/* Get SD modulator channels */
>> +	channels = iio_channel_get_all(&indio_dev->dev);
>> +	if (IS_ERR(channels)) {
>> +		dev_err(&indio_dev->dev, "Failed to get channel %ld\n",
>> +			PTR_ERR(channels));
>> +		return PTR_ERR(channels);
>> +	}
>> +	chan = &channels[0];
>> +
>> +	adc->sd_chan = devm_kzalloc(&indio_dev->dev,
>> +				    sizeof(*adc->sd_chan) * num_ch, GFP_KERNEL);
>> +	if (!adc->sd_chan)
>> +		return -ENOMEM;
>> +
>> +	sd_chan = adc->sd_chan;
>> +
>>   	for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
>>   		ch[chan_idx].scan_index = chan_idx;
>>   		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &ch[chan_idx]);
>> @@ -1513,6 +1580,38 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
>>   			dev_err(&indio_dev->dev, "Channels init failed\n");
>>   			return ret;
>>   		}
>> +
>> +		if (!chan->indio_dev)
>> +			return -EINVAL;
>> +
>> +		ret = iio_read_channel_scale(chan, &sd_chan->scale_val,
>> +					     &sd_chan->scale_val2);
>> +		if (ret < 0) {
>> +			dev_err(&indio_dev->dev,
>> +				"Failed to get channel %d scale\n", chan_idx);
>> +			return ret;
>> +		}
>> +
>> +		if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_OFFSET)) {
>> +			ret = iio_read_channel_offset(chan, &sd_chan->offset,
>> +						      &val2);
>> +			if (ret < 0) {
>> +				dev_err(&indio_dev->dev,
>> +					"Failed to get channel %d offset\n",
>> +					chan_idx);
>> +				return ret;
>> +			}
>> +		}
>> +
>> +		sd_chan->differential = chan->channel->differential;
>> +
>> +		dev_dbg(&indio_dev->dev, "Channel %d %s scale ref=%d offset=%d",
>> +			chan_idx, chan->channel->differential ?
>> +			"differential" : "single-ended",
>> +			sd_chan->scale_val, sd_chan->offset);
>> +
>> +		chan++;
>> +		sd_chan++;
>>   	}
>>   
>>   	indio_dev->num_channels = num_ch;

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] iio: adc: sd modulator: add scale support
  2020-02-08 16:03   ` Jonathan Cameron
@ 2020-02-11 15:22     ` Olivier MOYSAN
  0 siblings, 0 replies; 15+ messages in thread
From: Olivier MOYSAN @ 2020-02-11 15:22 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: robh+dt, mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

Hi Jonathan,

On 2/8/20 5:03 PM, Jonathan Cameron wrote:
> On Tue, 4 Feb 2020 11:10:06 +0100
> Olivier Moysan <olivier.moysan@st.com> wrote:
>
>> Add scale support to sigma delta modulator.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
> I note below that there are probably some complexities around
> whether vref is used as you have it here or not.
>
> A few other bits inline around a race condition introduced in probe / remove.
>
> Thanks,
>
> Jonathan
>
>> ---
>>   drivers/iio/adc/sd_adc_modulator.c | 108 ++++++++++++++++++++++++++---
>>   1 file changed, 100 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iio/adc/sd_adc_modulator.c b/drivers/iio/adc/sd_adc_modulator.c
>> index 560d8c7d9d86..a83f35832050 100644
>> --- a/drivers/iio/adc/sd_adc_modulator.c
>> +++ b/drivers/iio/adc/sd_adc_modulator.c
>> @@ -10,8 +10,7 @@
>>   #include <linux/iio/triggered_buffer.h>
>>   #include <linux/module.h>
>>   #include <linux/of_device.h>
>> -
>> -static const struct iio_info iio_sd_mod_iio_info;
>> +#include <linux/regulator/consumer.h>
>>   
>>   static const struct iio_chan_spec iio_sd_mod_ch = {
>>   	.type = IIO_VOLTAGE,
>> @@ -21,34 +20,126 @@ static const struct iio_chan_spec iio_sd_mod_ch = {
>>   		.realbits = 1,
>>   		.shift = 0,
>>   	},
>> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> This relies on generic sigma delta modulators using an external vref.
> They might have an internal always on regulator...
> I would suggest we only support scale for devices with explicitly
> defined compatibles where we can know what regulators are used etc.
>
> For many devices, there will be a single powersupply called vdd-supply
> or similar in DT.  It may also provide a reference voltage.
I will remove scale support for generic sd-modulator,
according to your comment on sd modulator bindings.
The DFSDM driver expects scale attribute from sd-modulator.
So, some rework of DFSDM driver will be required
to also support raw data reading.

>> +};
>> +
>> +static const struct iio_chan_spec iio_sd_mod_ch_ads = {
>> +	.type = IIO_VOLTAGE,
>> +	.indexed = 1,
>> +	.scan_type = {
>> +		.sign = 'u',
>> +		.realbits = 1,
>> +		.shift = 0,
>> +	},
>> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>> +	.differential = 1,
>> +};
>> +
>> +struct iio_sd_mod_priv {
>> +	struct regulator *vref;
>> +	int vref_mv;
>> +};
>> +
>> +static int iio_sd_mod_read_raw(struct iio_dev *indio_dev,
>> +			       struct iio_chan_spec const *chan, int *val,
>> +			       int *val2, long mask)
>> +{
>> +	struct iio_sd_mod_priv *priv = iio_priv(indio_dev);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*val = priv->vref_mv;
>> +		*val2 = chan->scan_type.realbits;
>> +		return IIO_VAL_INT;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_info iio_sd_mod_iio_info = {
>> +	.read_raw = iio_sd_mod_read_raw,
>>   };
>>   
>>   static int iio_sd_mod_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> +	struct iio_sd_mod_priv *priv;
>>   	struct iio_dev *iio;
>> +	int ret;
>>   
>> -	iio = devm_iio_device_alloc(dev, 0);
>> +	iio = devm_iio_device_alloc(dev, sizeof(*priv));
>>   	if (!iio)
>>   		return -ENOMEM;
>>   
>> +	iio->channels = (const struct iio_chan_spec *)
>> +					of_device_get_match_data(&pdev->dev);
>> +
>> +	priv = iio_priv(iio);
>> +
>>   	iio->dev.parent = dev;
>>   	iio->dev.of_node = dev->of_node;
>>   	iio->name = dev_name(dev);
>>   	iio->info = &iio_sd_mod_iio_info;
>>   	iio->modes = INDIO_BUFFER_HARDWARE;
>> -
>>   	iio->num_channels = 1;
>> -	iio->channels = &iio_sd_mod_ch;
>>   
>>   	platform_set_drvdata(pdev, iio);
>>   
>> -	return devm_iio_device_register(&pdev->dev, iio);
>> +	priv->vref = devm_regulator_get_optional(dev, "vref");
>> +	if (IS_ERR(priv->vref)) {
>> +		ret = PTR_ERR(priv->vref);
>> +		if (ret != -ENODEV) {
>> +			if (ret != -EPROBE_DEFER)
>> +				dev_err(dev, "vref get failed, %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	if (!IS_ERR(priv->vref)) {
>> +		ret = regulator_enable(priv->vref);
>> +		if (ret < 0) {
>> +			dev_err(dev, "vref enable failed %d\n", ret);
>> +			return ret;
>> +		}
>> +
>> +		ret = regulator_get_voltage(priv->vref);
>> +		if (ret < 0) {
>> +			dev_err(dev, "vref get failed, %d\n", ret);
>> +			goto err_regulator_disable;
>> +		}
>> +
>> +		priv->vref_mv = ret / 1000;
>> +		dev_dbg(dev, "vref+=%dmV\n", priv->vref_mv);
>> +	}
>> +
>> +	ret = devm_iio_device_register(&pdev->dev, iio);
> This exposes the userspace and in kernel interfaces.  Those
> are partly dependent on the regulator enable you have above.
>
> Using devm_ version fo this interface leaves you with a race in remove.
> The regulator is disabled before you have remove the interfaces that
> will only work if we assume it is still on.
>
> Hence, you should either use devm_add_action_or_reset magic
> to ensure we still do everything in the right order, or do it
> manually by using iio_device_register and iio_device_unregister.
>
I will fix this in v2.

Regards
Olivier
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to register sd-modulator, %d\n", ret);
>> +		goto err_regulator_disable;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_regulator_disable:
>> +	regulator_disable(priv->vref);
>> +
>> +	return ret;
>> +}
>> +
>> +static int iio_sd_mod_remove(struct platform_device *pdev)
>> +{
>> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +	struct iio_sd_mod_priv *priv = iio_priv(indio_dev);
>> +
>> +	if (priv->vref)
>> +		return regulator_disable(priv->vref);
>> +
>> +	return 0;
>>   }
>>   
>>   static const struct of_device_id sd_adc_of_match[] = {
>> -	{ .compatible = "sd-modulator" },
>> -	{ .compatible = "ads1201" },
>> +	{ .compatible = "sd-modulator", .data = &iio_sd_mod_ch },
>> +	{ .compatible = "ads1201", .data = &iio_sd_mod_ch_ads },
>>   	{ }
>>   };
>>   MODULE_DEVICE_TABLE(of, sd_adc_of_match);
>> @@ -59,6 +150,7 @@ static struct platform_driver iio_sd_mod_adc = {
>>   		.of_match_table = of_match_ptr(sd_adc_of_match),
>>   	},
>>   	.probe = iio_sd_mod_probe,
>> +	.remove = iio_sd_mod_remove,
>>   };
>>   
>>   module_platform_driver(iio_sd_mod_adc);

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] dt-bindings: iio: adc: sd modulator: add vref support
  2020-02-08 16:04   ` Jonathan Cameron
@ 2020-02-11 15:24     ` Olivier MOYSAN
  0 siblings, 0 replies; 15+ messages in thread
From: Olivier MOYSAN @ 2020-02-11 15:24 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: robh+dt, mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

On 2/8/20 5:04 PM, Jonathan Cameron wrote:
> On Tue, 4 Feb 2020 11:10:05 +0100
> Olivier Moysan <olivier.moysan@st.com> wrote:
>
>> Add vref supply support to sigma delta modulator.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
>> ---
>>   .../devicetree/bindings/iio/adc/sigma-delta-modulator.yaml    | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml b/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml
>> index a390343d0c2a..2afe0765e971 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml
>> @@ -8,6 +8,7 @@ title: Device-Tree bindings for sigma delta modulator
>>   
>>   maintainers:
>>     - Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> +  - Olivier Moysan <olivier.moysan@st.com>
>>   
>>   properties:
>>     compatible:
>> @@ -21,6 +22,9 @@ properties:
>>     '#io-channel-cells':
>>       const: 0
>>   
>> +  vref-supply:
>> +    description: Phandle to the vref input analog reference voltage.
> I note this in review of patch 2 but in general I'm not sure we should
> be introducing this for generic devices.   It's fine if we have an
> explicit compatible but there is no reason to assume a generic sd-modulator
> uses an external reference.
>
> Jonathan
Ok, I will remove reference to external voltage for generic 
sd-modulator, in v2.
I will add it for ads1201 compatible, instead.

Thanks for your review
Olivier
>> +
>>   required:
>>     - compatible
>>     - '#io-channel-cells'

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] iio: adc: stm32-dfsdm: add scale and offset support
  2020-02-11 15:19     ` Olivier MOYSAN
@ 2020-02-14 13:11       ` Jonathan Cameron
  2020-02-14 14:49         ` Olivier MOYSAN
  0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2020-02-14 13:11 UTC (permalink / raw)
  To: Olivier MOYSAN
  Cc: robh+dt, mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

On Tue, 11 Feb 2020 15:19:01 +0000
Olivier MOYSAN <olivier.moysan@st.com> wrote:

> Hi Jonathan,
> 
> On 2/8/20 5:18 PM, Jonathan Cameron wrote:
> > On Tue, 4 Feb 2020 11:10:08 +0100
> > Olivier Moysan <olivier.moysan@st.com> wrote:
> >  
> >> Add scale and offset attributes support to STM32 DFSDM.
> >>
> >> Signed-off-by: Olivier Moysan <olivier.moysan@st.com>  
> > Hmm. I can't remember this history of this but we've kind of
> > ended up backwards wrt to other consumer drivers.
> >
> > In some sense this is similar to the analog gyroscopes.  In those
> > the consumer driver is the gyroscope which is consuming the raw
> > readings from an ADC connected to the channel.  This results
> > in us getting readings reported by the gyroscope driver.
> >
> > Here we have a sigma delta convertor consuming the pulse train
> > from a sigma delta device.  So the channels are reported by
> > the sigma delta receiver, whereas i think the nearest equivalent
> > to the analog voltage outputing gyroscopes would have been if
> > we had reported the channel values at the sigma delta converter.  
> The DFSDM driver is currently used as a consumer of the sd modulator.
> The scale and offset values of the channels are already computed by
> the DFSDM driver, and provided by this driver to the IIO ABI.
> However, the DFSDM has no voltage reference, so it has to retrieve
> it from sd-modulator channels, for the scale factor computation.
> 
>                                      scale  offset
>                                        ^      ^
>                                        |      |       IIO ABI
> +-------------------------------------------------------------+
>           +---------------+          +-------------+
>           |sd driver      |          |DFSDM driver |
>           +---------------+          +-------------+
> +-------------------------------------------------------------+
>                                                           HW
>           +---------------+          +-------------+
> +------->+ sd-modulator  +--------->+ DFSDM +-------->
> analog   +------+--------+          +-------------+ output
> input           ^
>                  | vref
>                  +
> 
> 
> Is it the topology your are expecting ?

It's not the one we'd expect if we are aligning with similar cases
elsewhere in IIO.  For example, if we attach an analog accelerometer
to an ADC, we report the accel channels on the accelerometer not the
ADC.  The equivalent would be to see the DFSDM as providing a
conversion service to the SD device which is actually executing
the measurement and has the input channels.


         scale  offset  raw
           ^      ^      ^
           |      |      |                              IIO ABI
 +-------------------------------------------------------------+
           +---------------+          +-------------+
           |sd driver      |          |DFSDM driver |
           +---------------+          +-------------+
 +-------------------------------------------------------------+
                                                           HW
           +---------------+          +-------------+
 +------->+ sd-modulator  +--------->+ DFSDM +-------->
 analog   +------+--------+          +-------------+ output
 input           ^
                  | vref
>                  +
> 

> If not, I probably missedsomething. Could you please clarify this point ?
> 
> Regards
> Olivier
> > This wasn't really an issue when the only values available were
> > raw, but if we are adding scale and offset, they are things that
> > belong to the ad1201 for example, not the upstream stm32-dfsdm unit.
> >
> > Thinking of it another way, we don't report an SPI ADC output in
> > the driver for the SPI master.
> >
> > Could we flip it around without breaking anything?
> >
> > Jonathan
> >  
> >> ---
> >>   drivers/iio/adc/stm32-dfsdm-adc.c | 105 +++++++++++++++++++++++++++++-
> >>   1 file changed, 102 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> >> index 07b9dfdf8e76..b85fd3e90496 100644
> >> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> >> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> >> @@ -10,6 +10,7 @@
> >>   #include <linux/dma-mapping.h>
> >>   #include <linux/iio/adc/stm32-dfsdm-adc.h>
> >>   #include <linux/iio/buffer.h>
> >> +#include <linux/iio/consumer.h>
> >>   #include <linux/iio/hw-consumer.h>
> >>   #include <linux/iio/sysfs.h>
> >>   #include <linux/iio/timer/stm32-lptim-trigger.h>
> >> @@ -67,6 +68,13 @@ struct stm32_dfsdm_dev_data {
> >>   	const struct regmap_config *regmap_cfg;
> >>   };
> >>   
> >> +struct stm32_dfsdm_sd_chan_info {
> >> +	int scale_val;
> >> +	int scale_val2;
> >> +	int offset;
> >> +	unsigned int differential;
> >> +};
> >> +
> >>   struct stm32_dfsdm_adc {
> >>   	struct stm32_dfsdm *dfsdm;
> >>   	const struct stm32_dfsdm_dev_data *dev_data;
> >> @@ -79,6 +87,7 @@ struct stm32_dfsdm_adc {
> >>   	struct iio_hw_consumer *hwc;
> >>   	struct completion completion;
> >>   	u32 *buffer;
> >> +	struct stm32_dfsdm_sd_chan_info *sd_chan;
> >>   
> >>   	/* Audio specific */
> >>   	unsigned int spi_freq;  /* SPI bus clock frequency */
> >> @@ -1271,7 +1280,10 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
> >>   				int *val2, long mask)
> >>   {
> >>   	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >> -	int ret;
> >> +	struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
> >> +	struct stm32_dfsdm_filter_osr *flo = &fl->flo[fl->fast];
> >> +	u32 max = flo->max << (flo->lshift - chan->scan_type.shift);
> >> +	int ret, idx = chan->scan_index;
> >>   
> >>   	switch (mask) {
> >>   	case IIO_CHAN_INFO_RAW:
> >> @@ -1307,6 +1319,41 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
> >>   		*val = adc->sample_freq;
> >>   
> >>   		return IIO_VAL_INT;
> >> +
> >> +	case IIO_CHAN_INFO_SCALE:
> >> +		/*
> >> +		 * Scale is expressed in mV.
> >> +		 * When fast mode is disabled, actual resolution may be lower
> >> +		 * than 2^n, where n=realbits-1.
> >> +		 * This leads to underestimating input voltage. To
> >> +		 * compensate this deviation, the voltage reference can be
> >> +		 * corrected with a factor = realbits resolution / actual max
> >> +		 */
> >> +		*val = div_u64((u64)adc->sd_chan[idx].scale_val *
> >> +			       (u64)BIT(DFSDM_DATA_RES - 1), max);
> >> +		*val2 = chan->scan_type.realbits;
> >> +		if (adc->sd_chan[idx].differential)
> >> +			*val *= 2;
> >> +
> >> +		return IIO_VAL_FRACTIONAL_LOG2;
> >> +
> >> +	case IIO_CHAN_INFO_OFFSET:
> >> +		/*
> >> +		 * DFSDM output data are in the range [-2^n,2^n-1],
> >> +		 * with n=realbits-1.
> >> +		 * - Differential modulator:
> >> +		 * Offset correspond to SD modulator offset.
> >> +		 * - Single ended modulator:
> >> +		 * Input is in [0V,Vref] range, where 0V corresponds to -2^n.
> >> +		 * Add 2^n to offset. (i.e. middle of input range)
> >> +		 * offset = offset(sd) * vref / res(sd) * max / vref.
> >> +		 */
> >> +		*val = div_u64((u64)max * adc->sd_chan[idx].offset,
> >> +			       BIT(adc->sd_chan[idx].scale_val2 - 1));
> >> +		if (!adc->sd_chan[idx].differential)
> >> +			*val += max;
> >> +
> >> +		return IIO_VAL_INT;
> >>   	}
> >>   
> >>   	return -EINVAL;
> >> @@ -1430,7 +1477,9 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
> >>   	 * IIO_CHAN_INFO_RAW: used to compute regular conversion
> >>   	 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
> >>   	 */
> >> -	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> >> +	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >> +				 BIT(IIO_CHAN_INFO_SCALE) |
> >> +				 BIT(IIO_CHAN_INFO_OFFSET);
> >>   	ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
> >>   					BIT(IIO_CHAN_INFO_SAMP_FREQ);
> >>   
> >> @@ -1481,8 +1530,10 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
> >>   {
> >>   	struct iio_chan_spec *ch;
> >>   	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >> +	struct iio_channel *channels, *chan;
> >> +	struct stm32_dfsdm_sd_chan_info *sd_chan;
> >>   	int num_ch;
> >> -	int ret, chan_idx;
> >> +	int ret, chan_idx, val2;
> >>   
> >>   	adc->oversamp = DFSDM_DEFAULT_OVERSAMPLING;
> >>   	ret = stm32_dfsdm_compute_all_osrs(indio_dev, adc->oversamp);
> >> @@ -1506,6 +1557,22 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
> >>   	if (!ch)
> >>   		return -ENOMEM;
> >>   
> >> +	/* Get SD modulator channels */
> >> +	channels = iio_channel_get_all(&indio_dev->dev);
> >> +	if (IS_ERR(channels)) {
> >> +		dev_err(&indio_dev->dev, "Failed to get channel %ld\n",
> >> +			PTR_ERR(channels));
> >> +		return PTR_ERR(channels);
> >> +	}
> >> +	chan = &channels[0];
> >> +
> >> +	adc->sd_chan = devm_kzalloc(&indio_dev->dev,
> >> +				    sizeof(*adc->sd_chan) * num_ch, GFP_KERNEL);
> >> +	if (!adc->sd_chan)
> >> +		return -ENOMEM;
> >> +
> >> +	sd_chan = adc->sd_chan;
> >> +
> >>   	for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
> >>   		ch[chan_idx].scan_index = chan_idx;
> >>   		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &ch[chan_idx]);
> >> @@ -1513,6 +1580,38 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
> >>   			dev_err(&indio_dev->dev, "Channels init failed\n");
> >>   			return ret;
> >>   		}
> >> +
> >> +		if (!chan->indio_dev)
> >> +			return -EINVAL;
> >> +
> >> +		ret = iio_read_channel_scale(chan, &sd_chan->scale_val,
> >> +					     &sd_chan->scale_val2);
> >> +		if (ret < 0) {
> >> +			dev_err(&indio_dev->dev,
> >> +				"Failed to get channel %d scale\n", chan_idx);
> >> +			return ret;
> >> +		}
> >> +
> >> +		if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_OFFSET)) {
> >> +			ret = iio_read_channel_offset(chan, &sd_chan->offset,
> >> +						      &val2);
> >> +			if (ret < 0) {
> >> +				dev_err(&indio_dev->dev,
> >> +					"Failed to get channel %d offset\n",
> >> +					chan_idx);
> >> +				return ret;
> >> +			}
> >> +		}
> >> +
> >> +		sd_chan->differential = chan->channel->differential;
> >> +
> >> +		dev_dbg(&indio_dev->dev, "Channel %d %s scale ref=%d offset=%d",
> >> +			chan_idx, chan->channel->differential ?
> >> +			"differential" : "single-ended",
> >> +			sd_chan->scale_val, sd_chan->offset);
> >> +
> >> +		chan++;
> >> +		sd_chan++;
> >>   	}
> >>   
> >>   	indio_dev->num_channels = num_ch;  


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] iio: adc: stm32-dfsdm: add scale and offset support
  2020-02-14 13:11       ` Jonathan Cameron
@ 2020-02-14 14:49         ` Olivier MOYSAN
  2020-02-14 15:10           ` Jonathan Cameron
  0 siblings, 1 reply; 15+ messages in thread
From: Olivier MOYSAN @ 2020-02-14 14:49 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: robh+dt, mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

Hi Jonathan,

On 2/14/20 2:11 PM, Jonathan Cameron wrote:
> On Tue, 11 Feb 2020 15:19:01 +0000
> Olivier MOYSAN <olivier.moysan@st.com> wrote:
>
>> Hi Jonathan,
>>
>> On 2/8/20 5:18 PM, Jonathan Cameron wrote:
>>> On Tue, 4 Feb 2020 11:10:08 +0100
>>> Olivier Moysan <olivier.moysan@st.com> wrote:
>>>   
>>>> Add scale and offset attributes support to STM32 DFSDM.
>>>>
>>>> Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
>>> Hmm. I can't remember this history of this but we've kind of
>>> ended up backwards wrt to other consumer drivers.
>>>
>>> In some sense this is similar to the analog gyroscopes.  In those
>>> the consumer driver is the gyroscope which is consuming the raw
>>> readings from an ADC connected to the channel.  This results
>>> in us getting readings reported by the gyroscope driver.
>>>
>>> Here we have a sigma delta convertor consuming the pulse train
>>> from a sigma delta device.  So the channels are reported by
>>> the sigma delta receiver, whereas i think the nearest equivalent
>>> to the analog voltage outputing gyroscopes would have been if
>>> we had reported the channel values at the sigma delta converter.
>> The DFSDM driver is currently used as a consumer of the sd modulator.
>> The scale and offset values of the channels are already computed by
>> the DFSDM driver, and provided by this driver to the IIO ABI.
>> However, the DFSDM has no voltage reference, so it has to retrieve
>> it from sd-modulator channels, for the scale factor computation.
>>
>>                                       scale  offset
>>                                         ^      ^
>>                                         |      |       IIO ABI
>> +-------------------------------------------------------------+
>>            +---------------+          +-------------+
>>            |sd driver      |          |DFSDM driver |
>>            +---------------+          +-------------+
>> +-------------------------------------------------------------+
>>                                                            HW
>>            +---------------+          +-------------+
>> +------->+ sd-modulator  +--------->+ DFSDM +-------->
>> analog   +------+--------+          +-------------+ output
>> input           ^
>>                   | vref
>>                   +
>>
>>
>> Is it the topology your are expecting ?
> It's not the one we'd expect if we are aligning with similar cases
> elsewhere in IIO.  For example, if we attach an analog accelerometer
> to an ADC, we report the accel channels on the accelerometer not the
> ADC.  The equivalent would be to see the DFSDM as providing a
> conversion service to the SD device which is actually executing
> the measurement and has the input channels.
>
>
>           scale  offset  raw
>             ^      ^      ^
>             |      |      |                              IIO ABI
>   +-------------------------------------------------------------+
>             +---------------+          +-------------+
>             |sd driver      |          |DFSDM driver |
>             +---------------+          +-------------+
>   +-------------------------------------------------------------+
>                                                             HW
>             +---------------+          +-------------+
>   +------->+ sd-modulator  +--------->+ DFSDM +-------->
>   analog   +------+--------+          +-------------+ output
>   input           ^
>                    | vref
>>                   +
>>
Thanks for your clarification.
ok, moving to this logic is a significant change.
I need to evaluate further the impact on the dfsdm driver.

Regards
Olivier
>> If not, I probably missedsomething. Could you please clarify this point ?
>>
>> Regards
>> Olivier
>>> This wasn't really an issue when the only values available were
>>> raw, but if we are adding scale and offset, they are things that
>>> belong to the ad1201 for example, not the upstream stm32-dfsdm unit.
>>>
>>> Thinking of it another way, we don't report an SPI ADC output in
>>> the driver for the SPI master.
>>>
>>> Could we flip it around without breaking anything?
>>>
>>> Jonathan
>>>   
>>>> ---
>>>>    drivers/iio/adc/stm32-dfsdm-adc.c | 105 +++++++++++++++++++++++++++++-
>>>>    1 file changed, 102 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
>>>> index 07b9dfdf8e76..b85fd3e90496 100644
>>>> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
>>>> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
>>>> @@ -10,6 +10,7 @@
>>>>    #include <linux/dma-mapping.h>
>>>>    #include <linux/iio/adc/stm32-dfsdm-adc.h>
>>>>    #include <linux/iio/buffer.h>
>>>> +#include <linux/iio/consumer.h>
>>>>    #include <linux/iio/hw-consumer.h>
>>>>    #include <linux/iio/sysfs.h>
>>>>    #include <linux/iio/timer/stm32-lptim-trigger.h>
>>>> @@ -67,6 +68,13 @@ struct stm32_dfsdm_dev_data {
>>>>    	const struct regmap_config *regmap_cfg;
>>>>    };
>>>>    
>>>> +struct stm32_dfsdm_sd_chan_info {
>>>> +	int scale_val;
>>>> +	int scale_val2;
>>>> +	int offset;
>>>> +	unsigned int differential;
>>>> +};
>>>> +
>>>>    struct stm32_dfsdm_adc {
>>>>    	struct stm32_dfsdm *dfsdm;
>>>>    	const struct stm32_dfsdm_dev_data *dev_data;
>>>> @@ -79,6 +87,7 @@ struct stm32_dfsdm_adc {
>>>>    	struct iio_hw_consumer *hwc;
>>>>    	struct completion completion;
>>>>    	u32 *buffer;
>>>> +	struct stm32_dfsdm_sd_chan_info *sd_chan;
>>>>    
>>>>    	/* Audio specific */
>>>>    	unsigned int spi_freq;  /* SPI bus clock frequency */
>>>> @@ -1271,7 +1280,10 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>>>>    				int *val2, long mask)
>>>>    {
>>>>    	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>>>> -	int ret;
>>>> +	struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
>>>> +	struct stm32_dfsdm_filter_osr *flo = &fl->flo[fl->fast];
>>>> +	u32 max = flo->max << (flo->lshift - chan->scan_type.shift);
>>>> +	int ret, idx = chan->scan_index;
>>>>    
>>>>    	switch (mask) {
>>>>    	case IIO_CHAN_INFO_RAW:
>>>> @@ -1307,6 +1319,41 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>>>>    		*val = adc->sample_freq;
>>>>    
>>>>    		return IIO_VAL_INT;
>>>> +
>>>> +	case IIO_CHAN_INFO_SCALE:
>>>> +		/*
>>>> +		 * Scale is expressed in mV.
>>>> +		 * When fast mode is disabled, actual resolution may be lower
>>>> +		 * than 2^n, where n=realbits-1.
>>>> +		 * This leads to underestimating input voltage. To
>>>> +		 * compensate this deviation, the voltage reference can be
>>>> +		 * corrected with a factor = realbits resolution / actual max
>>>> +		 */
>>>> +		*val = div_u64((u64)adc->sd_chan[idx].scale_val *
>>>> +			       (u64)BIT(DFSDM_DATA_RES - 1), max);
>>>> +		*val2 = chan->scan_type.realbits;
>>>> +		if (adc->sd_chan[idx].differential)
>>>> +			*val *= 2;
>>>> +
>>>> +		return IIO_VAL_FRACTIONAL_LOG2;
>>>> +
>>>> +	case IIO_CHAN_INFO_OFFSET:
>>>> +		/*
>>>> +		 * DFSDM output data are in the range [-2^n,2^n-1],
>>>> +		 * with n=realbits-1.
>>>> +		 * - Differential modulator:
>>>> +		 * Offset correspond to SD modulator offset.
>>>> +		 * - Single ended modulator:
>>>> +		 * Input is in [0V,Vref] range, where 0V corresponds to -2^n.
>>>> +		 * Add 2^n to offset. (i.e. middle of input range)
>>>> +		 * offset = offset(sd) * vref / res(sd) * max / vref.
>>>> +		 */
>>>> +		*val = div_u64((u64)max * adc->sd_chan[idx].offset,
>>>> +			       BIT(adc->sd_chan[idx].scale_val2 - 1));
>>>> +		if (!adc->sd_chan[idx].differential)
>>>> +			*val += max;
>>>> +
>>>> +		return IIO_VAL_INT;
>>>>    	}
>>>>    
>>>>    	return -EINVAL;
>>>> @@ -1430,7 +1477,9 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
>>>>    	 * IIO_CHAN_INFO_RAW: used to compute regular conversion
>>>>    	 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
>>>>    	 */
>>>> -	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>>>> +	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>> +				 BIT(IIO_CHAN_INFO_SCALE) |
>>>> +				 BIT(IIO_CHAN_INFO_OFFSET);
>>>>    	ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
>>>>    					BIT(IIO_CHAN_INFO_SAMP_FREQ);
>>>>    
>>>> @@ -1481,8 +1530,10 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
>>>>    {
>>>>    	struct iio_chan_spec *ch;
>>>>    	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>>>> +	struct iio_channel *channels, *chan;
>>>> +	struct stm32_dfsdm_sd_chan_info *sd_chan;
>>>>    	int num_ch;
>>>> -	int ret, chan_idx;
>>>> +	int ret, chan_idx, val2;
>>>>    
>>>>    	adc->oversamp = DFSDM_DEFAULT_OVERSAMPLING;
>>>>    	ret = stm32_dfsdm_compute_all_osrs(indio_dev, adc->oversamp);
>>>> @@ -1506,6 +1557,22 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
>>>>    	if (!ch)
>>>>    		return -ENOMEM;
>>>>    
>>>> +	/* Get SD modulator channels */
>>>> +	channels = iio_channel_get_all(&indio_dev->dev);
>>>> +	if (IS_ERR(channels)) {
>>>> +		dev_err(&indio_dev->dev, "Failed to get channel %ld\n",
>>>> +			PTR_ERR(channels));
>>>> +		return PTR_ERR(channels);
>>>> +	}
>>>> +	chan = &channels[0];
>>>> +
>>>> +	adc->sd_chan = devm_kzalloc(&indio_dev->dev,
>>>> +				    sizeof(*adc->sd_chan) * num_ch, GFP_KERNEL);
>>>> +	if (!adc->sd_chan)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	sd_chan = adc->sd_chan;
>>>> +
>>>>    	for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
>>>>    		ch[chan_idx].scan_index = chan_idx;
>>>>    		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &ch[chan_idx]);
>>>> @@ -1513,6 +1580,38 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
>>>>    			dev_err(&indio_dev->dev, "Channels init failed\n");
>>>>    			return ret;
>>>>    		}
>>>> +
>>>> +		if (!chan->indio_dev)
>>>> +			return -EINVAL;
>>>> +
>>>> +		ret = iio_read_channel_scale(chan, &sd_chan->scale_val,
>>>> +					     &sd_chan->scale_val2);
>>>> +		if (ret < 0) {
>>>> +			dev_err(&indio_dev->dev,
>>>> +				"Failed to get channel %d scale\n", chan_idx);
>>>> +			return ret;
>>>> +		}
>>>> +
>>>> +		if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_OFFSET)) {
>>>> +			ret = iio_read_channel_offset(chan, &sd_chan->offset,
>>>> +						      &val2);
>>>> +			if (ret < 0) {
>>>> +				dev_err(&indio_dev->dev,
>>>> +					"Failed to get channel %d offset\n",
>>>> +					chan_idx);
>>>> +				return ret;
>>>> +			}
>>>> +		}
>>>> +
>>>> +		sd_chan->differential = chan->channel->differential;
>>>> +
>>>> +		dev_dbg(&indio_dev->dev, "Channel %d %s scale ref=%d offset=%d",
>>>> +			chan_idx, chan->channel->differential ?
>>>> +			"differential" : "single-ended",
>>>> +			sd_chan->scale_val, sd_chan->offset);
>>>> +
>>>> +		chan++;
>>>> +		sd_chan++;
>>>>    	}
>>>>    
>>>>    	indio_dev->num_channels = num_ch;

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] iio: adc: stm32-dfsdm: add scale and offset support
  2020-02-14 14:49         ` Olivier MOYSAN
@ 2020-02-14 15:10           ` Jonathan Cameron
  0 siblings, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2020-02-14 15:10 UTC (permalink / raw)
  To: Olivier MOYSAN
  Cc: robh+dt, mark.rutland, knaack.h, lars, devicetree, linux-iio,
	linux-kernel, pmeerw, linux-stm32, linux-arm-kernel

On Fri, 14 Feb 2020 14:49:18 +0000
Olivier MOYSAN <olivier.moysan@st.com> wrote:

> Hi Jonathan,
> 
> On 2/14/20 2:11 PM, Jonathan Cameron wrote:
> > On Tue, 11 Feb 2020 15:19:01 +0000
> > Olivier MOYSAN <olivier.moysan@st.com> wrote:
> >  
> >> Hi Jonathan,
> >>
> >> On 2/8/20 5:18 PM, Jonathan Cameron wrote:  
> >>> On Tue, 4 Feb 2020 11:10:08 +0100
> >>> Olivier Moysan <olivier.moysan@st.com> wrote:
> >>>     
> >>>> Add scale and offset attributes support to STM32 DFSDM.
> >>>>
> >>>> Signed-off-by: Olivier Moysan <olivier.moysan@st.com>  
> >>> Hmm. I can't remember this history of this but we've kind of
> >>> ended up backwards wrt to other consumer drivers.
> >>>
> >>> In some sense this is similar to the analog gyroscopes.  In those
> >>> the consumer driver is the gyroscope which is consuming the raw
> >>> readings from an ADC connected to the channel.  This results
> >>> in us getting readings reported by the gyroscope driver.
> >>>
> >>> Here we have a sigma delta convertor consuming the pulse train
> >>> from a sigma delta device.  So the channels are reported by
> >>> the sigma delta receiver, whereas i think the nearest equivalent
> >>> to the analog voltage outputing gyroscopes would have been if
> >>> we had reported the channel values at the sigma delta converter.  
> >> The DFSDM driver is currently used as a consumer of the sd modulator.
> >> The scale and offset values of the channels are already computed by
> >> the DFSDM driver, and provided by this driver to the IIO ABI.
> >> However, the DFSDM has no voltage reference, so it has to retrieve
> >> it from sd-modulator channels, for the scale factor computation.
> >>
> >>                                       scale  offset
> >>                                         ^      ^
> >>                                         |      |       IIO ABI
> >> +-------------------------------------------------------------+
> >>            +---------------+          +-------------+
> >>            |sd driver      |          |DFSDM driver |
> >>            +---------------+          +-------------+
> >> +-------------------------------------------------------------+
> >>                                                            HW
> >>            +---------------+          +-------------+
> >> +------->+ sd-modulator  +--------->+ DFSDM +-------->
> >> analog   +------+--------+          +-------------+ output
> >> input           ^
> >>                   | vref
> >>                   +
> >>
> >>
> >> Is it the topology your are expecting ?  
> > It's not the one we'd expect if we are aligning with similar cases
> > elsewhere in IIO.  For example, if we attach an analog accelerometer
> > to an ADC, we report the accel channels on the accelerometer not the
> > ADC.  The equivalent would be to see the DFSDM as providing a
> > conversion service to the SD device which is actually executing
> > the measurement and has the input channels.
> >
> >
> >           scale  offset  raw
> >             ^      ^      ^
> >             |      |      |                              IIO ABI
> >   +-------------------------------------------------------------+
> >             +---------------+          +-------------+
> >             |sd driver      |          |DFSDM driver |
> >             +---------------+          +-------------+
> >   +-------------------------------------------------------------+
> >                                                             HW
> >             +---------------+          +-------------+
> >   +------->+ sd-modulator  +--------->+ DFSDM +-------->
> >   analog   +------+--------+          +-------------+ output
> >   input           ^
> >                    | vref  
> >>                   +
> >>  
> Thanks for your clarification.
> ok, moving to this logic is a significant change.
> I need to evaluate further the impact on the dfsdm driver.

Understood!  If we can't do it without potentially breaking users then
such is life.

Jonathan

> 
> Regards
> Olivier
> >> If not, I probably missedsomething. Could you please clarify this point ?
> >>
> >> Regards
> >> Olivier  
> >>> This wasn't really an issue when the only values available were
> >>> raw, but if we are adding scale and offset, they are things that
> >>> belong to the ad1201 for example, not the upstream stm32-dfsdm unit.
> >>>
> >>> Thinking of it another way, we don't report an SPI ADC output in
> >>> the driver for the SPI master.
> >>>
> >>> Could we flip it around without breaking anything?
> >>>
> >>> Jonathan
> >>>     
> >>>> ---
> >>>>    drivers/iio/adc/stm32-dfsdm-adc.c | 105 +++++++++++++++++++++++++++++-
> >>>>    1 file changed, 102 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> >>>> index 07b9dfdf8e76..b85fd3e90496 100644
> >>>> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> >>>> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> >>>> @@ -10,6 +10,7 @@
> >>>>    #include <linux/dma-mapping.h>
> >>>>    #include <linux/iio/adc/stm32-dfsdm-adc.h>
> >>>>    #include <linux/iio/buffer.h>
> >>>> +#include <linux/iio/consumer.h>
> >>>>    #include <linux/iio/hw-consumer.h>
> >>>>    #include <linux/iio/sysfs.h>
> >>>>    #include <linux/iio/timer/stm32-lptim-trigger.h>
> >>>> @@ -67,6 +68,13 @@ struct stm32_dfsdm_dev_data {
> >>>>    	const struct regmap_config *regmap_cfg;
> >>>>    };
> >>>>    
> >>>> +struct stm32_dfsdm_sd_chan_info {
> >>>> +	int scale_val;
> >>>> +	int scale_val2;
> >>>> +	int offset;
> >>>> +	unsigned int differential;
> >>>> +};
> >>>> +
> >>>>    struct stm32_dfsdm_adc {
> >>>>    	struct stm32_dfsdm *dfsdm;
> >>>>    	const struct stm32_dfsdm_dev_data *dev_data;
> >>>> @@ -79,6 +87,7 @@ struct stm32_dfsdm_adc {
> >>>>    	struct iio_hw_consumer *hwc;
> >>>>    	struct completion completion;
> >>>>    	u32 *buffer;
> >>>> +	struct stm32_dfsdm_sd_chan_info *sd_chan;
> >>>>    
> >>>>    	/* Audio specific */
> >>>>    	unsigned int spi_freq;  /* SPI bus clock frequency */
> >>>> @@ -1271,7 +1280,10 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
> >>>>    				int *val2, long mask)
> >>>>    {
> >>>>    	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >>>> -	int ret;
> >>>> +	struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
> >>>> +	struct stm32_dfsdm_filter_osr *flo = &fl->flo[fl->fast];
> >>>> +	u32 max = flo->max << (flo->lshift - chan->scan_type.shift);
> >>>> +	int ret, idx = chan->scan_index;
> >>>>    
> >>>>    	switch (mask) {
> >>>>    	case IIO_CHAN_INFO_RAW:
> >>>> @@ -1307,6 +1319,41 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
> >>>>    		*val = adc->sample_freq;
> >>>>    
> >>>>    		return IIO_VAL_INT;
> >>>> +
> >>>> +	case IIO_CHAN_INFO_SCALE:
> >>>> +		/*
> >>>> +		 * Scale is expressed in mV.
> >>>> +		 * When fast mode is disabled, actual resolution may be lower
> >>>> +		 * than 2^n, where n=realbits-1.
> >>>> +		 * This leads to underestimating input voltage. To
> >>>> +		 * compensate this deviation, the voltage reference can be
> >>>> +		 * corrected with a factor = realbits resolution / actual max
> >>>> +		 */
> >>>> +		*val = div_u64((u64)adc->sd_chan[idx].scale_val *
> >>>> +			       (u64)BIT(DFSDM_DATA_RES - 1), max);
> >>>> +		*val2 = chan->scan_type.realbits;
> >>>> +		if (adc->sd_chan[idx].differential)
> >>>> +			*val *= 2;
> >>>> +
> >>>> +		return IIO_VAL_FRACTIONAL_LOG2;
> >>>> +
> >>>> +	case IIO_CHAN_INFO_OFFSET:
> >>>> +		/*
> >>>> +		 * DFSDM output data are in the range [-2^n,2^n-1],
> >>>> +		 * with n=realbits-1.
> >>>> +		 * - Differential modulator:
> >>>> +		 * Offset correspond to SD modulator offset.
> >>>> +		 * - Single ended modulator:
> >>>> +		 * Input is in [0V,Vref] range, where 0V corresponds to -2^n.
> >>>> +		 * Add 2^n to offset. (i.e. middle of input range)
> >>>> +		 * offset = offset(sd) * vref / res(sd) * max / vref.
> >>>> +		 */
> >>>> +		*val = div_u64((u64)max * adc->sd_chan[idx].offset,
> >>>> +			       BIT(adc->sd_chan[idx].scale_val2 - 1));
> >>>> +		if (!adc->sd_chan[idx].differential)
> >>>> +			*val += max;
> >>>> +
> >>>> +		return IIO_VAL_INT;
> >>>>    	}
> >>>>    
> >>>>    	return -EINVAL;
> >>>> @@ -1430,7 +1477,9 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
> >>>>    	 * IIO_CHAN_INFO_RAW: used to compute regular conversion
> >>>>    	 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
> >>>>    	 */
> >>>> -	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> >>>> +	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >>>> +				 BIT(IIO_CHAN_INFO_SCALE) |
> >>>> +				 BIT(IIO_CHAN_INFO_OFFSET);
> >>>>    	ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
> >>>>    					BIT(IIO_CHAN_INFO_SAMP_FREQ);
> >>>>    
> >>>> @@ -1481,8 +1530,10 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
> >>>>    {
> >>>>    	struct iio_chan_spec *ch;
> >>>>    	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >>>> +	struct iio_channel *channels, *chan;
> >>>> +	struct stm32_dfsdm_sd_chan_info *sd_chan;
> >>>>    	int num_ch;
> >>>> -	int ret, chan_idx;
> >>>> +	int ret, chan_idx, val2;
> >>>>    
> >>>>    	adc->oversamp = DFSDM_DEFAULT_OVERSAMPLING;
> >>>>    	ret = stm32_dfsdm_compute_all_osrs(indio_dev, adc->oversamp);
> >>>> @@ -1506,6 +1557,22 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
> >>>>    	if (!ch)
> >>>>    		return -ENOMEM;
> >>>>    
> >>>> +	/* Get SD modulator channels */
> >>>> +	channels = iio_channel_get_all(&indio_dev->dev);
> >>>> +	if (IS_ERR(channels)) {
> >>>> +		dev_err(&indio_dev->dev, "Failed to get channel %ld\n",
> >>>> +			PTR_ERR(channels));
> >>>> +		return PTR_ERR(channels);
> >>>> +	}
> >>>> +	chan = &channels[0];
> >>>> +
> >>>> +	adc->sd_chan = devm_kzalloc(&indio_dev->dev,
> >>>> +				    sizeof(*adc->sd_chan) * num_ch, GFP_KERNEL);
> >>>> +	if (!adc->sd_chan)
> >>>> +		return -ENOMEM;
> >>>> +
> >>>> +	sd_chan = adc->sd_chan;
> >>>> +
> >>>>    	for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
> >>>>    		ch[chan_idx].scan_index = chan_idx;
> >>>>    		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &ch[chan_idx]);
> >>>> @@ -1513,6 +1580,38 @@ static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
> >>>>    			dev_err(&indio_dev->dev, "Channels init failed\n");
> >>>>    			return ret;
> >>>>    		}
> >>>> +
> >>>> +		if (!chan->indio_dev)
> >>>> +			return -EINVAL;
> >>>> +
> >>>> +		ret = iio_read_channel_scale(chan, &sd_chan->scale_val,
> >>>> +					     &sd_chan->scale_val2);
> >>>> +		if (ret < 0) {
> >>>> +			dev_err(&indio_dev->dev,
> >>>> +				"Failed to get channel %d scale\n", chan_idx);
> >>>> +			return ret;
> >>>> +		}
> >>>> +
> >>>> +		if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_OFFSET)) {
> >>>> +			ret = iio_read_channel_offset(chan, &sd_chan->offset,
> >>>> +						      &val2);
> >>>> +			if (ret < 0) {
> >>>> +				dev_err(&indio_dev->dev,
> >>>> +					"Failed to get channel %d offset\n",
> >>>> +					chan_idx);
> >>>> +				return ret;
> >>>> +			}
> >>>> +		}
> >>>> +
> >>>> +		sd_chan->differential = chan->channel->differential;
> >>>> +
> >>>> +		dev_dbg(&indio_dev->dev, "Channel %d %s scale ref=%d offset=%d",
> >>>> +			chan_idx, chan->channel->differential ?
> >>>> +			"differential" : "single-ended",
> >>>> +			sd_chan->scale_val, sd_chan->offset);
> >>>> +
> >>>> +		chan++;
> >>>> +		sd_chan++;
> >>>>    	}
> >>>>    
> >>>>    	indio_dev->num_channels = num_ch;  


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-04 10:10 [PATCH 0/4] iio: adc: stm32-dfsdm: add scale and offset support Olivier Moysan
2020-02-04 10:10 ` [PATCH 1/4] dt-bindings: iio: adc: sd modulator: add vref support Olivier Moysan
2020-02-06 19:08   ` Rob Herring
2020-02-08 16:04   ` Jonathan Cameron
2020-02-11 15:24     ` Olivier MOYSAN
2020-02-04 10:10 ` [PATCH 2/4] iio: adc: sd modulator: add scale support Olivier Moysan
2020-02-08 16:03   ` Jonathan Cameron
2020-02-11 15:22     ` Olivier MOYSAN
2020-02-04 10:10 ` [PATCH 3/4] iio: adc: stm32-dfsdm: use resolution define Olivier Moysan
2020-02-04 10:10 ` [PATCH 4/4] iio: adc: stm32-dfsdm: add scale and offset support Olivier Moysan
2020-02-08 16:18   ` Jonathan Cameron
2020-02-11 15:19     ` Olivier MOYSAN
2020-02-14 13:11       ` Jonathan Cameron
2020-02-14 14:49         ` Olivier MOYSAN
2020-02-14 15:10           ` Jonathan Cameron

Linux-Devicetree Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-devicetree/0 linux-devicetree/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-devicetree linux-devicetree/ https://lore.kernel.org/linux-devicetree \
		devicetree@vger.kernel.org
	public-inbox-index linux-devicetree

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-devicetree


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git