* [PATCH 0/3] Allow to tune sampling time on STM32 ADC
@ 2017-07-18 12:35 Fabrice Gasnier
2017-07-18 12:35 ` [PATCH 1/3] iio: adc: stm32: fix common clock rate Fabrice Gasnier
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Fabrice Gasnier @ 2017-07-18 12:35 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/3] iio: adc: stm32: fix common clock rate 2017-07-18 12:35 [PATCH 0/3] Allow to tune sampling time on STM32 ADC Fabrice Gasnier @ 2017-07-18 12:35 ` Fabrice Gasnier 2017-07-23 10:51 ` Jonathan Cameron 2017-07-18 12:35 ` [PATCH 2/3] dt-bindings: iio: adc: stm32: add optional min-sample-time Fabrice Gasnier 2017-07-18 12:35 ` [PATCH 3/3] " Fabrice Gasnier 2 siblings, 1 reply; 13+ messages in thread From: Fabrice Gasnier @ 2017-07-18 12:35 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 1/3] iio: adc: stm32: fix common clock rate 2017-07-18 12:35 ` [PATCH 1/3] iio: adc: stm32: fix common clock rate Fabrice Gasnier @ 2017-07-23 10:51 ` Jonathan Cameron 2017-07-24 7:43 ` Fabrice Gasnier 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Cameron @ 2017-07-23 10:51 UTC (permalink / raw) To: linux-arm-kernel 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; > } ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] iio: adc: stm32: fix common clock rate 2017-07-23 10:51 ` Jonathan Cameron @ 2017-07-24 7:43 ` Fabrice Gasnier 2017-07-24 15:03 ` Jonathan Cameron 0 siblings, 1 reply; 13+ messages in thread From: Fabrice Gasnier @ 2017-07-24 7:43 UTC (permalink / raw) To: linux-arm-kernel 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; >> } > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] iio: adc: stm32: fix common clock rate 2017-07-24 7:43 ` Fabrice Gasnier @ 2017-07-24 15:03 ` Jonathan Cameron 0 siblings, 0 replies; 13+ messages in thread From: Jonathan Cameron @ 2017-07-24 15:03 UTC (permalink / raw) To: linux-arm-kernel 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 at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] dt-bindings: iio: adc: stm32: add optional min-sample-time 2017-07-18 12:35 [PATCH 0/3] Allow to tune sampling time on STM32 ADC Fabrice Gasnier 2017-07-18 12:35 ` [PATCH 1/3] iio: adc: stm32: fix common clock rate Fabrice Gasnier @ 2017-07-18 12:35 ` Fabrice Gasnier 2017-07-23 10:53 ` Jonathan Cameron 2017-07-18 12:35 ` [PATCH 3/3] " Fabrice Gasnier 2 siblings, 1 reply; 13+ messages in thread From: Fabrice Gasnier @ 2017-07-18 12:35 UTC (permalink / raw) To: linux-arm-kernel 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 <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: adc at 40012000 { -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] dt-bindings: iio: adc: stm32: add optional min-sample-time 2017-07-18 12:35 ` [PATCH 2/3] dt-bindings: iio: adc: stm32: add optional min-sample-time Fabrice Gasnier @ 2017-07-23 10:53 ` Jonathan Cameron 2017-07-24 7:55 ` Fabrice Gasnier 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Cameron @ 2017-07-23 10:53 UTC (permalink / raw) To: linux-arm-kernel On Tue, 18 Jul 2017 14:35:31 +0200 Fabrice Gasnier <fabrice.gasnier@st.com> wrote: > STM32 ADC allows each channel to be sampled with a different sampling > time. There's an application note that deals with this: 'How to get > the best ADC accuracy in STM32...' It basically depends on analog input > signal electrical properties (depends on board). > > Add optional 'min-sample-time' property so this can be tuned in dt. > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> 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 > --- > 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: adc at 40012000 { ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] dt-bindings: iio: adc: stm32: add optional min-sample-time 2017-07-23 10:53 ` Jonathan Cameron @ 2017-07-24 7:55 ` Fabrice Gasnier 2017-07-24 15:04 ` Jonathan Cameron 0 siblings, 1 reply; 13+ messages in thread From: Fabrice Gasnier @ 2017-07-24 7:55 UTC (permalink / raw) To: linux-arm-kernel On 07/23/2017 12:53 PM, Jonathan Cameron wrote: > On Tue, 18 Jul 2017 14:35:31 +0200 > Fabrice Gasnier <fabrice.gasnier@st.com> wrote: > >> STM32 ADC allows each channel to be sampled with a different sampling >> time. There's an application note that deals with this: 'How to get >> the best ADC accuracy in STM32...' It basically depends on analog input >> signal electrical properties (depends on board). >> >> Add optional 'min-sample-time' property so this can be tuned in dt. >> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > > 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 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 > > Otherwise, seems good to me. > > Jonathan >> --- >> 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: adc at 40012000 { > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] dt-bindings: iio: adc: stm32: add optional min-sample-time 2017-07-24 7:55 ` Fabrice Gasnier @ 2017-07-24 15:04 ` Jonathan Cameron 0 siblings, 0 replies; 13+ messages in thread From: Jonathan Cameron @ 2017-07-24 15:04 UTC (permalink / raw) To: linux-arm-kernel On Mon, 24 Jul 2017 09:55:37 +0200 Fabrice Gasnier <fabrice.gasnier@st.com> wrote: > On 07/23/2017 12:53 PM, Jonathan Cameron wrote: > > On Tue, 18 Jul 2017 14:35:31 +0200 > > Fabrice Gasnier <fabrice.gasnier@st.com> wrote: > > > >> STM32 ADC allows each channel to be sampled with a different sampling > >> time. There's an application note that deals with this: 'How to get > >> the best ADC accuracy in STM32...' It basically depends on analog input > >> signal electrical properties (depends on board). > >> > >> Add optional 'min-sample-time' property so this can be tuned in dt. > >> > >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > > > > 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 > > 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' ? 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 > > Thanks for reviewing, > Best regards, > Fabrice > > > > > Otherwise, seems good to me. > > > > Jonathan > >> --- > >> 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: adc at 40012000 { > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] iio: adc: stm32: add optional min-sample-time 2017-07-18 12:35 [PATCH 0/3] Allow to tune sampling time on STM32 ADC Fabrice Gasnier 2017-07-18 12:35 ` [PATCH 1/3] iio: adc: stm32: fix common clock rate Fabrice Gasnier 2017-07-18 12:35 ` [PATCH 2/3] dt-bindings: iio: adc: stm32: add optional min-sample-time Fabrice Gasnier @ 2017-07-18 12:35 ` Fabrice Gasnier 2017-07-23 11:00 ` Jonathan Cameron 2 siblings, 1 reply; 13+ messages in thread From: Fabrice Gasnier @ 2017-07-18 12:35 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] iio: adc: stm32: add optional min-sample-time 2017-07-18 12:35 ` [PATCH 3/3] " Fabrice Gasnier @ 2017-07-23 11:00 ` Jonathan Cameron 2017-07-24 8:16 ` Fabrice Gasnier 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Cameron @ 2017-07-23 11:00 UTC (permalink / raw) To: linux-arm-kernel 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[] = { ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] iio: adc: stm32: add optional min-sample-time 2017-07-23 11:00 ` Jonathan Cameron @ 2017-07-24 8:16 ` Fabrice Gasnier 2017-07-24 15:06 ` Jonathan Cameron 0 siblings, 1 reply; 13+ messages in thread From: Fabrice Gasnier @ 2017-07-24 8:16 UTC (permalink / raw) To: linux-arm-kernel 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[] = { > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] iio: adc: stm32: add optional min-sample-time 2017-07-24 8:16 ` Fabrice Gasnier @ 2017-07-24 15:06 ` Jonathan Cameron 0 siblings, 0 replies; 13+ messages in thread From: Jonathan Cameron @ 2017-07-24 15:06 UTC (permalink / raw) To: linux-arm-kernel 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 at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-07-24 15:06 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-07-18 12:35 [PATCH 0/3] Allow to tune sampling time on STM32 ADC Fabrice Gasnier 2017-07-18 12:35 ` [PATCH 1/3] iio: adc: stm32: fix common clock rate Fabrice Gasnier 2017-07-23 10:51 ` Jonathan Cameron 2017-07-24 7:43 ` Fabrice Gasnier 2017-07-24 15:03 ` Jonathan Cameron 2017-07-18 12:35 ` [PATCH 2/3] dt-bindings: iio: adc: stm32: add optional min-sample-time Fabrice Gasnier 2017-07-23 10:53 ` Jonathan Cameron 2017-07-24 7:55 ` Fabrice Gasnier 2017-07-24 15:04 ` Jonathan Cameron 2017-07-18 12:35 ` [PATCH 3/3] " Fabrice Gasnier 2017-07-23 11:00 ` Jonathan Cameron 2017-07-24 8:16 ` Fabrice Gasnier 2017-07-24 15:06 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).