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

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

[Fabrice Gasnier]

STM32 ADC allows each channel to be sampled with a different sampling
time. This series fixes common clock rate and adds optional device
tree property, so minimum sampling time can be tuned. This is done via
device tree as it's tightly coupled with hardware (analog source).

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Fabrice Gasnier]

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

Fix common clock rate used then by stm32-adc sub-devices: take common
prescaler into account.
Fix ADC max clock rate on STM32H7 (fADC from datasheet)

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

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

[Jonathan Cameron]

On Tue, 18 Jul 2017 14:35:30 +0200
Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org> 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-qxv4g6HH51o@public.gmane.org>
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;
>  }

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Fabrice Gasnier]

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;
>>  }
> 

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

[Jonathan Cameron]

On Mon, 24 Jul 2017 09:43:18 +0200
Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org> wrote:

> On 07/23/2017 12:51 PM, Jonathan Cameron wrote:
> > On Tue, 18 Jul 2017 14:35:30 +0200
> > Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org> 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-qxv4g6HH51o@public.gmane.org>  
> > 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-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Fabrice Gasnier]

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

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

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@40012000 {
-- 
1.9.1

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

[Jonathan Cameron]

On Tue, 18 Jul 2017 14:35:31 +0200
Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org> 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-qxv4g6HH51o@public.gmane.org>

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@40012000 {

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

[Fabrice Gasnier]

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@40012000 {
> 

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

[Jonathan Cameron]

On Mon, 24 Jul 2017 09:55:37 +0200
Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org> wrote:

> On 07/23/2017 12:53 PM, Jonathan Cameron wrote:
> > On Tue, 18 Jul 2017 14:35:31 +0200
> > Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org> 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-qxv4g6HH51o@public.gmane.org>  
> > 
> > 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@40012000 {  
> >   
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] iio: adc: stm32: add optional min-sample-time

[Fabrice Gasnier]

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

This patch allows to configure 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

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

[Jonathan Cameron]

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[] = {

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

[Fabrice Gasnier]

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[] = {
> 

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

[Jonathan Cameron]

On Mon, 24 Jul 2017 10:16:00 +0200
Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org> wrote:

> On 07/23/2017 01:00 PM, Jonathan Cameron wrote:
> > On Tue, 18 Jul 2017 14:35:32 +0200
> > Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org> 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-qxv4g6HH51o@public.gmane.org>  
> > 
> > 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html