Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1690230 > unrolled thread
| Started by | Fabrice Gasnier <fabrice.gasnier@st.com> |
|---|---|
| First post | 2017-07-18 14:40 +0200 |
| Last post | 2017-07-24 17:10 +0200 |
| Articles | 9 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH 0/3] Allow to tune sampling time on STM32 ADC Fabrice Gasnier <fabrice.gasnier@st.com> - 2017-07-18 14:40 +0200
[PATCH 3/3] iio: adc: stm32: add optional min-sample-time Fabrice Gasnier <fabrice.gasnier@st.com> - 2017-07-18 14:40 +0200
Re: [PATCH 3/3] iio: adc: stm32: add optional min-sample-time Jonathan Cameron <jic23@kernel.org> - 2017-07-23 13:10 +0200
Re: [PATCH 3/3] iio: adc: stm32: add optional min-sample-time Fabrice Gasnier <fabrice.gasnier@st.com> - 2017-07-24 10:20 +0200
Re: [PATCH 3/3] iio: adc: stm32: add optional min-sample-time Jonathan Cameron <Jonathan.Cameron@huawei.com> - 2017-07-24 17:10 +0200
[PATCH 1/3] iio: adc: stm32: fix common clock rate Fabrice Gasnier <fabrice.gasnier@st.com> - 2017-07-18 14:40 +0200
Re: [PATCH 1/3] iio: adc: stm32: fix common clock rate Jonathan Cameron <jic23@kernel.org> - 2017-07-23 13:00 +0200
Re: [PATCH 1/3] iio: adc: stm32: fix common clock rate Fabrice Gasnier <fabrice.gasnier@st.com> - 2017-07-24 09:50 +0200
Re: [PATCH 1/3] iio: adc: stm32: fix common clock rate Jonathan Cameron <Jonathan.Cameron@huawei.com> - 2017-07-24 17:10 +0200
| From | Fabrice Gasnier <fabrice.gasnier@st.com> |
|---|---|
| Date | 2017-07-18 14:40 +0200 |
| Subject | [PATCH 0/3] Allow to tune sampling time on STM32 ADC |
| Message-ID | <u4zXc-2qo-7@gated-at.bofh.it> |
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). Fabrice Gasnier (3): iio: adc: stm32: fix common clock rate dt-bindings: iio: adc: stm32: add optional min-sample-time iio: adc: stm32: add optional min-sample-time .../devicetree/bindings/iio/adc/st,stm32-adc.txt | 5 + drivers/iio/adc/stm32-adc-core.c | 10 +- drivers/iio/adc/stm32-adc.c | 134 ++++++++++++++++++++- 3 files changed, 140 insertions(+), 9 deletions(-) -- 1.9.1
[toc] | [next] | [standalone]
| From | Fabrice Gasnier <fabrice.gasnier@st.com> |
|---|---|
| Date | 2017-07-18 14:40 +0200 |
| Subject | [PATCH 3/3] iio: adc: stm32: add optional min-sample-time |
| Message-ID | <u4zXc-2qo-15@gated-at.bofh.it> |
| In reply to | #1690230 |
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 <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
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Cameron <jic23@kernel.org> |
|---|---|
| Date | 2017-07-23 13:10 +0200 |
| Subject | Re: [PATCH 3/3] iio: adc: stm32: add optional min-sample-time |
| Message-ID | <u6mVQ-6Nw-13@gated-at.bofh.it> |
| In reply to | #1690232 |
On Tue, 18 Jul 2017 14:35:32 +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 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 <fabrice.gasnier@st.com>
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.
> ---
> 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);
> +
I might be missing something, but doesn't this fail for the single shared
value case?
> 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[] = {
[toc] | [prev] | [next] | [standalone]
| From | Fabrice Gasnier <fabrice.gasnier@st.com> |
|---|---|
| Date | 2017-07-24 10:20 +0200 |
| Subject | Re: [PATCH 3/3] iio: adc: stm32: add optional min-sample-time |
| Message-ID | <u6GKR-2eZ-11@gated-at.bofh.it> |
| In reply to | #1694296 |
On 07/23/2017 01:00 PM, Jonathan Cameron wrote:
> On Tue, 18 Jul 2017 14:35:32 +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 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 <fabrice.gasnier@st.com>
>
> 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.
>> ---
>> 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);
>> +
> 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
>> 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[] = {
>
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Cameron <Jonathan.Cameron@huawei.com> |
|---|---|
| Date | 2017-07-24 17:10 +0200 |
| Subject | Re: [PATCH 3/3] iio: adc: stm32: add optional min-sample-time |
| Message-ID | <u6N9F-6A9-29@gated-at.bofh.it> |
| In reply to | #1694532 |
On Mon, 24 Jul 2017 10:16:00 +0200
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> On 07/23/2017 01:00 PM, Jonathan Cameron wrote:
> > On Tue, 18 Jul 2017 14:35:32 +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 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 <fabrice.gasnier@st.com>
> >
> > 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>
> >> }
> >> @@ -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
> 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 ?
Probably best or I'll forget sometime down the line and think it looks
crazy again ;)
Thanks for the explanation.
Jonathan
>
> Please let me know,
> Best Regards,
> Fabrice
>
> >> 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[] = {
> >
> --
> 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
[toc] | [prev] | [next] | [standalone]
| From | Fabrice Gasnier <fabrice.gasnier@st.com> |
|---|---|
| Date | 2017-07-18 14:40 +0200 |
| Subject | [PATCH 1/3] iio: adc: stm32: fix common clock rate |
| Message-ID | <u4zXc-2qo-27@gated-at.bofh.it> |
| In reply to | #1690230 |
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)
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
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
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Cameron <jic23@kernel.org> |
|---|---|
| Date | 2017-07-23 13:00 +0200 |
| Subject | Re: [PATCH 1/3] iio: adc: stm32: fix common clock rate |
| Message-ID | <u6mM9-6va-7@gated-at.bofh.it> |
| In reply to | #1690236 |
On Tue, 18 Jul 2017 14:35:30 +0200
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> 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)
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
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
> ---
> 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;
> }
[toc] | [prev] | [next] | [standalone]
| From | Fabrice Gasnier <fabrice.gasnier@st.com> |
|---|---|
| Date | 2017-07-24 09:50 +0200 |
| Subject | Re: [PATCH 1/3] iio: adc: stm32: fix common clock rate |
| Message-ID | <u6GhP-1Ow-11@gated-at.bofh.it> |
| In reply to | #1694295 |
On 07/23/2017 12:51 PM, Jonathan Cameron wrote:
> On Tue, 18 Jul 2017 14:35:30 +0200
> Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
>
>> 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)
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> 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.
>
> 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
>
> Thanks,
>
> Jonathan
>> ---
>> 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;
>> }
>
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Cameron <Jonathan.Cameron@huawei.com> |
|---|---|
| Date | 2017-07-24 17:10 +0200 |
| Subject | Re: [PATCH 1/3] iio: adc: stm32: fix common clock rate |
| Message-ID | <u6N9F-6A9-27@gated-at.bofh.it> |
| In reply to | #1694502 |
On Mon, 24 Jul 2017 09:43:18 +0200
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> On 07/23/2017 12:51 PM, Jonathan Cameron wrote:
> > On Tue, 18 Jul 2017 14:35:30 +0200
> > Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> >
> >> 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)
> >>
> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > 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.
>
> >
> > 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.
Description is much better thanks. I'll wait for v2 and merge this as a
fix.
Jonathan
>
> Thanks for your review,
> Best Regards,
> Fabrice
> >
> > Thanks,
> >
> > Jonathan
> >> ---
> >> 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;
> >> }
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web