Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932378AbdGXPEB (ORCPT ); Mon, 24 Jul 2017 11:04:01 -0400 Received: from szxga02-in.huawei.com ([45.249.212.188]:10342 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753017AbdGXPDw (ORCPT ); Mon, 24 Jul 2017 11:03:52 -0400 Date: Mon, 24 Jul 2017 23:03:01 +0800 From: Jonathan Cameron To: Fabrice Gasnier CC: Jonathan Cameron , , , , , , , , , , , , , , Subject: Re: [PATCH 1/3] iio: adc: stm32: fix common clock rate Message-ID: <20170724230301.00007791@huawei.com> In-Reply-To: <0385445a-8be9-6e67-b19d-b72466c23e34@st.com> References: <1500381332-17086-1-git-send-email-fabrice.gasnier@st.com> <1500381332-17086-2-git-send-email-fabrice.gasnier@st.com> <20170723115148.07db4374@kernel.org> <0385445a-8be9-6e67-b19d-b72466c23e34@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.59760C38.0041,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: 31abffb8273fce47564eaefbf7ff529b Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4089 Lines: 111 On Mon, 24 Jul 2017 09:43:18 +0200 Fabrice Gasnier wrote: > On 07/23/2017 12:51 PM, Jonathan Cameron wrote: > > On Tue, 18 Jul 2017 14:35:30 +0200 > > Fabrice Gasnier 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 > > Patch itself is fine, but the description could do with > > information on what the result of this being wrong is. > > Hi Jonathan, > > I agree with you and will improve description in v2. I'm thinking of: > > Fixes: 95e339b6e85d ("iio: adc: stm32: add support for STM32H7") > > ADC clock input is provided to internal prescaler (that decreases its > frequency). It's then used as reference clock for conversions. > > - Fix common clock rate used then by stm32-adc sub-devices. Take common > prescaler into account. Currently, rate is used to set "boost" mode. It > may unnecessarily be set. This impacts power consumption. > - Fix ADC max clock rate on STM32H7 (fADC from datasheet). Currently, > prescaler may be set too low. This can result in ADC reference clock > used for conversion to exceed max allowed clock frequency. > > > > > I have no idea if this is a patch I should be sending upstream > > asap or should hold for the next merge window. > > Probably yes... I hope the description is better above? Just to mention > this impacts stm32h7 adc, device tree node to use it is not yet integrated. Description is much better thanks. I'll wait for v2 and merge this as a fix. Jonathan > > Thanks for your review, > Best Regards, > Fabrice > > > > Thanks, > > > > Jonathan > >> --- > >> drivers/iio/adc/stm32-adc-core.c | 10 +++++----- > >> 1 file changed, 5 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c > >> index e09233b..6096763 100644 > >> --- a/drivers/iio/adc/stm32-adc-core.c > >> +++ b/drivers/iio/adc/stm32-adc-core.c > >> @@ -64,7 +64,7 @@ > >> #define STM32H7_CKMODE_MASK GENMASK(17, 16) > >> > >> /* STM32 H7 maximum analog clock rate (from datasheet) */ > >> -#define STM32H7_ADC_MAX_CLK_RATE 72000000 > >> +#define STM32H7_ADC_MAX_CLK_RATE 36000000 > >> > >> /** > >> * stm32_adc_common_regs - stm32 common registers, compatible dependent data > >> @@ -148,14 +148,14 @@ static int stm32f4_adc_clk_sel(struct platform_device *pdev, > >> return -EINVAL; > >> } > >> > >> - priv->common.rate = rate; > >> + priv->common.rate = rate / stm32f4_pclk_div[i]; > >> val = readl_relaxed(priv->common.base + STM32F4_ADC_CCR); > >> val &= ~STM32F4_ADC_ADCPRE_MASK; > >> val |= i << STM32F4_ADC_ADCPRE_SHIFT; > >> writel_relaxed(val, priv->common.base + STM32F4_ADC_CCR); > >> > >> dev_dbg(&pdev->dev, "Using analog clock source at %ld kHz\n", > >> - rate / (stm32f4_pclk_div[i] * 1000)); > >> + priv->common.rate / 1000); > >> > >> return 0; > >> } > >> @@ -250,7 +250,7 @@ static int stm32h7_adc_clk_sel(struct platform_device *pdev, > >> > >> out: > >> /* rate used later by each ADC instance to control BOOST mode */ > >> - priv->common.rate = rate; > >> + priv->common.rate = rate / div; > >> > >> /* Set common clock mode and prescaler */ > >> val = readl_relaxed(priv->common.base + STM32H7_ADC_CCR); > >> @@ -260,7 +260,7 @@ static int stm32h7_adc_clk_sel(struct platform_device *pdev, > >> writel_relaxed(val, priv->common.base + STM32H7_ADC_CCR); > >> > >> dev_dbg(&pdev->dev, "Using %s clock/%d source at %ld kHz\n", > >> - ckmode ? "bus" : "adc", div, rate / (div * 1000)); > >> + ckmode ? "bus" : "adc", div, priv->common.rate / 1000); > >> > >> return 0; > >> } > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel