[PATCH v2 0/3] Allow to tune sampling time on STM32 ADC

[PATCH v2 0/3] Allow to tune sampling time on STM32 ADC

[Fabrice Gasnier]

STM32 ADC allows each channel to be sampled with a different sampling
time. This series fixes common clock rate and adds optional device
tree property, so minimum sampling time can be tuned. This is done via
device tree as it's tightly coupled with hardware (analog source).
---
Changes in v2:
- Better describe fix patch
- Use 'st,min-sample-time-nsecs' instead of 'min-sample-time'.
- Add a comment on of_property_read_u32_index() use.

Fabrice Gasnier (3):
  iio: adc: stm32: fix common clock rate
  dt-bindings: iio: adc: stm32: add optional st,min-sample-time-nsecs
  iio: adc: stm32: add optional st,min-sample-time-nsecs

 .../devicetree/bindings/iio/adc/st,stm32-adc.txt   |   5 +
 drivers/iio/adc/stm32-adc-core.c                   |  10 +-
 drivers/iio/adc/stm32-adc.c                        | 140 ++++++++++++++++++++-
 3 files changed, 146 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH v2 1/3] iio: adc: stm32: fix common clock rate

[Fabrice Gasnier]

Fixes: 95e339b6e85d ("iio: adc: stm32: add support for STM32H7")

ADC clock input is provided to internal prescaler (that decreases its
frequency). It's then used as reference clock for conversions.

- Fix common clock rate used then by stm32-adc sub-devices. Take common
  prescaler into account. Currently, rate is used to set "boost" mode.
  It may unnecessarily be set. This impacts power consumption.
- Fix ADC max clock rate on STM32H7 (fADC from datasheet). Currently,
  prescaler may be set too low. This can result in ADC reference
  clock used for conversion to exceed max allowed clock frequency.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
Changes in v2:
- Better description of wrong things being fixed.
---
 drivers/iio/adc/stm32-adc-core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
index e09233b..6096763 100644
--- a/drivers/iio/adc/stm32-adc-core.c
+++ b/drivers/iio/adc/stm32-adc-core.c
@@ -64,7 +64,7 @@
 #define STM32H7_CKMODE_MASK		GENMASK(17, 16)
 
 /* STM32 H7 maximum analog clock rate (from datasheet) */
-#define STM32H7_ADC_MAX_CLK_RATE	72000000
+#define STM32H7_ADC_MAX_CLK_RATE	36000000
 
 /**
  * stm32_adc_common_regs - stm32 common registers, compatible dependent data
@@ -148,14 +148,14 @@ static int stm32f4_adc_clk_sel(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
-	priv->common.rate = rate;
+	priv->common.rate = rate / stm32f4_pclk_div[i];
 	val = readl_relaxed(priv->common.base + STM32F4_ADC_CCR);
 	val &= ~STM32F4_ADC_ADCPRE_MASK;
 	val |= i << STM32F4_ADC_ADCPRE_SHIFT;
 	writel_relaxed(val, priv->common.base + STM32F4_ADC_CCR);
 
 	dev_dbg(&pdev->dev, "Using analog clock source at %ld kHz\n",
-		rate / (stm32f4_pclk_div[i] * 1000));
+		priv->common.rate / 1000);
 
 	return 0;
 }
@@ -250,7 +250,7 @@ static int stm32h7_adc_clk_sel(struct platform_device *pdev,
 
 out:
 	/* rate used later by each ADC instance to control BOOST mode */
-	priv->common.rate = rate;
+	priv->common.rate = rate / div;
 
 	/* Set common clock mode and prescaler */
 	val = readl_relaxed(priv->common.base + STM32H7_ADC_CCR);
@@ -260,7 +260,7 @@ static int stm32h7_adc_clk_sel(struct platform_device *pdev,
 	writel_relaxed(val, priv->common.base + STM32H7_ADC_CCR);
 
 	dev_dbg(&pdev->dev, "Using %s clock/%d source at %ld kHz\n",
-		ckmode ? "bus" : "adc", div, rate / (div * 1000));
+		ckmode ? "bus" : "adc", div, priv->common.rate / 1000);
 
 	return 0;
 }
-- 
1.9.1

[PATCH v2 2/3] dt-bindings: iio: adc: stm32: add optional st,min-sample-time-nsecs

[Fabrice Gasnier]

STM32 ADC allows each channel to be sampled with a different sampling
time. There's an application note that deals with this: 'How to get
the best ADC accuracy in STM32...' It basically depends on analog input
signal electrical properties (depends on board).

Add optional 'st,min-sample-time-nsecs' property so this can be tuned
in dt.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
Changes in v2:
- Use 'st,min-sample-time-nsecs' instead of 'min-sample-time'.
---
 Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
index 8310073..48bfcaa3 100644
--- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
@@ -74,6 +74,11 @@ Optional properties:
   * can be 6, 8, 10 or 12 on stm32f4
   * can be 8, 10, 12, 14 or 16 on stm32h7
   Default is maximum resolution if unset.
