RE: [PATCH v3 4/5] IIO : ADC: tiadc: Add support of TI's ADC driver

[Patil, Rachna]

On Thu, Sep 13, 2012 at 17:43:30, Jonathan Cameron wrote:
> On 13/09/12 11:40, 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.
> >
> > Signed-off-by: Patil, Rachna <rachna@xxxxxx>
> 
> There's a little fuzz in applying this due to other drivers that have gone in recently.
> 
> Actually this is going to be 'interesting' to merge. Dmitry, Samuel thoughts on who takes this one and how?  Maybe this is a case for a 'special' branch pulled into more than one tree?
> 
> 
> One minor thing inline.  I have an aversion to dynamic allocation of
> things that are then constant.
> 
> Also the module name is simply ti_adc. Does seem a little 'vague'
> given the range of ADC's TI makes :)  Perhaps keep the reference
> to the tsc in there?  Personally I'd have preferred the whole thing
> being named after a particular part number (any one it support would
> do) to avoid a clash in future with a new touch screen adc from TI.
> Bit late for that though I guess ;)

Yes, true.
TI definitely might come up with more IP's of this type.
This IP(TSC / ADC) is present on AM335x. If necessary we can rename the driver to ti_am335x_XXX.

> 
> Jonathan
> > ---
> > Changes in v2:
> > 	Addressed review comments from Matthias Kaehlcke
> >
> > Changes in v3:
> > 	Addressed review comments from Jonathan Cameron.
> > 	Added comments, new line appropriately.
> >
> >   drivers/iio/adc/Kconfig              |    7 +
> >   drivers/iio/adc/Makefile             |    1 +
> >   drivers/iio/adc/ti_adc.c             |  223 ++++++++++++++++++++++++++++++++++
> >   drivers/mfd/ti_tscadc.c              |   18 +++-
> >   include/linux/mfd/ti_tscadc.h        |    9 ++-
> >   include/linux/platform_data/ti_adc.h |   14 ++
> >   6 files changed, 270 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 0000000..56f8af2
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti_adc.c
> > @@ -0,0 +1,223 @@
> > +/*
> > + * 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 <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/err.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/iio/iio.h>
> > +
> > +#include <linux/mfd/ti_tscadc.h>
> > +#include <linux/platform_data/ti_adc.h>
> > +
> > +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 int    stepconfig;
> > +	int i, channels = 0, steps;
> > +
> > +	/*
> > +	 * 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, int channels)
> > +{
> > +	struct iio_chan_spec *chan_array;
> > +	int i;
> > +
> > +	idev->num_channels = channels;
> > +	chan_array = kcalloc(idev->num_channels, sizeof(struct iio_chan_spec),
> > +					GFP_KERNEL);
> > +
> > +	if (chan_array == NULL)
> > +		return -ENOMEM;
> > +
> Minor point, but I'd be tempted to do this as a static const array and 
> then just set num_channels appropriately.  Still it's such a simple 
> driver that I'm not that fussed.

I looked at some other drivers, they seem to be doing the same way.
I would like to go with the existing convention.

> > +	for (i = 0; i < (idev->num_channels); i++) {
> > +		struct iio_chan_spec *chan = chan_array + i;
> > +		chan->type = IIO_VOLTAGE;
> > +		chan->indexed = 1;
> > +		chan->channel = i;
> > +		chan->info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT;
> > +	}
> > +
> > +	idev->channels = chan_array;
> > +
> > +	return idev->num_channels;
> > +}
> > +

Regards,
Rachna
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/