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

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 min-sample-time via device tree, either for:
- all channels at once:
min-sample-time = <10000>; /* nanosecs */

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

Signed-off-by: Fabrice Gasnier <***@st.com>
---
drivers/iio/adc/stm32-adc.c | 134 ++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 130 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 5bfcc1f..e6f6b5e 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, "min-sample-time");
+ if (ret > 1 && ret != num_channels) {
+ dev_err(&indio_dev->dev, "Invalid min-sample-time\n");
+ return -EINVAL;
+ }
+
channels = devm_kcalloc(&indio_dev->dev, num_channels,
sizeof(struct iio_chan_spec), GFP_KERNEL);
if (!channels)
@@ -1558,9 +1678,13 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
dev_err(&indio_dev->dev, "Invalid channel %d\n", val);
return -EINVAL;
}
+
+ of_property_read_u32_index(node, "min-sample-time", 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 +1879,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 +1891,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

Jonathan Cameron

2017-07-23 11:00:22 UTC

On Tue, 18 Jul 2017 14:35:32 +0200

Post by 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
'How to get the best ADC accuracy in STM32...'
min-sample-time = <10000>; /* nanosecs */
st,adc-channels = <0 1>;
min-sample-time = <5000 10000>; /* nanosecs */

On question inline which may well just be down to me missing what
happens when you query index element 2 from a 1 element device tree
array.

Post by Fabrice Gasnier
---
drivers/iio/adc/stm32-adc.c | 134 ++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 130 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 5bfcc1f..e6f6b5e 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 {
*/
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 {
*/
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 {
*/
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)
*
+ * 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,
*
+ * - 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, "min-sample-time");
+ if (ret > 1 && ret != num_channels) {
+ dev_err(&indio_dev->dev, "Invalid min-sample-time\n");
+ return -EINVAL;
+ }
+
channels = devm_kcalloc(&indio_dev->dev, num_channels,
sizeof(struct iio_chan_spec), GFP_KERNEL);
if (!channels)
@@ -1558,9 +1678,13 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
dev_err(&indio_dev->dev, "Invalid channel %d\n", val);
return -EINVAL;
}
+
+ of_property_read_u32_index(node, "min-sample-time", scan_index,
+ &smp);
+

I might be missing something, but doesn't this fail for the single shared
value case?

Post by Fabrice Gasnier
stm32_adc_chan_init_one(indio_dev, &channels[scan_index],
&adc_info->channels[val],
- scan_index);
+ scan_index, smp);
scan_index++;
}
@@ -1755,6 +1879,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 +1891,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[] = {

Fabrice Gasnier

2017-07-24 08:16:00 UTC

Post by Jonathan Cameron
On Tue, 18 Jul 2017 14:35:32 +0200

On question inline which may well just be down to me missing what
happens when you query index element 2 from a 1 element device tree
array.

Hi Jonathan,

I should probably comment on it, please see inline.

I might be missing something, but doesn't this fail for the single shared
value case?

Yes, this fails in the single shared value case, with index >= 1.
Rewinding... when index is 0, it picks up 1st (shared) value.
Then fails for other index values.

of_property_read_u32_index() documentation states out value remains
untouched in case of error:
/**
* of_property_read_u32_index...
[...]
* The out_value is modified only if a valid u32 value can be decoded.

This is what I use here (as far as I tested it, it worked like a charm).
Do you wish I add a comment to describe this ?

Please let me know,
Best Regards,
Fabrice

Jonathan Cameron

2017-07-24 15:06:06 UTC

On Mon, 24 Jul 2017 10:16:00 +0200

Post by Jonathan Cameron
On Tue, 18 Jul 2017 14:35:32 +0200

On question inline which may well just be down to me missing what
happens when you query index element 2 from a 1 element device tree
array.

Hi Jonathan,
I should probably comment on it, please see inline.

<snip>

Post by Fabrice Gasnier
}
@@ -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, "min-sample-time");
+ if (ret > 1 && ret != num_channels) {
+ dev_err(&indio_dev->dev, "Invalid min-sample-time\n");
+ return -EINVAL;
+ }
+
channels = devm_kcalloc(&indio_dev->dev, num_channels,
sizeof(struct iio_chan_spec), GFP_KERNEL);
if (!channels)
@@ -1558,9 +1678,13 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
dev_err(&indio_dev->dev, "Invalid channel %d\n", val);
return -EINVAL;
}
+
+ of_property_read_u32_index(node, "min-sample-time", scan_index,
+ &smp);
+

I might be missing something, but doesn't this fail for the single shared
value case?

Yes, this fails in the single shared value case, with index >= 1.
Rewinding... when index is 0, it picks up 1st (shared) value.
Then fails for other index values.
of_property_read_u32_index() documentation states out value remains
/**
* of_property_read_u32_index...
[...]
* The out_value is modified only if a valid u32 value can be decoded.
This is what I use here (as far as I tested it, it worked like a charm).
Do you wish I add a comment to describe this ?

Probably best or I'll forget sometime down the line and think it looks
crazy again ;)

Thanks for the explanation.

Jonathan

Post by Fabrice Gasnier
Please let me know,
Best Regards,
Fabrice

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

Fabrice Gasnier

2017-07-18 12:35:31 UTC

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 'min-sample-time' property so this can be tuned in dt.

Signed-off-by: Fabrice Gasnier <***@st.com>
---
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..9cd964b 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.
+- min-sample-time: 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: ***@40012000 {

--
1.9.1

Jonathan Cameron

2017-07-23 10:53:37 UTC

On Tue, 18 Jul 2017 14:35:31 +0200

Post by 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 'min-sample-time' property so this can be tuned in dt.

This isn't yet very standard, so I think it needs a manufacturer
prefix, e.g. st,min-sample-time. Also convention is I believe
to include the units as part of the naming where appropriate.

Hence st,min-sample-time-nsecs

Otherwise, seems good to me.

Jonathan

Post by Fabrice Gasnier
---
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..9cd964b 100644
--- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
* 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.
+- min-sample-time: 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.

Fabrice Gasnier

2017-07-24 07:55:37 UTC

Post by Jonathan Cameron
On Tue, 18 Jul 2017 14:35:31 +0200

Hi Jonathan,

I originally used st,... form, when writing this patch, until I found
out there's 'min-sample-time' in vf610 adc dt-bindings:
- Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
It looks like to me very similar.

I also agree it misses '-nsecs' units. Sure, I can use your suggestion
above...

Please can you just confirm if I should use same property as vf610 adc,
or 'st,min-sample-time-nsecs' ?

Thanks for reviewing,
Best regards,
Fabrice

Post by Jonathan Cameron
Otherwise, seems good to me.
Jonathan

Jonathan Cameron

2017-07-24 15:04:18 UTC

On Mon, 24 Jul 2017 09:55:37 +0200

Post by Jonathan Cameron
On Tue, 18 Jul 2017 14:35:31 +0200

Hi Jonathan,
I originally used st,... form, when writing this patch, until I found
- Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
It looks like to me very similar.
I also agree it misses '-nsecs' units. Sure, I can use your suggestion
above...
Please can you just confirm if I should use same property as vf610 adc,
or 'st,min-sample-time-nsecs' ?

st,min-sample-time-nsecs.

We might well add a generic option in future, but the burden of
supporting this old one will be trivial anyway so not a problem.

Jonathan

Post by Fabrice Gasnier
Thanks for reviewing,
Best regards,
Fabrice

Post by Jonathan Cameron
Otherwise, seems good to me.
Jonathan

_______________________________________________
linux-arm-kernel mailing list
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Jonathan Cameron

2017-07-23 10:51:48 UTC

On Tue, 18 Jul 2017 14:35:30 +0200

Post by Fabrice Gasnier
Fixes commit 95e339b6e85d ("iio: adc: stm32: add support for STM32H7")
Fix common clock rate used then by stm32-adc sub-devices: take common
prescaler into account.
Fix ADC max clock rate on STM32H7 (fADC from datasheet)

Patch itself is fine, but the description could do with
information on what the result of this being wrong is.

I have no idea if this is a patch I should be sending upstream
asap or should hold for the next merge window.

Thanks,

Jonathan

Post by Fabrice Gasnier
---
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,
/* 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;
}

Fabrice Gasnier

2017-07-24 07:43:18 UTC

Post by Jonathan Cameron
On Tue, 18 Jul 2017 14:35:30 +0200

Patch itself is fine, but the description could do with
information on what the result of this being wrong is.

Hi Jonathan,

I agree with you and will improve description in v2. I'm thinking of:

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.

Post by Jonathan Cameron
I have no idea if this is a patch I should be sending upstream
asap or should hold for the next merge window.

Probably yes... I hope the description is better above? Just to mention
this impacts stm32h7 adc, device tree node to use it is not yet integrated.

Thanks for your review,
Best Regards,
Fabrice

Post by Jonathan Cameron
Thanks,
Jonathan

Jonathan Cameron

2017-07-24 15:03:01 UTC

On Mon, 24 Jul 2017 09:43:18 +0200