RE: [PATCH v2 4/5] IIO : ADC: tiadc: Add support of TI's ADC driver
Hi,
On Fri, Aug 31, 2012 at 01:12:01, Jonathan Cameron wrote:
> On 08/30/2012 08:38 AM, Patil, Rachna wrote:
> > This patch adds support for TI's ADC driver.
> > This is a multifunctional device.
> > Analog input lines are provided on which voltage measurements can be
> > carried out.
> > You can have upto 8 input lines.
> >
> Nice concise driver.
>
> A few comments and questions inline. Nothing significant really though...
> Half of them are me wanting to improve my understanding of what is going on
> (and not have to remember it in the future ;)
Thanks for the comments. Please find my reply inline.
>
> > Signed-off-by: Patil, Rachna
> > ---
> > Changes in v2:
> > Addressed review comments from Matthias Kaehlcke
> >
> > drivers/iio/adc/Kconfig |7 +
> > drivers/iio/adc/Makefile |1 +
> > drivers/iio/adc/ti_adc.c | 216
> > ++
> > drivers/mfd/ti_tscadc.c | 18 +++-
> > include/linux/mfd/ti_tscadc.h|9 ++-
> > include/linux/platform_data/ti_adc.h | 14 ++
> > 6 files changed, 263 insertions(+), 2 deletions(-) create mode
> > 100644 drivers/iio/adc/ti_adc.c create mode 100644
> > include/linux/platform_data/ti_adc.h
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index
> > 8a78b4f..ad32df8 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -22,4 +22,11 @@ config AT91_ADC
> > help
> > Say yes here to build support for Atmel AT91 ADC.
> >
> > +config TI_ADC
> > + tristate "TI's ADC driver"
> > + depends on ARCH_OMAP2PLUS
> > + help
> > + Say yes here to build support for Texas Instruments ADC
> > + driver which is also a MFD client.
> > +
> > endmenu
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index
> > 52eec25..a930cee 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -4,3 +4,4 @@
> >
> > obj-$(CONFIG_AD7266) += ad7266.o
> > obj-$(CONFIG_AT91_ADC) += at91_adc.o
> > +obj-$(CONFIG_TI_ADC) += ti_adc.o
> > diff --git a/drivers/iio/adc/ti_adc.c b/drivers/iio/adc/ti_adc.c new
> > file mode 100644 index 000..d2e621c
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti_adc.c
> > @@ -0,0 +1,216 @@
> > +/*
> > + * TI ADC MFD driver
> > + *
> > + * Copyright (C) 2012 Texas Instruments Incorporated -
> > +http://www.ti.com/
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied
> > +warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +
> > +struct adc_device {
> > + struct ti_tscadc_dev *mfd_tscadc;
> > + struct iio_dev *idev;
> > + int channels;
> > +};
> > +
> > +static unsigned int adc_readl(struct adc_device *adc, unsigned int
> > +reg) {
> > + return readl(adc->mfd_tscadc->tscadc_base + reg); }
> > +
> > +static void adc_writel(struct adc_device *adc, unsigned int reg,
> > + unsigned int val)
> > +{
> > + writel(val, adc->mfd_tscadc->tscadc_base + reg); }
> > +
> > +static void adc_step_config(struct adc_device *adc_dev) {
> > + unsigned intstepconfig;
> > + int i, channels = 0, steps;
> > +
> > + /*
>
> Don't suppose you could tell us what the steps actually are?
> It's not a term I've come across before and right now I can't seem to find
> the relevant datasheet (and am inherently lazy ;)
The user must first program the Step Configuration registers in order to
configure a channel input to be sampled. There are 16 programmable Step
Configuration registers which are used by the sequencer to control which
switches to turn on or off (inputs to the Analog front end used for
touchscreen), which channel to sample, and which mode to use (HW triggered or
SW enabled, one-shot or continuous mode), averaging, where to save the FIFO
data, etc. The sequencer is completely controlled by software and behaves
accordingly to how the Step Registers are programmed. A step is the general
term for sampling a channel input.
>
> > +* There are 16 configurable steps and 8 analog input
> > +* lines available which are shared between Touchscreen and ADC.
> > +*
> > +* Steps backwards i.e. from 16 towards 0 are used by ADC
> > +* depending on number of input lines needed.
> > +* Channel would represent which analog input
> > +* needs to be given to ADC to digitalize data.
> > +*/
> > +
> > + steps = TOTAL_STEPS
Re: [PATCH v2 4/5] IIO : ADC: tiadc: Add support of TI's ADC driver
On 08/30/2012 08:38 AM, Patil, Rachna wrote:
> This patch adds support for TI's ADC driver.
> This is a multifunctional device.
> Analog input lines are provided on which
> voltage measurements can be carried out.
> You can have upto 8 input lines.
>
Nice concise driver.
A few comments and questions inline. Nothing significant
really though... Half of them are me wanting to improve
my understanding of what is going on (and not have to
remember it in the future ;)
> Signed-off-by: Patil, Rachna
> ---
> Changes in v2:
> Addressed review comments from Matthias Kaehlcke
>
> drivers/iio/adc/Kconfig |7 +
> drivers/iio/adc/Makefile |1 +
> drivers/iio/adc/ti_adc.c | 216
> ++
> drivers/mfd/ti_tscadc.c | 18 +++-
> include/linux/mfd/ti_tscadc.h|9 ++-
> include/linux/platform_data/ti_adc.h | 14 ++
> 6 files changed, 263 insertions(+), 2 deletions(-)
> create mode 100644 drivers/iio/adc/ti_adc.c
> create mode 100644 include/linux/platform_data/ti_adc.h
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 8a78b4f..ad32df8 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -22,4 +22,11 @@ config AT91_ADC
> help
> Say yes here to build support for Atmel AT91 ADC.
>
> +config TI_ADC
> + tristate "TI's ADC driver"
> + depends on ARCH_OMAP2PLUS
> + help
> + Say yes here to build support for Texas Instruments ADC
> + driver which is also a MFD client.
> +
> endmenu
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 52eec25..a930cee 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -4,3 +4,4 @@
>
> obj-$(CONFIG_AD7266) += ad7266.o
> obj-$(CONFIG_AT91_ADC) += at91_adc.o
> +obj-$(CONFIG_TI_ADC) += ti_adc.o
> diff --git a/drivers/iio/adc/ti_adc.c b/drivers/iio/adc/ti_adc.c
> new file mode 100644
> index 000..d2e621c
> --- /dev/null
> +++ b/drivers/iio/adc/ti_adc.c
> @@ -0,0 +1,216 @@
> +/*
> + * TI ADC MFD driver
> + *
> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +struct adc_device {
> + struct ti_tscadc_dev *mfd_tscadc;
> + struct iio_dev *idev;
> + int channels;
> +};
> +
> +static unsigned int adc_readl(struct adc_device *adc, unsigned int reg)
> +{
> + return readl(adc->mfd_tscadc->tscadc_base + reg);
> +}
> +
> +static void adc_writel(struct adc_device *adc, unsigned int reg,
> + unsigned int val)
> +{
> + writel(val, adc->mfd_tscadc->tscadc_base + reg);
> +}
> +
> +static void adc_step_config(struct adc_device *adc_dev)
> +{
> + unsigned intstepconfig;
> + int i, channels = 0, steps;
> +
> + /*
Don't suppose you could tell us what the steps actually are?
It's not a term I've come across before and right now I can't
seem to find the relevant datasheet (and am inherently lazy ;)
> + * There are 16 configurable steps and 8 analog input
> + * lines available which are shared between Touchscreen and ADC.
> + *
> + * Steps backwards i.e. from 16 towards 0 are used by ADC
> + * depending on number of input lines needed.
> + * Channel would represent which analog input
> + * needs to be given to ADC to digitalize data.
> + */
> +
> + steps = TOTAL_STEPS - adc_dev->channels;
> + channels = TOTAL_CHANNELS - adc_dev->channels;
> +
> + stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
> +
> + for (i = (steps + 1); i <= TOTAL_STEPS; i++) {
> + adc_writel(adc_dev, REG_STEPCONFIG(i),
> + stepconfig | STEPCONFIG_INP(channels));
> + adc_writel(adc_dev, REG_STEPDELAY(i),
> + STEPCONFIG_OPENDLY);
> + channels++;
> + }
> + adc_writel(adc_dev, REG_SE, STPENB_STEPENB);
> +}
> +
> +static int tiadc_channel_init(struct iio_dev *idev, struct adc_device
> *adc_dev)
> +{
> + struct iio_chan_spec *chan_array;
> + int i;
> +
> + idev->num_channels = adc_dev->channels;
Given adc_dev is just used for this why not just pass in the number of channels?
> + chan_array = kcalloc(idev->num_channels, sizeof(struct iio_chan_spec),
> + GFP_KERNEL);
> +
> +
