From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lee Jones Subject: Re: [PATCH 2/2] iio: adc: ti_am335x_adc: make sample delay, open delay, averaging DT parameters Date: Thu, 28 Aug 2014 08:11:23 +0100 Message-ID: <20140828071123.GE24579@lee--X1> References: <1409141990-29627-1-git-send-email-vigneshr@ti.com> <1409141990-29627-2-git-send-email-vigneshr@ti.com> <20140827135635.GH6364@lee--X1> <53FEBA43.2000607@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <53FEBA43.2000607@ti.com> Sender: linux-doc-owner@vger.kernel.org To: Vignesh R Cc: Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Jonathan Cameron , Randy Dunlap , Samuel Ortiz , Felipe Balbi , Sebastian Andrzej Siewior , devicetree@vger.kernel.org, linux-doc@vger.kernel.org, linux-iio@vger.kernel.org, Sekhar Nori List-Id: devicetree@vger.kernel.org On Thu, 28 Aug 2014, Vignesh R wrote: > On Wednesday 27 August 2014 07:26 PM, Lee Jones wrote: > > On Wed, 27 Aug 2014, Vignesh R wrote: > >=20 > >> Number of averaging, open delay, sample delay are made DT paramete= rs. > >> By decreasing averaging and delays more samples can be obtained pe= r > >> second increasing performance of ADC. Previously the number of > >> averages per step was fixed to 16. Making these parameters > >> configurable will help in balancing speed vs accuracy. > >> For each ADC step provide DT based paramters to set open delay, > >> sample delay and number of averaging. One configurable step is > >> used per ADC channel. Since there can be atmost 8 ADC channels, > >> steps 16 to 8 are used for ADC. > >> > >> Signed-off-by: Vignesh R > >> --- > >> .../bindings/input/touchscreen/ti-tsc-adc.txt | 18 +++++++ > >> drivers/iio/adc/ti_am335x_adc.c | 49 +++++++= ++++++++++--- > >> include/linux/mfd/ti_am335x_tscadc.h | 3 ++ > >> 3 files changed, 63 insertions(+), 7 deletions(-) > >=20 > > [...] > >=20 > >> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/= mfd/ti_am335x_tscadc.h > >> index fb96c84..26d3e84 100644 > >> --- a/include/linux/mfd/ti_am335x_tscadc.h > >> +++ b/include/linux/mfd/ti_am335x_tscadc.h > >> @@ -82,14 +82,17 @@ > >=20 > > There are so many different formats at play here, it's difficult to > > see what's happening where! >=20 > I did not understand "different formats". Please explain. My complaint is with the existing layout, rather than your patch: > #define STEPCONFIG_INP_AN4 STEPCONFIG_INP(4) > #define STEPCONFIG_INP_ADCREFM STEPCONFIG_INP(8) > #define STEPCONFIG_FIFO1 BIT(26) Base10 MACROS. #define STEPCONFIG_AVG_MAX 16 Base10 int. > /* Delay register */ > #define STEPDELAY_OPEN_MASK (0x3FFFF << 0) Base16 shift. > #define STEPDELAY_OPEN(val) ((val) << 0) MACRO shift. > #define STEPCONFIG_OPENDLY STEPDELAY_OPEN(0x098) Base16 MACRO. > #define STEPDELAY_OPEN_MAX 0x3FFFF Base16 int. > #define STEPDELAY_SAMPLE_MASK (0xFF << 24) Base16 shift. > #define STEPDELAY_SAMPLE(val) ((val) << 24) MACRO shift. > #define STEPCONFIG_SAMPLEDLY STEPDELAY_SAMPLE(0) 24 shift of 0 (still zero?). > #define STEPDELAY_SAMPLE_MAX 0xFF Base16 int. There is no consistency here, it's just a bunch of 'stuff', which makes making sense of them all difficult. > >> #define STEPCONFIG_INP_AN4 STEPCONFIG_INP(4) > >> #define STEPCONFIG_INP_ADCREFM STEPCONFIG_INP(8) > >> #define STEPCONFIG_FIFO1 BIT(26) > >> +#define STEPCONFIG_AVG_MAX 16 > >> =20 > >> /* Delay register */ > >> #define STEPDELAY_OPEN_MASK (0x3FFFF << 0) > >> #define STEPDELAY_OPEN(val) ((val) << 0) > >=20 > > What's the point in shifting by zero? >=20 > I agree. But my patch did not add these lines. I can remove them. Do = you > want me to do this change as part of current patch or as a separate > cleanup patch? That's up to Jonathan. --=20 Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org =E2=94=82 Open source software for ARM SoCs =46ollow Linaro: Facebook | Twitter | Blog