On 28/08/14 08:11, Lee Jones wrote: > 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: >>> >>>> Number of averaging, open delay, sample delay are made DT parameters. >>>> By decreasing averaging and delays more samples can be obtained per >>>> 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 <vigneshr@xxxxxx> >>>> --- >>>> .../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(-) >>> >>> [...] >>> >>>> 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 @@ >>> >>> There are so many different formats at play here, it's difficult to >>> see what's happening where! >> >> 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 >>>> >>>> /* Delay register */ >>>> #define STEPDELAY_OPEN_MASK (0x3FFFF << 0) >>>> #define STEPDELAY_OPEN(val) ((val) << 0) >>> >>> What's the point in shifting by zero? >> >> 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. I'm always happy to take sane cleanups like this. Separate cleanup patch please. That way it's easy to verify that there are no functional changes without it getting in the way of the 'interesting' patch. Thanks, Jonathan > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
