Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S932509AbdGXPGo (ORCPT <rfc822;w@1wt.eu>);
        Mon, 24 Jul 2017 11:06:44 -0400
Received: from szxga02-in.huawei.com ([45.249.212.188]:10343 "EHLO
        szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1756245AbdGXPGi (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 24 Jul 2017 11:06:38 -0400
Date: Mon, 24 Jul 2017 23:06:06 +0800
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
To: Fabrice Gasnier <fabrice.gasnier@st.com>
CC: Jonathan Cameron <jic23@kernel.org>, <robh+dt@kernel.org>,
        <linux@armlinux.org.uk>, <mark.rutland@arm.com>,
        <mcoquelin.stm32@gmail.com>, <alexandre.torgue@st.com>,
        <lars@metafoo.de>, <knaack.h@gmx.de>, <pmeerw@pmeerw.net>,
        <benjamin.gaignard@linaro.org>, <benjamin.gaignard@st.com>,
        <linux-iio@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/3] iio: adc: stm32: add optional min-sample-time
Message-ID: <20170724230606.00001044@huawei.com>
In-Reply-To: <80041c76-4428-9330-6dcb-4ca0f718d2e3@st.com>
References: <1500381332-17086-1-git-send-email-fabrice.gasnier@st.com>
        <1500381332-17086-4-git-send-email-fabrice.gasnier@st.com>
        <20170723120022.7d06063e@kernel.org>
        <80041c76-4428-9330-6dcb-4ca0f718d2e3@st.com>
Organization: Huawei
X-Mailer: Claws Mail 3.15.0 (GTK+ 2.24.31; x86_64-w64-mingw32)
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
X-Originating-IP: [10.206.48.115]
X-CFilter-Loop: Reflected
X-Mirapoint-Virus-RAPID-Raw: score=unknown(0),
        refid=str=0001.0A020203.59760CF2.00AB,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0,
        ip=0.0.0.0,
        so=2014-11-16 11:51:01,
        dmn=2013-03-21 17:37:32
X-Mirapoint-Loop-Id: 119dffd830cf1116ee8cf25b869c64fc
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4421
Lines: 122

On Mon, 24 Jul 2017 10:16:00 +0200
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:

> On 07/23/2017 01:00 PM, Jonathan Cameron wrote:
> > On Tue, 18 Jul 2017 14:35:32 +0200
> > Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> >   
> >> STM32 ADC allows each channel to be sampled with a different sampling time,
> >> by setting SMPR registers. Basically, value depends on local electrical
> >> properties. Selecting correct value for sampling time highly depends on
> >> analog source impedance. There is a manual that may help in this process:
> >> 'How to get the best ADC accuracy in STM32...'
> >>
> >> This patch allows to configure min-sample-time via device tree, either for:
> >> - all channels at once:
> >> min-sample-time = <10000>; /* nanosecs */
> >>
> >> - independently for each channel (must match "st,adc-channels" list):
> >> st,adc-channels = <0 1>;
> >> min-sample-time = <5000 10000>; /* nanosecs */
> >>
> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>  
> > 
> > On question inline which may well just be down to me missing what
> > happens when you query index element 2 from a 1 element device tree
> > array.  
> 
> Hi Jonathan,
> 
> I should probably comment on it, please see inline.
> 
<snip>
> >>  }
> >> @@ -1538,8 +1651,8 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
> >>  	struct property *prop;
> >>  	const __be32 *cur;
> >>  	struct iio_chan_spec *channels;
> >> -	int scan_index = 0, num_channels;
> >> -	u32 val;
> >> +	int scan_index = 0, num_channels, ret;
> >> +	u32 val, smp = 0;
> >>  
> >>  	num_channels = of_property_count_u32_elems(node, "st,adc-channels");
> >>  	if (num_channels < 0 ||
> >> @@ -1548,6 +1661,13 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
> >>  		return num_channels < 0 ? num_channels : -EINVAL;
> >>  	}
> >>  
> >> +	/* Optional sample time is provided either for each, or all channels */
> >> +	ret = of_property_count_u32_elems(node, "min-sample-time");
> >> +	if (ret > 1 && ret != num_channels) {
> >> +		dev_err(&indio_dev->dev, "Invalid min-sample-time\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >>  	channels = devm_kcalloc(&indio_dev->dev, num_channels,
> >>  				sizeof(struct iio_chan_spec), GFP_KERNEL);
> >>  	if (!channels)
> >> @@ -1558,9 +1678,13 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
> >>  			dev_err(&indio_dev->dev, "Invalid channel %d\n", val);
> >>  			return -EINVAL;
> >>  		}
> >> +
> >> +		of_property_read_u32_index(node, "min-sample-time", scan_index,
> >> +					   &smp);
> >> +  
> > I might be missing something, but doesn't this fail for the single shared
> > value case?  
> 
> Yes, this fails in the single shared value case, with index >= 1.
> Rewinding... when index is 0, it picks up 1st (shared) value.
> Then fails for other index values.
> 
> of_property_read_u32_index() documentation states out value remains
> untouched in case of error:
> /**
>  * of_property_read_u32_index...
> [...]
>  * The out_value is modified only if a valid u32 value can be decoded.
> 
> This is what I use here (as far as I tested it, it worked like a charm).
> Do you wish I add a comment to describe this ?
Probably best or I'll forget sometime down the line and think it looks
crazy again ;)

Thanks for the explanation.

Jonathan
> 
> Please let me know,
> Best Regards,
> Fabrice
> 
> >>  		stm32_adc_chan_init_one(indio_dev, &channels[scan_index],
> >>  					&adc_info->channels[val],
> >> -					scan_index);
> >> +					scan_index, smp);
> >>  		scan_index++;
> >>  	}
> >>  
> >> @@ -1755,6 +1879,7 @@ static int stm32_adc_remove(struct platform_device *pdev)
> >>  	.clk_required = true,
> >>  	.start_conv = stm32f4_adc_start_conv,
> >>  	.stop_conv = stm32f4_adc_stop_conv,
> >> +	.smp_cycles = stm32f4_adc_smp_cycles,
> >>  };
> >>  
> >>  static const struct stm32_adc_cfg stm32h7_adc_cfg = {
> >> @@ -1766,6 +1891,7 @@ static int stm32_adc_remove(struct platform_device *pdev)
> >>  	.stop_conv = stm32h7_adc_stop_conv,
> >>  	.prepare = stm32h7_adc_prepare,
> >>  	.unprepare = stm32h7_adc_unprepare,
> >> +	.smp_cycles = stm32h7_adc_smp_cycles,
> >>  };
> >>  
> >>  static const struct of_device_id stm32_adc_of_match[] = {  
> >   
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html