+- st,min-sample-time-nsecs: Minimum sampling time in nanoseconds.
+  Depending on hardware (board) e.g. high/low analog input source impedance,
+  fine tune of ADC sampling time may be recommended.
+  This can be either one value or an array that matches 'st,adc-channels' list,
+  to set sample time resp. for all channels, or independently for each channel.
 
 Example:
 	adc: adc@40012000 {
-- 
1.9.1

[PATCH v2 3/3] iio: adc: stm32: add optional st,min-sample-time-nsecs

[Fabrice Gasnier]

STM32 ADC allows each channel to be sampled with a different sampling time,
by setting SMPR registers. Basically, value depends on local electrical
properties. Selecting correct value for sampling time highly depends on
analog source impedance. There is a manual that may help in this process:
'How to get the best ADC accuracy in STM32...'

This patch allows to configure minimum sampling time via device tree,
either for:
- all channels at once:
  st,min-sample-time-nsecs = <10000>;

- independently for each channel (must match "st,adc-channels" list):
  st,adc-channels = <0 1>;
  st,min-sample-time-nsecs = <5000 10000>;

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
Changes in v2:
- Use 'st,min-sample-time-nsecs' instead of 'min-sample-time'.
- Add a comment on of_property_read_u32_index() use.
---
 drivers/iio/adc/stm32-adc.c | 140 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 136 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 5bfcc1f..6bc6028 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -83,6 +83,8 @@
 #define STM32H7_ADC_IER			0x04
 #define STM32H7_ADC_CR			0x08
 #define STM32H7_ADC_CFGR		0x0C
+#define STM32H7_ADC_SMPR1		0x14
+#define STM32H7_ADC_SMPR2		0x18
 #define STM32H7_ADC_PCSEL		0x1C
 #define STM32H7_ADC_SQR1		0x30
 #define STM32H7_ADC_SQR2		0x34
@@ -151,6 +153,7 @@ enum stm32h7_adc_dmngt {
 #define STM32H7_BOOST_CLKRATE		20000000UL
 
 #define STM32_ADC_MAX_SQ		16	/* SQ1..SQ16 */
+#define STM32_ADC_MAX_SMP		7	/* SMPx range is [0..7] */
 #define STM32_ADC_TIMEOUT_US		100000
 #define STM32_ADC_TIMEOUT	(msecs_to_jiffies(STM32_ADC_TIMEOUT_US / 1000))
 
@@ -227,6 +230,8 @@ struct stm32_adc_regs {
  * @exten:		trigger control register & bitfield
  * @extsel:		trigger selection register & bitfield
  * @res:		resolution selection register & bitfield
+ * @smpr:		smpr1 & smpr2 registers offset array
+ * @smp_bits:		smpr1 & smpr2 index and bitfields
  */
 struct stm32_adc_regspec {
 	const u32 dr;
@@ -236,6 +241,8 @@ struct stm32_adc_regspec {
 	const struct stm32_adc_regs exten;
 	const struct stm32_adc_regs extsel;
 	const struct stm32_adc_regs res;
+	const u32 smpr[2];
+	const struct stm32_adc_regs *smp_bits;
 };
 
 struct stm32_adc;
@@ -251,6 +258,7 @@ struct stm32_adc_regspec {
  * @start_conv:		routine to start conversions
  * @stop_conv:		routine to stop conversions
  * @unprepare:		optional unprepare routine (disable, power-down)
+ * @smp_cycles:		programmable sampling time (ADC clock cycles)
  */
 struct stm32_adc_cfg {
 	const struct stm32_adc_regspec	*regs;
@@ -262,6 +270,7 @@ struct stm32_adc_cfg {
 	void (*start_conv)(struct stm32_adc *, bool dma);
 	void (*stop_conv)(struct stm32_adc *);
 	void (*unprepare)(struct stm32_adc *);
+	const unsigned int *smp_cycles;
 };
 
 /**
@@ -283,6 +292,7 @@ struct stm32_adc_cfg {
  * @rx_dma_buf:		dma rx buffer bus address
  * @rx_buf_sz:		dma rx buffer size
  * @pcsel		bitmask to preselect channels on some devices
+ * @smpr_val:		sampling time settings (e.g. smpr1 / smpr2)
  * @cal:		optional calibration data on some devices
  */
 struct stm32_adc {
@@ -303,6 +313,7 @@ struct stm32_adc {
 	dma_addr_t		rx_dma_buf;
 	unsigned int		rx_buf_sz;
 	u32			pcsel;
+	u32			smpr_val[2];
 	struct stm32_adc_calib	cal;
 };
 
@@ -431,6 +442,39 @@ struct stm32_adc_info {
 	{}, /* sentinel */
 };
 
+/**
+ * stm32f4_smp_bits[] - describe sampling time register index & bit fields
+ * Sorted so it can be indexed by channel number.
+ */
+static const struct stm32_adc_regs stm32f4_smp_bits[] = {
+	/* STM32F4_ADC_SMPR2: smpr[] index, mask, shift for SMP0 to SMP9 */
+	{ 1, GENMASK(2, 0), 0 },
+	{ 1, GENMASK(5, 3), 3 },
+	{ 1, GENMASK(8, 6), 6 },
+	{ 1, GENMASK(11, 9), 9 },
+	{ 1, GENMASK(14, 12), 12 },
+	{ 1, GENMASK(17, 15), 15 },
+	{ 1, GENMASK(20, 18), 18 },
+	{ 1, GENMASK(23, 21), 21 },
+	{ 1, GENMASK(26, 24), 24 },
+	{ 1, GENMASK(29, 27), 27 },
+	/* STM32F4_ADC_SMPR1, smpr[] index, mask, shift for SMP10 to SMP18 */
+	{ 0, GENMASK(2, 0), 0 },
+	{ 0, GENMASK(5, 3), 3 },
+	{ 0, GENMASK(8, 6), 6 },
+	{ 0, GENMASK(11, 9), 9 },
+	{ 0, GENMASK(14, 12), 12 },
+	{ 0, GENMASK(17, 15), 15 },
+	{ 0, GENMASK(20, 18), 18 },
+	{ 0, GENMASK(23, 21), 21 },
+	{ 0, GENMASK(26, 24), 24 },
+};
+
+/* STM32F4 programmable sampling time (ADC clock cycles) */
+static const unsigned int stm32f4_adc_smp_cycles[STM32_ADC_MAX_SMP + 1] = {
+	3, 15, 28, 56, 84, 112, 144, 480,
+};
+
 static const struct stm32_adc_regspec stm32f4_adc_regspec = {
 	.dr = STM32F4_ADC_DR,
 	.ier_eoc = { STM32F4_ADC_CR1, STM32F4_EOCIE },
@@ -440,6 +484,8 @@ struct stm32_adc_info {
 	.extsel = { STM32F4_ADC_CR2, STM32F4_EXTSEL_MASK,
 		    STM32F4_EXTSEL_SHIFT },
 	.res = { STM32F4_ADC_CR1, STM32F4_RES_MASK, STM32F4_RES_SHIFT },
+	.smpr = { STM32F4_ADC_SMPR1, STM32F4_ADC_SMPR2 },
+	.smp_bits = stm32f4_smp_bits,
 };
 
 static const struct stm32_adc_regs stm32h7_sq[STM32_ADC_MAX_SQ + 1] = {
@@ -483,6 +529,40 @@ struct stm32_adc_info {
 	{},
 };
 
+/**
+ * stm32h7_smp_bits - describe sampling time register index & bit fields
+ * Sorted so it can be indexed by channel number.
+ */
+static const struct stm32_adc_regs stm32h7_smp_bits[] = {
+	/* STM32H7_ADC_SMPR1, smpr[] index, mask, shift for SMP0 to SMP9 */
+	{ 0, GENMASK(2, 0), 0 },
+	{ 0, GENMASK(5, 3), 3 },
+	{ 0, GENMASK(8, 6), 6 },
+	{ 0, GENMASK(11, 9), 9 },
+	{ 0, GENMASK(14, 12), 12 },
+	{ 0, GENMASK(17, 15), 15 },
+	{ 0, GENMASK(20, 18), 18 },
+	{ 0, GENMASK(23, 21), 21 },
+	{ 0, GENMASK(26, 24), 24 },
+	{ 0, GENMASK(29, 27), 27 },
+	/* STM32H7_ADC_SMPR2, smpr[] index, mask, shift for SMP10 to SMP19 */
+	{ 1, GENMASK(2, 0), 0 },
+	{ 1, GENMASK(5, 3), 3 },
+	{ 1, GENMASK(8, 6), 6 },
+	{ 1, GENMASK(11, 9), 9 },
+	{ 1, GENMASK(14, 12), 12 },
+	{ 1, GENMASK(17, 15), 15 },
+	{ 1, GENMASK(20, 18), 18 },
+	{ 1, GENMASK(23, 21), 21 },
+	{ 1, GENMASK(26, 24), 24 },
+	{ 1, GENMASK(29, 27), 27 },
+};
+
+/* STM32H7 programmable sampling time (ADC clock cycles, rounded down) */
+static const unsigned int stm32h7_adc_smp_cycles[STM32_ADC_MAX_SMP + 1] = {
+	1, 2, 8, 16, 32, 64, 387, 810,
+};
+
 static const struct stm32_adc_regspec stm32h7_adc_regspec = {
 	.dr = STM32H7_ADC_DR,
 	.ier_eoc = { STM32H7_ADC_IER, STM32H7_EOCIE },
@@ -492,6 +572,8 @@ struct stm32_adc_info {
 	.extsel = { STM32H7_ADC_CFGR, STM32H7_EXTSEL_MASK,
 		    STM32H7_EXTSEL_SHIFT },
 	.res = { STM32H7_ADC_CFGR, STM32H7_RES_MASK, STM32H7_RES_SHIFT },
+	.smpr = { STM32H7_ADC_SMPR1, STM32H7_ADC_SMPR2 },
+	.smp_bits = stm32h7_smp_bits,
 };
 
 /**
@@ -933,6 +1015,7 @@ static void stm32h7_adc_unprepare(struct stm32_adc *adc)
  * @scan_mask: channels to be converted
  *
  * Conversion sequence :
+ * Apply sampling time settings for all channels.
  * Configure ADC scan sequence based on selected channels in scan_mask.
  * Add channels to SQR registers, from scan_mask LSB to MSB, then
  * program sequence len.
@@ -946,6 +1029,10 @@ static int stm32_adc_conf_scan_seq(struct iio_dev *indio_dev,
 	u32 val, bit;
 	int i = 0;
 
+	/* Apply sampling time settings */
+	stm32_adc_writel(adc, adc->cfg->regs->smpr[0], adc->smpr_val[0]);
+	stm32_adc_writel(adc, adc->cfg->regs->smpr[1], adc->smpr_val[1]);
+
 	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
 		chan = indio_dev->channels + bit;
 		/*
@@ -1079,6 +1166,7 @@ static int stm32_adc_get_trig_pol(struct iio_dev *indio_dev,
  * @res: conversion result
  *
  * The function performs a single conversion on a given channel:
+ * - Apply sampling time settings
  * - Program sequencer with one channel (e.g. in SQ1 with len = 1)
  * - Use SW trigger
  * - Start conversion, then wait for interrupt completion.
@@ -1103,6 +1191,10 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
 			return ret;
 	}
 
+	/* Apply sampling time settings */
+	stm32_adc_writel(adc, regs->smpr[0], adc->smpr_val[0]);
+	stm32_adc_writel(adc, regs->smpr[1], adc->smpr_val[1]);
+
 	/* Program chan number in regular sequence (SQ1) */
 	val = stm32_adc_readl(adc, regs->sqr[1].reg);
 	val &= ~regs->sqr[1].mask;
@@ -1507,10 +1599,28 @@ static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev)
 	return 0;
 }
 
+static void stm32_adc_smpr_init(struct stm32_adc *adc, int channel, u32 smp_ns)
+{
+	const struct stm32_adc_regs *smpr = &adc->cfg->regs->smp_bits[channel];
+	u32 period_ns, shift = smpr->shift, mask = smpr->mask;
+	unsigned int smp, r = smpr->reg;
+
+	/* Determine sampling time (ADC clock cycles) */
+	period_ns = NSEC_PER_SEC / adc->common->rate;
+	for (smp = 0; smp <= STM32_ADC_MAX_SMP; smp++)
+		if ((period_ns * adc->cfg->smp_cycles[smp]) >= smp_ns)
+			break;
+	if (smp > STM32_ADC_MAX_SMP)
+		smp = STM32_ADC_MAX_SMP;
+
+	/* pre-build sampling time registers (e.g. smpr1, smpr2) */
+	adc->smpr_val[r] = (adc->smpr_val[r] & ~mask) | (smp << shift);
+}
+
 static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
 				    struct iio_chan_spec *chan,
 				    const struct stm32_adc_chan_spec *channel,
-				    int scan_index)
+				    int scan_index, u32 smp)
 {
 	struct stm32_adc *adc = iio_priv(indio_dev);
 
@@ -1526,6 +1636,9 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
 	chan->scan_type.storagebits = 16;
 	chan->ext_info = stm32_adc_ext_info;
 
+	/* Prepare sampling time settings */
+	stm32_adc_smpr_init(adc, chan->channel, smp);
+
 	/* pre-build selected channels mask */
 	adc->pcsel |= BIT(chan->channel);
 }
@@ -1538,8 +1651,8 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
 	struct property *prop;
 	const __be32 *cur;
 	struct iio_chan_spec *channels;
-	int scan_index = 0, num_channels;
-	u32 val;
+	int scan_index = 0, num_channels, ret;
+	u32 val, smp = 0;
 
 	num_channels = of_property_count_u32_elems(node, "st,adc-channels");
 	if (num_channels < 0 ||
@@ -1548,6 +1661,13 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
 		return num_channels < 0 ? num_channels : -EINVAL;
 	}
 
+	/* Optional sample time is provided either for each, or all channels */
+	ret = of_property_count_u32_elems(node, "st,min-sample-time-nsecs");
+	if (ret > 1 && ret != num_channels) {
+		dev_err(&indio_dev->dev, "Invalid st,min-sample-time-nsecs\n");
+		return -EINVAL;
+	}
+
 	channels = devm_kcalloc(&indio_dev->dev, num_channels,
 				sizeof(struct iio_chan_spec), GFP_KERNEL);
 	if (!channels)
@@ -1558,9 +1678,19 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
 			dev_err(&indio_dev->dev, "Invalid channel %d\n", val);
 			return -EINVAL;
 		}
+
+		/*
+		 * Using of_property_read_u32_index(), smp value will only be
+		 * modified if valid u32 value can be decoded. This allows to
+		 * get either no value, 1 shared value for all indexes, or one
+		 * value per channel.
+		 */
+		of_property_read_u32_index(node, "st,min-sample-time-nsecs",
+					   scan_index, &smp);
+
 		stm32_adc_chan_init_one(indio_dev, &channels[scan_index],
 					&adc_info->channels[val],
-					scan_index);
+					scan_index, smp);
 		scan_index++;
 	}
 
@@ -1755,6 +1885,7 @@ static int stm32_adc_remove(struct platform_device *pdev)
 	.clk_required = true,
 	.start_conv = stm32f4_adc_start_conv,
 	.stop_conv = stm32f4_adc_stop_conv,
+	.smp_cycles = stm32f4_adc_smp_cycles,
 };
 
 static const struct stm32_adc_cfg stm32h7_adc_cfg = {
@@ -1766,6 +1897,7 @@ static int stm32_adc_remove(struct platform_device *pdev)
 	.stop_conv = stm32h7_adc_stop_conv,
 	.prepare = stm32h7_adc_prepare,
 	.unprepare = stm32h7_adc_unprepare,
+	.smp_cycles = stm32h7_adc_smp_cycles,
 };
 
 static const struct of_device_id stm32_adc_of_match[] = {
-- 
1.9.1

Re: [PATCH v2 1/3] iio: adc: stm32: fix common clock rate

[Jonathan Cameron]

On Mon, 24 Jul 2017 18:10:38 +0200
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:

> Fixes: 95e339b6e85d ("iio: adc: stm32: add support for STM32H7")
> 
> ADC clock input is provided to internal prescaler (that decreases its
> frequency). It's then used as reference clock for conversions.
> 
> - Fix common clock rate used then by stm32-adc sub-devices. Take common
>   prescaler into account. Currently, rate is used to set "boost" mode.
>   It may unnecessarily be set. This impacts power consumption.
> - Fix ADC max clock rate on STM32H7 (fADC from datasheet). Currently,
>   prescaler may be set too low. This can result in ADC reference
>   clock used for conversion to exceed max allowed clock frequency.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
I've applied this one to the fixes togreg branch of iio.git.

Thanks,

Jonathan
> ---
> Changes in v2:
> - Better description of wrong things being fixed.
> ---
>  drivers/iio/adc/stm32-adc-core.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
> index e09233b..6096763 100644
> --- a/drivers/iio/adc/stm32-adc-core.c
> +++ b/drivers/iio/adc/stm32-adc-core.c
> @@ -64,7 +64,7 @@
>  #define STM32H7_CKMODE_MASK		GENMASK(17, 16)
>  
>  /* STM32 H7 maximum analog clock rate (from datasheet) */
> -#define STM32H7_ADC_MAX_CLK_RATE	72000000
> +#define STM32H7_ADC_MAX_CLK_RATE	36000000
>  
>  /**
>   * stm32_adc_common_regs - stm32 common registers, compatible dependent data
> @@ -148,14 +148,14 @@ static int stm32f4_adc_clk_sel(struct platform_device *pdev,
>  		return -EINVAL;
>  	}
>  
> -	priv->common.rate = rate;
> +	priv->common.rate = rate / stm32f4_pclk_div[i];
>  	val = readl_relaxed(priv->common.base + STM32F4_ADC_CCR);
>  	val &= ~STM32F4_ADC_ADCPRE_MASK;
>  	val |= i << STM32F4_ADC_ADCPRE_SHIFT;
>  	writel_relaxed(val, priv->common.base + STM32F4_ADC_CCR);
>  
>  	dev_dbg(&pdev->dev, "Using analog clock source at %ld kHz\n",
> -		rate / (stm32f4_pclk_div[i] * 1000));
> +		priv->common.rate / 1000);
>  
>  	return 0;
>  }
> @@ -250,7 +250,7 @@ static int stm32h7_adc_clk_sel(struct platform_device *pdev,
>  
>  out:
>  	/* rate used later by each ADC instance to control BOOST mode */
> -	priv->common.rate = rate;
> +	priv->common.rate = rate / div;
>  
>  	/* Set common clock mode and prescaler */
>  	val = readl_relaxed(priv->common.base + STM32H7_ADC_CCR);
> @@ -260,7 +260,7 @@ static int stm32h7_adc_clk_sel(struct platform_device *pdev,
>  	writel_relaxed(val, priv->common.base + STM32H7_ADC_CCR);
>  
>  	dev_dbg(&pdev->dev, "Using %s clock/%d source at %ld kHz\n",
> -		ckmode ? "bus" : "adc", div, rate / (div * 1000));
> +		ckmode ? "bus" : "adc", div, priv->common.rate / 1000);
>  
>  	return 0;
>  }

Re: [PATCH v2 2/3] dt-bindings: iio: adc: stm32: add optional st,min-sample-time-nsecs

[Jonathan Cameron]

On Mon, 24 Jul 2017 18:10:39 +0200
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:

> STM32 ADC allows each channel to be sampled with a different sampling
> time. There's an application note that deals with this: 'How to get
> the best ADC accuracy in STM32...' It basically depends on analog input
> signal electrical properties (depends on board).
> 
> Add optional 'st,min-sample-time-nsecs' property so this can be tuned
> in dt.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
This ideally wants a devicetree review so I'm going to sit on
it for a few days longer.

Looks fine to me as does the implementation patch.

Give me a kick in a week or so if we have seen no progress on
this.  I will be travelling from next weekend and access may
be a little intermittent for up to two weeks.

Thanks,

Jonathan
> ---
> Changes in v2:
> - Use 'st,min-sample-time-nsecs' instead of 'min-sample-time'.
> ---
>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
> index 8310073..48bfcaa3 100644
> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
> @@ -74,6 +74,11 @@ Optional properties:
>    * can be 6, 8, 10 or 12 on stm32f4
>    * can be 8, 10, 12, 14 or 16 on stm32h7
>    Default is maximum resolution if unset.
> +- st,min-sample-time-nsecs: Minimum sampling time in nanoseconds.
> +  Depending on hardware (board) e.g. high/low analog input source impedance,
> +  fine tune of ADC sampling time may be recommended.
> +  This can be either one value or an array that matches 'st,adc-channels' list,
> +  to set sample time resp. for all channels, or independently for each channel.
>  
>  Example:
>  	adc: adc@40012000 {

Re: [PATCH v2 2/3] dt-bindings: iio: adc: stm32: add optional st,min-sample-time-nsecs

[Rob Herring]

On Mon, Jul 24, 2017 at 06:10:39PM +0200, Fabrice Gasnier wrote:
> STM32 ADC allows each channel to be sampled with a different sampling
> time. There's an application note that deals with this: 'How to get
> the best ADC accuracy in STM32...' It basically depends on analog input
> signal electrical properties (depends on board).
> 
> Add optional 'st,min-sample-time-nsecs' property so this can be tuned
> in dt.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
> Changes in v2:
> - Use 'st,min-sample-time-nsecs' instead of 'min-sample-time'.
> ---
>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 5 +++++
>  1 file changed, 5 insertions(+)

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

Re: [PATCH v2 2/3] dt-bindings: iio: adc: stm32: add optional st,min-sample-time-nsecs

[Jonathan Cameron]

On Thu, 3 Aug 2017 12:01:54 -0500
Rob Herring <robh@kernel.org> wrote:

> On Mon, Jul 24, 2017 at 06:10:39PM +0200, Fabrice Gasnier wrote:
> > STM32 ADC allows each channel to be sampled with a different sampling
> > time. There's an application note that deals with this: 'How to get
> > the best ADC accuracy in STM32...' It basically depends on analog input
> > signal electrical properties (depends on board).
> > 
> > Add optional 'st,min-sample-time-nsecs' property so this can be tuned
> > in dt.
> > 
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > ---
> > Changes in v2:
> > - Use 'st,min-sample-time-nsecs' instead of 'min-sample-time'.
> > ---
> >  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 5 +++++
> >  1 file changed, 5 insertions(+)  
> 
> Acked-by: Rob Herring <robh@kernel.org>
Applied to the togreg branch of iio.git and pushed out as testings.

Thanks for review Rob and thanks for the patch Fabrice.

Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] iio: adc: stm32: add optional st,min-sample-time-nsecs

[Jonathan Cameron]

On Mon, 24 Jul 2017 18:10:40 +0200
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:

> STM32 ADC allows each channel to be sampled with a different sampling time,
> by setting SMPR registers. Basically, value depends on local electrical
> properties. Selecting correct value for sampling time highly depends on
> analog source impedance. There is a manual that may help in this process:
> 'How to get the best ADC accuracy in STM32...'
> 
> This patch allows to configure minimum sampling time via device tree,
> either for:
> - all channels at once:
>   st,min-sample-time-nsecs = <10000>;
> 
> - independently for each channel (must match "st,adc-channels" list):
>   st,adc-channels = <0 1>;
>   st,min-sample-time-nsecs = <5000 10000>;
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan
> ---
> Changes in v2:
> - Use 'st,min-sample-time-nsecs' instead of 'min-sample-time'.
> - Add a comment on of_property_read_u32_index() use.
> ---
>  drivers/iio/adc/stm32-adc.c | 140 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 136 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 5bfcc1f..6bc6028 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -83,6 +83,8 @@
>  #define STM32H7_ADC_IER			0x04
>  #define STM32H7_ADC_CR			0x08
>  #define STM32H7_ADC_CFGR		0x0C
> +#define STM32H7_ADC_SMPR1		0x14
> +#define STM32H7_ADC_SMPR2		0x18
>  #define STM32H7_ADC_PCSEL		0x1C
>  #define STM32H7_ADC_SQR1		0x30
>  #define STM32H7_ADC_SQR2		0x34
> @@ -151,6 +153,7 @@ enum stm32h7_adc_dmngt {
>  #define STM32H7_BOOST_CLKRATE		20000000UL
>  
>  #define STM32_ADC_MAX_SQ		16	/* SQ1..SQ16 */
> +#define STM32_ADC_MAX_SMP		7	/* SMPx range is [0..7] */
>  #define STM32_ADC_TIMEOUT_US		100000
>  #define STM32_ADC_TIMEOUT	(msecs_to_jiffies(STM32_ADC_TIMEOUT_US / 1000))
>  
> @@ -227,6 +230,8 @@ struct stm32_adc_regs {
>   * @exten:		trigger control register & bitfield
>   * @extsel:		trigger selection register & bitfield
>   * @res:		resolution selection register & bitfield
> + * @smpr:		smpr1 & smpr2 registers offset array
> + * @smp_bits:		smpr1 & smpr2 index and bitfields
>   */
>  struct stm32_adc_regspec {
>  	const u32 dr;
> @@ -236,6 +241,8 @@ struct stm32_adc_regspec {
>  	const struct stm32_adc_regs exten;
>  	const struct stm32_adc_regs extsel;
>  	const struct stm32_adc_regs res;
> +	const u32 smpr[2];
> +	const struct stm32_adc_regs *smp_bits;
>  };
>  
>  struct stm32_adc;
> @@ -251,6 +258,7 @@ struct stm32_adc_regspec {
>   * @start_conv:		routine to start conversions
>   * @stop_conv:		routine to stop conversions
>   * @unprepare:		optional unprepare routine (disable, power-down)
> + * @smp_cycles:		programmable sampling time (ADC clock cycles)
>   */
>  struct stm32_adc_cfg {
>  	const struct stm32_adc_regspec	*regs;
> @@ -262,6 +270,7 @@ struct stm32_adc_cfg {
>  	void (*start_conv)(struct stm32_adc *, bool dma);
>  	void (*stop_conv)(struct stm32_adc *);
>  	void (*unprepare)(struct stm32_adc *);
> +	const unsigned int *smp_cycles;
>  };
>  
>  /**
> @@ -283,6 +292,7 @@ struct stm32_adc_cfg {
>   * @rx_dma_buf:		dma rx buffer bus address
>   * @rx_buf_sz:		dma rx buffer size
>   * @pcsel		bitmask to preselect channels on some devices
> + * @smpr_val:		sampling time settings (e.g. smpr1 / smpr2)
>   * @cal:		optional calibration data on some devices
>   */
>  struct stm32_adc {
> @@ -303,6 +313,7 @@ struct stm32_adc {
>  	dma_addr_t		rx_dma_buf;
>  	unsigned int		rx_buf_sz;
>  	u32			pcsel;
> +	u32			smpr_val[2];
>  	struct stm32_adc_calib	cal;
>  };
>  
> @@ -431,6 +442,39 @@ struct stm32_adc_info {
>  	{}, /* sentinel */
>  };
>  
> +/**
> + * stm32f4_smp_bits[] - describe sampling time register index & bit fields
> + * Sorted so it can be indexed by channel number.
> + */
> +static const struct stm32_adc_regs stm32f4_smp_bits[] = {
> +	/* STM32F4_ADC_SMPR2: smpr[] index, mask, shift for SMP0 to SMP9 */
> +	{ 1, GENMASK(2, 0), 0 },
> +	{ 1, GENMASK(5, 3), 3 },
> +	{ 1, GENMASK(8, 6), 6 },
> +	{ 1, GENMASK(11, 9), 9 },
> +	{ 1, GENMASK(14, 12), 12 },
> +	{ 1, GENMASK(17, 15), 15 },
> +	{ 1, GENMASK(20, 18), 18 },
> +	{ 1, GENMASK(23, 21), 21 },
> +	{ 1, GENMASK(26, 24), 24 },
> +	{ 1, GENMASK(29, 27), 27 },
> +	/* STM32F4_ADC_SMPR1, smpr[] index, mask, shift for SMP10 to SMP18 */
> +	{ 0, GENMASK(2, 0), 0 },
> +	{ 0, GENMASK(5, 3), 3 },
> +	{ 0, GENMASK(8, 6), 6 },
> +	{ 0, GENMASK(11, 9), 9 },
> +	{ 0, GENMASK(14, 12), 12 },
> +	{ 0, GENMASK(17, 15), 15 },
> +	{ 0, GENMASK(20, 18), 18 },
> +	{ 0, GENMASK(23, 21), 21 },
> +	{ 0, GENMASK(26, 24), 24 },
> +};
> +
> +/* STM32F4 programmable sampling time (ADC clock cycles) */
> +static const unsigned int stm32f4_adc_smp_cycles[STM32_ADC_MAX_SMP + 1] = {
> +	3, 15, 28, 56, 84, 112, 144, 480,
> +};
> +
>  static const struct stm32_adc_regspec stm32f4_adc_regspec = {
>  	.dr = STM32F4_ADC_DR,
>  	.ier_eoc = { STM32F4_ADC_CR1, STM32F4_EOCIE },
> @@ -440,6 +484,8 @@ struct stm32_adc_info {
>  	.extsel = { STM32F4_ADC_CR2, STM32F4_EXTSEL_MASK,
>  		    STM32F4_EXTSEL_SHIFT },
>  	.res = { STM32F4_ADC_CR1, STM32F4_RES_MASK, STM32F4_RES_SHIFT },
> +	.smpr = { STM32F4_ADC_SMPR1, STM32F4_ADC_SMPR2 },
> +	.smp_bits = stm32f4_smp_bits,
>  };
>  
>  static const struct stm32_adc_regs stm32h7_sq[STM32_ADC_MAX_SQ + 1] = {
> @@ -483,6 +529,40 @@ struct stm32_adc_info {
>  	{},
>  };
>  
> +/**
> + * stm32h7_smp_bits - describe sampling time register index & bit fields
> + * Sorted so it can be indexed by channel number.
> + */
> +static const struct stm32_adc_regs stm32h7_smp_bits[] = {
> +	/* STM32H7_ADC_SMPR1, smpr[] index, mask, shift for SMP0 to SMP9 */
> +	{ 0, GENMASK(2, 0), 0 },
> +	{ 0, GENMASK(5, 3), 3 },
> +	{ 0, GENMASK(8, 6), 6 },
> +	{ 0, GENMASK(11, 9), 9 },
> +	{ 0, GENMASK(14, 12), 12 },
> +	{ 0, GENMASK(17, 15), 15 },
> +	{ 0, GENMASK(20, 18), 18 },
> +	{ 0, GENMASK(23, 21), 21 },
> +	{ 0, GENMASK(26, 24), 24 },
> +	{ 0, GENMASK(29, 27), 27 },
> +	/* STM32H7_ADC_SMPR2, smpr[] index, mask, shift for SMP10 to SMP19 */
> +	{ 1, GENMASK(2, 0), 0 },
> +	{ 1, GENMASK(5, 3), 3 },
> +	{ 1, GENMASK(8, 6), 6 },
> +	{ 1, GENMASK(11, 9), 9 },
> +	{ 1, GENMASK(14, 12), 12 },
> +	{ 1, GENMASK(17, 15), 15 },
> +	{ 1, GENMASK(20, 18), 18 },
> +	{ 1, GENMASK(23, 21), 21 },
> +	{ 1, GENMASK(26, 24), 24 },
> +	{ 1, GENMASK(29, 27), 27 },
> +};
> +
> +/* STM32H7 programmable sampling time (ADC clock cycles, rounded down) */
> +static const unsigned int stm32h7_adc_smp_cycles[STM32_ADC_MAX_SMP + 1] = {
> +	1, 2, 8, 16, 32, 64, 387, 810,
> +};
> +
>  static const struct stm32_adc_regspec stm32h7_adc_regspec = {
>  	.dr = STM32H7_ADC_DR,
>  	.ier_eoc = { STM32H7_ADC_IER, STM32H7_EOCIE },
> @@ -492,6 +572,8 @@ struct stm32_adc_info {
>  	.extsel = { STM32H7_ADC_CFGR, STM32H7_EXTSEL_MASK,
>  		    STM32H7_EXTSEL_SHIFT },
>  	.res = { STM32H7_ADC_CFGR, STM32H7_RES_MASK, STM32H7_RES_SHIFT },
> +	.smpr = { STM32H7_ADC_SMPR1, STM32H7_ADC_SMPR2 },
> +	.smp_bits = stm32h7_smp_bits,
>  };
>  
>  /**
> @@ -933,6 +1015,7 @@ static void stm32h7_adc_unprepare(struct stm32_adc *adc)
>   * @scan_mask: channels to be converted
>   *
>   * Conversion sequence :
> + * Apply sampling time settings for all channels.
>   * Configure ADC scan sequence based on selected channels in scan_mask.
>   * Add channels to SQR registers, from scan_mask LSB to MSB, then
>   * program sequence len.
> @@ -946,6 +1029,10 @@ static int stm32_adc_conf_scan_seq(struct iio_dev *indio_dev,
>  	u32 val, bit;
>  	int i = 0;
>  
> +	/* Apply sampling time settings */
> +	stm32_adc_writel(adc, adc->cfg->regs->smpr[0], adc->smpr_val[0]);
> +	stm32_adc_writel(adc, adc->cfg->regs->smpr[1], adc->smpr_val[1]);
> +
>  	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
>  		chan = indio_dev->channels + bit;
>  		/*
> @@ -1079,6 +1166,7 @@ static int stm32_adc_get_trig_pol(struct iio_dev *indio_dev,
>   * @res: conversion result
>   *
>   * The function performs a single conversion on a given channel:
> + * - Apply sampling time settings
>   * - Program sequencer with one channel (e.g. in SQ1 with len = 1)
>   * - Use SW trigger
>   * - Start conversion, then wait for interrupt completion.
> @@ -1103,6 +1191,10 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
>  			return ret;
>  	}
>  
> +	/* Apply sampling time settings */
> +	stm32_adc_writel(adc, regs->smpr[0], adc->smpr_val[0]);
> +	stm32_adc_writel(adc, regs->smpr[1], adc->smpr_val[1]);
> +
>  	/* Program chan number in regular sequence (SQ1) */
>  	val = stm32_adc_readl(adc, regs->sqr[1].reg);
>  	val &= ~regs->sqr[1].mask;
> @@ -1507,10 +1599,28 @@ static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev)
>  	return 0;
>  }
>  
> +static void stm32_adc_smpr_init(struct stm32_adc *adc, int channel, u32 smp_ns)
> +{
> +	const struct stm32_adc_regs *smpr = &adc->cfg->regs->smp_bits[channel];
> +	u32 period_ns, shift = smpr->shift, mask = smpr->mask;
> +	unsigned int smp, r = smpr->reg;
> +
> +	/* Determine sampling time (ADC clock cycles) */
> +	period_ns = NSEC_PER_SEC / adc->common->rate;
> +	for (smp = 0; smp <= STM32_ADC_MAX_SMP; smp++)
> +		if ((period_ns * adc->cfg->smp_cycles[smp]) >= smp_ns)
> +			break;
> +	if (smp > STM32_ADC_MAX_SMP)
> +		smp = STM32_ADC_MAX_SMP;
> +
> +	/* pre-build sampling time registers (e.g. smpr1, smpr2) */
> +	adc->smpr_val[r] = (adc->smpr_val[r] & ~mask) | (smp << shift);
> +}
> +
>  static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
>  				    struct iio_chan_spec *chan,
>  				    const struct stm32_adc_chan_spec *channel,
> -				    int scan_index)
> +				    int scan_index, u32 smp)
>  {
>  	struct stm32_adc *adc = iio_priv(indio_dev);
>  
> @@ -1526,6 +1636,9 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
>  	chan->scan_type.storagebits = 16;
>  	chan->ext_info = stm32_adc_ext_info;
>  
> +	/* Prepare sampling time settings */
> +	stm32_adc_smpr_init(adc, chan->channel, smp);
> +
>  	/* pre-build selected channels mask */
>  	adc->pcsel |= BIT(chan->channel);
>  }
> @@ -1538,8 +1651,8 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
>  	struct property *prop;
>  	const __be32 *cur;
>  	struct iio_chan_spec *channels;
> -	int scan_index = 0, num_channels;
> -	u32 val;
> +	int scan_index = 0, num_channels, ret;
> +	u32 val, smp = 0;
>  
>  	num_channels = of_property_count_u32_elems(node, "st,adc-channels");
>  	if (num_channels < 0 ||
> @@ -1548,6 +1661,13 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
>  		return num_channels < 0 ? num_channels : -EINVAL;
>  	}
>  
> +	/* Optional sample time is provided either for each, or all channels */
> +	ret = of_property_count_u32_elems(node, "st,min-sample-time-nsecs");
> +	if (ret > 1 && ret != num_channels) {
> +		dev_err(&indio_dev->dev, "Invalid st,min-sample-time-nsecs\n");
> +		return -EINVAL;
> +	}
> +
>  	channels = devm_kcalloc(&indio_dev->dev, num_channels,
>  				sizeof(struct iio_chan_spec), GFP_KERNEL);
>  	if (!channels)
> @@ -1558,9 +1678,19 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
>  			dev_err(&indio_dev->dev, "Invalid channel %d\n", val);
>  			return -EINVAL;
>  		}
> +
> +		/*
> +		 * Using of_property_read_u32_index(), smp value will only be
> +		 * modified if valid u32 value can be decoded. This allows to
> +		 * get either no value, 1 shared value for all indexes, or one
> +		 * value per channel.
> +		 */
> +		of_property_read_u32_index(node, "st,min-sample-time-nsecs",
> +					   scan_index, &smp);
> +
>  		stm32_adc_chan_init_one(indio_dev, &channels[scan_index],
>  					&adc_info->channels[val],
> -					scan_index);
> +					scan_index, smp);
>  		scan_index++;
>  	}
>  
> @@ -1755,6 +1885,7 @@ static int stm32_adc_remove(struct platform_device *pdev)
>  	.clk_required = true,
>  	.start_conv = stm32f4_adc_start_conv,
>  	.stop_conv = stm32f4_adc_stop_conv,
> +	.smp_cycles = stm32f4_adc_smp_cycles,
>  };
>  
>  static const struct stm32_adc_cfg stm32h7_adc_cfg = {
> @@ -1766,6 +1897,7 @@ static int stm32_adc_remove(struct platform_device *pdev)
>  	.stop_conv = stm32h7_adc_stop_conv,
>  	.prepare = stm32h7_adc_prepare,
>  	.unprepare = stm32h7_adc_unprepare,
> +	.smp_cycles = stm32h7_adc_smp_cycles,
>  };
>  
>  static const struct of_device_id stm32_adc_of_match[] = {