Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752024AbdGWLAa (ORCPT ); Sun, 23 Jul 2017 07:00:30 -0400 Received: from mail.kernel.org ([198.145.29.99]:58912 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751332AbdGWLA1 (ORCPT ); Sun, 23 Jul 2017 07:00:27 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C94B422B4B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=jic23@kernel.org Date: Sun, 23 Jul 2017 12:00:22 +0100 From: Jonathan Cameron To: Fabrice Gasnier Cc: , , , , , , , , , , , , , Subject: Re: [PATCH 3/3] iio: adc: stm32: add optional min-sample-time Message-ID: <20170723120022.7d06063e@kernel.org> In-Reply-To: <1500381332-17086-4-git-send-email-fabrice.gasnier@st.com> References: <1500381332-17086-1-git-send-email-fabrice.gasnier@st.com> <1500381332-17086-4-git-send-email-fabrice.gasnier@st.com> X-Mailer: Claws Mail 3.15.0-dirty (GTK+ 2.24.31; x86_64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12161 Lines: 332 On Tue, 18 Jul 2017 14:35:32 +0200 Fabrice Gasnier 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 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[] = {