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

On 14/09/12 07:00, Patil, Rachna wrote:

On Thu, Sep 13, 2012 at 17:43:30, Jonathan Cameron wrote:

quoted

On 13/09/12 11:40, Patil, Rachna wrote:

quoted

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 <redacted>

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.

I'd be in favour of doing this now rather than when the problem presents 
itself.  Anyone mind?

quoted

Jonathan

quoted

---
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.

quoted

quoted

+	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