Re: [PATCH 2/2] [RFC] Basic support for MX28 LRADC IIO interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On 2/10/2012 11:17 AM, Marek Vasut wrote:
On 2/10/2012 5:52 AM, Marek Vasut wrote:
Signed-off-by: Marek Vasut<marek.vasut@xxxxxxxxx>
Cc: Wolfgang Denk<wd@xxxxxxx>
Cc: Stefano Babic<sbabic@xxxxxxx>
Cc: Fabio Estevam<festevam@xxxxxxxxx>
---

   arch/arm/mach-mxs/mach-m28evk.c        |   35 ++-
   drivers/staging/iio/adc/Kconfig        |   10 +
   drivers/staging/iio/adc/Makefile       |    1 +
   drivers/staging/iio/adc/mxs-lradc.c    |  666
   ++++++++++++++++++++++++++++++++ 4 files changed, 700 insertions(+), 4
   deletions(-)
   create mode 100644 drivers/staging/iio/adc/mxs-lradc.c

NOTE: The quality of code is still really crap here, I'm more interested
in knowing whether I'm going in the right direction about this.
I may well point out stuff you'd fix anyway, but its easier for me that
way ;)

Anyhow, having taken a look I like the general approach.  The 'claim'
logic is nice and clean.
Really? Or what that irony ? ;-)
Logic is clean enough. Never said anything about the implementation and the complexity of doing it ;)

One nasty bit I hadn't realized is that you get separate interrupts per
channel. This is going
to make the data aggregation and demuxing used with buffers interesting
if you add that
support.

So thinking ahead, lets say you have 4 channels with oversampling as
ax2,bx1,cx4,dx8
But the oversampling happens in hardware. And I get a single interrupt at the
end of the sampling.
The problem with our buffered route is that it is done as one buffer per iio device. This makes sense from the point of view of most devices which have quick means of dumpling a whole 'scan' across the channels. The snag here is you can get different numbers of readings from different channels even when driven by the
same underlying trigger signal.

It will interrupt on each channel as
xxxxxxxx
    a a a a a
bbbbbbb
      c    c
          d
Easiest option will be to build up this as a single described 'scan' and
push that to the demux.
Fiddly and if there is a big oversampling value, everything gets
delayed.  Maybe we send scans
with a flag indicating no data for some fields and demux accordingly.
Probably reasonable to
restrict a consumer driver to having only one oversampling so as far as
they know it will come
through at that lower rate.

Still all that stuff is for another day.  Your driver is fine in as far
as it goes!
Great, now I can continue to adding touchscreen, temp etc.
Yes. This would make an interesting test case for our various in kernel interfaces though I appreciate you may not want to take that path as there will be a few corners requiring changes in the IIO core.

Thanks!

Thanks!

diff --git a/arch/arm/mach-mxs/mach-m28evk.c
b/arch/arm/mach-mxs/mach-m28evk.c index 2f27582..7a37d29 100644
--- a/arch/arm/mach-mxs/mach-m28evk.c
+++ b/arch/arm/mach-mxs/mach-m28evk.c
@@ -320,6 +320,31 @@ static struct mxs_mmc_platform_data
m28evk_mmc_pdata[] __initdata = {

   	},

   };

+#define	RES_IRQ(id, res)	{ .name = id, .start = (res), .end =
(res),
.flags = IORESOURCE_IRQ } +
+static struct resource mxs_lradc_rsrc[] = {
+	[0] = {
+		.start	= 0x80050000,
+		.end	= 0x80050000 + SZ_8K - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	RES_IRQ("LRADC CH0", MX28_INT_LRADC_CH0),
+	RES_IRQ("LRADC CH1", MX28_INT_LRADC_CH1),
+	RES_IRQ("LRADC CH2", MX28_INT_LRADC_CH2),
+	RES_IRQ("LRADC CH3", MX28_INT_LRADC_CH3),
+	RES_IRQ("LRADC CH4", MX28_INT_LRADC_CH4),
+	RES_IRQ("LRADC CH5", MX28_INT_LRADC_CH5),
+	RES_IRQ("LRADC CH6", MX28_INT_LRADC_CH6),
+	RES_IRQ("LRADC CH7", MX28_INT_LRADC_CH7),
+};
+
+static struct platform_device mxs_lradc = {
+	.name		= "mxs-lradc",
+	.id		= -1,
+	.resource	= mxs_lradc_rsrc,
+	.num_resources	= ARRAY_SIZE(mxs_lradc_rsrc),
+};
+

   static void __init m28evk_init(void)
   {

   	mxs_iomux_setup_multiple_pads(m28evk_pads, ARRAY_SIZE(m28evk_pads));

@@ -347,6 +372,8 @@ static void __init m28evk_init(void)

   	mx28_add_mxs_i2c(0);
   	i2c_register_board_info(0, m28_stk5v3_i2c_boardinfo,
   	
   			ARRAY_SIZE(m28_stk5v3_i2c_boardinfo));

+
+	platform_device_register(&mxs_lradc);

   }

   static void __init m28evk_timer_init(void)

diff --git a/drivers/staging/iio/adc/Kconfig
b/drivers/staging/iio/adc/Kconfig index d9decea..7f33032 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -193,4 +193,14 @@ config MAX1363_RING_BUFFER

   	  Say yes here to include ring buffer support in the MAX1363
   	  ADC driver.

+config MXS_LRADC
+	tristate "Freescale i.MX23/i.MX28 LRADC"
+	depends on ARCH_MXS
+	help
+	  Say yes here to build support for i.MX23/i.MX28 LRADC convertor
+	  built into these chips.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called mxs-lradc.
+

   endmenu

diff --git a/drivers/staging/iio/adc/Makefile
b/drivers/staging/iio/adc/Makefile index ceee7f3..2d764ec 100644
--- a/drivers/staging/iio/adc/Makefile
+++ b/drivers/staging/iio/adc/Makefile
@@ -37,3 +37,4 @@ obj-$(CONFIG_AD7192) += ad7192.o

   obj-$(CONFIG_ADT7310) += adt7310.o
   obj-$(CONFIG_ADT7410) += adt7410.o
   obj-$(CONFIG_AD7280) += ad7280a.o

+obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
diff --git a/drivers/staging/iio/adc/mxs-lradc.c
b/drivers/staging/iio/adc/mxs-lradc.c new file mode 100644
index 0000000..da50088
--- /dev/null
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -0,0 +1,666 @@
+/*
+ * ADT7410 digital temperature sensor driver supporting ADT7410
+ *
+ * Copyright 2010 Analog Devices Inc.
Obvious bit to fix here ;)  Doesn't bode that well that you have the
copyright
notice for a driver I've been trying to get the heck out of IIO.
*fingers crossed*
Heh, thanks for pointing this out.

+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+#include<linux/interrupt.h>
+#include<linux/device.h>
+#include<linux/kernel.h>
+#include<linux/slab.h>
+#include<linux/sysfs.h>
+#include<linux/list.h>
+#include<linux/io.h>
+#include<linux/module.h>
+#include<linux/platform_device.h>
+#include<linux/spinlock.h>
+#include<linux/wait.h>
+#include<linux/sched.h>
+
+#include<mach/mxs.h>
+#include<mach/common.h>
+
+#include "../iio.h"
+#include "../sysfs.h"
+
+#define	MAPPING_USED		(1<<   7)
+#define	MAPPING_XXX		(1<<   6)
+#define	MAPPING_SET_SLOT(m)	(((m)&   0x3)<<   4)
+#define	MAPPING_GET_SLOT(m)	(((m)>>   4)&   0x3)
+#define	MAPPING_CHAN(m)		((m)&   0xf)
+
+struct mxs_lradc_mapped_channel {
+	wait_queue_head_t		wq;
+	bool				wq_done;
+	uint16_t			chan_data;
+	uint16_t			mapping;
+	uint32_t			users;
+};
+
+struct mxs_lradc_drv_data {
+	struct iio_dev			*iio[4];
+	void __iomem			*mmio_base;
+
+	spinlock_t			lock;
+
+	uint16_t			claimed;
+
+	struct mxs_lradc_mapped_channel	ch[8];
yikes. Individual oversample controls. I was assuming these were per
sample trigger source.
That's going to make for some fiddly data management when you add
buffers.  We'll
have to define the ordering of data very carefully and the demux unit
will need adapting to
handle this.  Fun fun fun.  (note the demux unit becomes very important
for in kernel consumers
of iio buffered data).  I really need to get that code out there in a
more up to date form.
   Now I think Greg has been talked round to taking the 'pull interface'
set we can hopefully make
real progress on that front.

I'm never sure if I like 'interesting' hardware or if it just gives me a
headache!
Right!
+	uint8_t				ch_oversample[16];
+};
+
+struct mxs_lradc_data {
+	struct mxs_lradc_drv_data	*drv_data;
+	int				id;
+	spinlock_t			lock;
+
+	uint16_t			claimed;
+
+	uint32_t			loop_interval;
+};
+
+
+#define	LRADC_CH(n)				(0x50 + (0x10 * (n)))
+#define	LRADC_CH_ACCUMULATE			(1<<   29)
+#define	LRADC_CH_NUM_SAMPLES_MASK		(0x1f<<   24)
+#define	LRADC_CH_NUM_SAMPLES_OFFSET		24
+#define	LRADC_CH_VALUE_MASK			0x3ffff
+#define	LRADC_CH_VALUE_OFFSET			0
+
+#define	LRADC_DELAY(n)				(0xd0 + (0x10 * (n)))
+#define	LRADC_DELAY_TRIGGER_LRADCS_MASK		(0xff<<   24)
+#define	LRADC_DELAY_TRIGGER_LRADCS_OFFSET	24
+#define	LRADC_DELAY_KICK			(1<<   20)
+#define	LRADC_DELAY_TRIGGER_DELAYS_MASK		(0xf<<   16)
+#define	LRADC_DELAY_TRIGGER_DELAYS_OFFSET	16
+#define	LRADC_DELAY_LOOP_COUNT_MASK		(0x1f<<   11)
+#define	LRADC_DELAY_LOOP_COUNT_OFFSET		11
+#define	LRADC_DELAY_DELAY_MASK			0x7ff
+#define	LRADC_DELAY_DELAY_OFFSET		0
+
+#define	LRADC_CTRL0				0x00
+
+#define	LRADC_CTRL1				0x10
+#define	LRADC_CTRL1_LRADC_IRQ(n)		(1<<   (n))
+#define	LRADC_CTRL1_LRADC_IRQ_EN(n)		(1<<   ((n) + 16))
+
+#define	LRADC_CTRL2				0x20
+#define	LRADC_CTRL2_TEMPSENSE_PWD		(1<<   15)
+
+#define	LRADC_CTRL3				0x30
+
+#define	LRADC_CTRL4				0x140
+#define	LRADC_CTRL4_LRADCSELECT_MASK(n)		(0xf<<   ((n) * 4))
+#define	LRADC_CTRL4_LRADCSELECT_OFFSET(n)	((n) * 4)
The mess above will go into separate include.
Only do that it if it is needed elsewhere, otherwise it should just stay in here even if it looks a little messy.

+
+/*
+ * Global IIO attributes
+ */
+static ssize_t mxs_lradc_show_sampling_rate(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *iio_dev = dev_get_drvdata(dev);
+	struct mxs_lradc_data *data = iio_priv(iio_dev);
+
+	return sprintf(buf, "%u\n", data->loop_interval);
+}
+
+static ssize_t mxs_lradc_store_sampling_rate(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct iio_dev *iio_dev = dev_get_drvdata(dev);
+	struct mxs_lradc_data *data = iio_priv(iio_dev);
+	struct mxs_lradc_drv_data *drv_data = data->drv_data;
+	uint32_t reg;
+	unsigned long lval;
+	unsigned long lflags;
+
+	if (strict_strtoul(buf, 10,&lval))
+		return -EINVAL;
+
+	/* Check if the value is in requested range */
+	if (lval>= 2048)
+		return -EINVAL;
+
+	/*
+	 * Noone is accessing our delay trigger in the LRADC reg space so we
+	 * don't need to claim top-level spinlock.
+	 */
+	spin_lock_irqsave(&data->lock, lflags);
+
+	if (data->loop_interval != lval) {
+		data->loop_interval = lval;
+
+		/* Update the delay channel */
+		reg = readl(drv_data->mmio_base + LRADC_DELAY(data->id));
+		reg&= ~LRADC_DELAY_DELAY_MASK;
+		reg |= data->loop_interval;
+		writel(reg, drv_data->mmio_base + LRADC_DELAY(data->id));
+	}
+
+	spin_unlock_irqrestore(&data->lock, lflags);
+
+	return count;
+}
+
+static IIO_DEVICE_ATTR(sampling_rate, S_IRUGO | S_IWUSR,
+		       mxs_lradc_show_sampling_rate,
+		       mxs_lradc_store_sampling_rate, 0);
Hmm..  Not abi compliant, we probably do need to come up with a format
for specifying
such range restrictions. Also note sampling rate is always in hz.  This
will require
some fixed point fiddling but the interface has to be predictable.
Yep, I didn't know how to actually handle this. Since this configuration is per-
iio device, unlike the channel oversampling. Any pointers welcome.
Pretty much as you have done, just with stuff in hz. Getting frequency control to a point where we can get to it from client drivers is on the todo list, but right now what you have is the only way of doing it.

+static IIO_CONST_ATTR(sampling_rate_available,
+			"0...2047 (in 1/2000 second steps)");
+
+static struct attribute *mxs_lradc_attributes[] = {
+	&iio_dev_attr_sampling_rate.dev_attr.attr,
+	&iio_const_attr_sampling_rate_available.dev_attr.attr,
+	NULL,
+};
+
+static int mxs_lradc_can_claim_channel(struct iio_dev *iio_dev,
+			const struct iio_chan_spec *chan)
+{
+	struct mxs_lradc_data *data = iio_priv(iio_dev);
+	struct mxs_lradc_drv_data *drv_data = data->drv_data;
+	int i, count = 0;
+
+	/* The channel is already claimed by us. */
+	if (data->claimed&   (1<<   chan->address))
+		return 0;
+
+	/* Check if someone else didn't claim this */
+	if (drv_data->claimed&   (1<<   chan->address))
+		return -EINVAL;
+
+	for (i = 0; i<   16; i++)
+		if (drv_data->claimed&   (1<<   i))
+			count++;
+
+	/* Too many channels claimed */
+	if (count == 8)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int mxs_lradc_claim_channel(struct iio_dev *iio_dev,
+			const struct iio_chan_spec *chan)
+{
+	struct mxs_lradc_data *data = iio_priv(iio_dev);
+	struct mxs_lradc_drv_data *drv_data = data->drv_data;
+	int i, ret;
+	unsigned long gflags;
+	uint32_t overspl = 0;
+
+	spin_lock_irqsave(&drv_data->lock, gflags);
+
+	ret = mxs_lradc_can_claim_channel(iio_dev, chan);
+	if (ret)
+		goto err;
+
+	/* Claim the channel */
+	drv_data->claimed |= 1<<   chan->address;
+	data->claimed |= 1<<   chan->address;
+
+	/* Map the channel */
+	for (i = 0; i<   8; i++)
+		if (!(drv_data->ch[i].mapping&   MAPPING_USED))
+			break;
+
+	drv_data->ch[i].mapping = MAPPING_USED |
+				MAPPING_SET_SLOT(data->id) |
+				MAPPING_CHAN(chan->address);
+
+	/* Setup the mapping */
+	__mxs_clrl(LRADC_CTRL4_LRADCSELECT_MASK(i),
+		drv_data->mmio_base + LRADC_CTRL4);
+	__mxs_setl(chan->address<<   LRADC_CTRL4_LRADCSELECT_OFFSET(i),
+		drv_data->mmio_base + LRADC_CTRL4);
+
+	spin_unlock_irqrestore(&drv_data->lock, gflags);
+
+	overspl =
+		((drv_data->ch_oversample[chan->address] ? 1 : 0)
+			* LRADC_CH_ACCUMULATE) |
+		(drv_data->ch_oversample[chan->address]
+			<<   LRADC_CH_NUM_SAMPLES_OFFSET);
+	writel(overspl, drv_data->mmio_base  + LRADC_CH(i));
+
+	/* Enable IRQ on the channel */
+	__mxs_clrl(LRADC_CTRL1_LRADC_IRQ(i),
+		drv_data->mmio_base + LRADC_CTRL1);
+	__mxs_setl(LRADC_CTRL1_LRADC_IRQ_EN(i),
+		drv_data->mmio_base + LRADC_CTRL1);
+
+	/* Set out channel to be triggers by this delay queue */
+	__mxs_setl(1<<   (LRADC_DELAY_TRIGGER_LRADCS_OFFSET + i),
+		drv_data->mmio_base + LRADC_DELAY(data->id));
+
+	return 0;
+
+err:
+	spin_unlock_irqrestore(&drv_data->lock, gflags);
+	return -EINVAL;
+}
+
+static void mxs_lradc_relinquish_channel(struct iio_dev *iio_dev,
+			const struct iio_chan_spec *chan, int chanidx)
+{
+	struct mxs_lradc_data *data = iio_priv(iio_dev);
+	struct mxs_lradc_drv_data *drv_data = data->drv_data;
+	unsigned long gflags;
+	int i;
+
+	drv_data->ch[chanidx].users--;
+	if (drv_data->ch[chanidx].users == 0) {
+		/* No more users for this channel, stop generating interrupts */
+		__mxs_setl(LRADC_CTRL1_LRADC_IRQ(i) |
+			LRADC_CTRL1_LRADC_IRQ_EN(i),
+			drv_data->mmio_base + LRADC_CTRL1);
+
+		/* Don't trigger this channel */
+		__mxs_clrl(1<<   (LRADC_DELAY_TRIGGER_LRADCS_OFFSET + i),
+			drv_data->mmio_base + LRADC_DELAY(data->id));
+
+		spin_lock_irqsave(&drv_data->lock, gflags);
+
+		/* Relinquish this channel */
+		drv_data->claimed&= ~(1<<   chan->address);
+		data->claimed&= ~(1<<   chan->address);
+		drv_data->ch[chanidx].mapping = 0;
+
+		spin_unlock_irqrestore(&drv_data->lock, gflags);
+	}
+}
+
+/*
+ * I/O operations
+ */
+static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
+			const struct iio_chan_spec *chan,
+			int *val, int *val2, long m)
+{
+	struct mxs_lradc_data *data = iio_priv(iio_dev);
+	struct mxs_lradc_drv_data *drv_data = data->drv_data;
+	unsigned long lflags;
+	int i, ret;
+
+	switch (m) {
+	case 0:
+		spin_lock_irqsave(&data->lock, lflags);
+
So we claim the channel for this IIO dev (which is one of the 4 timed
triggers).
Then wait for the next sample.
Correct

Guess that does the job, though this is
all faking
a kind of polled device whereas this will fit much better as a interrupt
driven
device outputting via buffers (probably the next step once this is
fine).
This is actually a good idea since the sampling is constantly running anyway and
it'd allow for the data to be readily available without the delay.

Note
that we typically disable this sort of access if the device is doing
interrupt driven
reading.  I have been thinking about adding a buffer implementation that
would
provide just the last value and hence allowing this sort of single
channel read, but
that requires some infrastructure that hasn't merged yet.
Heh. Maybe I should add buffer support into the driver and simply push the stuff
into the buffers. I'd need to study this.
As mentioned above, that would be easy without those pesky oversampling controls.

+		ret = mxs_lradc_claim_channel(iio_dev, chan);
+		if (ret) {
+			spin_unlock_irqrestore(&data->lock, lflags);
+			return ret;
+		}
+
+		/*
+		 * Once we are here, the channel is mapped by us already.
+		 * Find the mapping.
+		 */
+		for (i = 0; i<   8; i++) {
+			if (!(drv_data->ch[i].mapping&   MAPPING_USED))
+				continue;
+
+			if (MAPPING_CHAN(drv_data->ch[i].mapping) ==
+				chan->address)
+				break;
+		}
+
+		/* Wait until sampling is done */
+		drv_data->ch[i].wq_done = false;
+
+		drv_data->ch[i].users++;
+
+		spin_unlock_irqrestore(&data->lock, lflags);
+
+		ret = wait_event_interruptible(drv_data->ch[i].wq,
+					drv_data->ch[i].wq_done);
+		if (ret)
+			ret = -EINTR;
+		else {
+			*val = readl(drv_data->mmio_base + LRADC_CH(i))&
+					LRADC_CH_VALUE_MASK;
+			*val /= (drv_data->ch_oversample[chan->address] + 1);
+			ret = IIO_VAL_INT;
+		}
+
+		spin_lock_irqsave(&data->lock, lflags);
+
+		mxs_lradc_relinquish_channel(iio_dev, chan, i);
+
+		spin_unlock_irqrestore(&data->lock, lflags);
+
+		return ret;
+
+	case IIO_CHAN_INFO_OVERSAMPLE_COUNT:
+		*val = drv_data->ch_oversample[chan->address];
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
+static int mxs_lradc_write_raw(struct iio_dev *iio_dev,
+			const struct iio_chan_spec *chan,
+			int val, int val2, long m)
+{
+	struct mxs_lradc_data *data = iio_priv(iio_dev);
+	struct mxs_lradc_drv_data *drv_data = data->drv_data;
+
+	switch (m) {
+	case IIO_CHAN_INFO_OVERSAMPLE_COUNT:
+		if ((val<= 0) || (val>= 32))
+			return -EINVAL;
+
+		drv_data->ch_oversample[chan->address] = val - 1;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static irqreturn_t mxs_lradc_handle_irq(int irq, void *data)
+{
+	struct mxs_lradc_drv_data *drv_data = data;
+	uint32_t reg = readl(drv_data->mmio_base + LRADC_CTRL1);
+	int i;
+
+	for (i = 0; i<   8; i++)
+		if (reg&   LRADC_CTRL1_LRADC_IRQ(i)) {
+			drv_data->ch[i].wq_done = true;
+			wake_up_interruptible(&drv_data->ch[i].wq);
+		}
+
+	__mxs_clrl(0xffff, drv_data->mmio_base + LRADC_CTRL1);
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info mxs_lradc_iio_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= mxs_lradc_read_raw,
+	.write_raw		= mxs_lradc_write_raw,
+};
+
These all look good...  However, one thing you weren't to know is that
we are trying
to deprecate the IIO_CHAN macro.  It's become a bit of a maintenance
nightmare
given the underlying structure keeps getting more complex as new
requirements
turn up. Arnd warned me about this when I originally put it in, but it
took me a
while to realise how right he was!
Roger that, I can even add a much simpler one.
Either define a local macro filling just those fields that matter here or
fill the structure directly.  Also you could leave out the IIO_ST stuff
for now as it
only means anything in a driver providing buffered access via the chrdev.
Same is true of scan_index.
This is something I still don't clearly understand (and I know I'm asking for
the third time or so).

How do I do continuous sampling (aka. opening a file and reading stuff from it)?
Since if I do sysfs read, this triggers raw_read() which opens the device and
closes it. So is there some /dev/ representation of the iio device or something?
Yes. If you register buffers you get such a chrdev. But note that the data coming out is raw scans across the channels. All the IIO_ST stuff in the chan_spec and scan_index values etc are about describing to userspace what it is getting. Take a look at the example userspace code in the documentation directory. That is probably the best way of understanding the outputs.
+static const struct iio_chan_spec mxs_lradc_chan_spec[] = {
+	[0] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 0, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		0, 0, IIO_ST('u', 18, 32, 0), 0),
+	[1] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 1, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		1, 1, IIO_ST('u', 18, 32, 0), 0),
+	[2] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 2, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		2, 2, IIO_ST('u', 18, 32, 0), 0),
+	[3] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 3, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		3, 3, IIO_ST('u', 18, 32, 0), 0),
+	[4] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 4, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		4, 4, IIO_ST('u', 18, 32, 0), 0),
+	[5] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 5, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		5, 5, IIO_ST('u', 18, 32, 0), 0),
+	[6] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 6, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		6, 6, IIO_ST('u', 18, 32, 0), 0),
+	/* VBATT */
+	[7] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 7, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		7, 7, IIO_ST('u', 18, 32, 0), 0),
+	/* Temp sense 0 */
+	[8] = IIO_CHAN(IIO_TEMP, IIO_NO_MOD, 1, IIO_RAW, NULL, 8, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		8, 8, IIO_ST('u', 18, 32, 0), 0),
+	/* Temp sense 1 */
+	[9] = IIO_CHAN(IIO_TEMP, IIO_NO_MOD, 1, IIO_RAW, NULL, 9, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		9, 9, IIO_ST('u', 18, 32, 0), 0),
+	/* VDDIO */
+	[10] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 10, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		10, 10, IIO_ST('u', 18, 32, 0), 0),
+	/* VTH */
+	[11] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 11, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		11, 11, IIO_ST('u', 18, 32, 0), 0),
+	/* VDDA */
+	[12] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 12, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		12, 12, IIO_ST('u', 18, 32, 0), 0),
+	/* VDDD */
+	[13] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 13, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		13, 13, IIO_ST('u', 18, 32, 0), 0),
+	/* VBG */
+	[14] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 14, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		14, 14, IIO_ST('u', 18, 32, 0), 0),
+	/* VDD5V */
+	[15] = IIO_CHAN(IIO_VOLTAGE, IIO_NO_MOD, 1, IIO_RAW, NULL, 15, 0,
+		IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+		IIO_CHAN_INFO_OVERSAMPLE_COUNT_SEPARATE_BIT,
+		15, 15, IIO_ST('u', 18, 32, 0), 0),
+};
+
+static void mxs_lradc_config(struct mxs_lradc_drv_data *drv_data)
+{
+	/* FIXME */
+	int freq = 0x3;	/* 6MHz */
+	int onchip_ground_ref = 0;
+
+	int i;
+
+	mxs_reset_block(drv_data->mmio_base + LRADC_CTRL0);
+
+	if (onchip_ground_ref)
+		__mxs_setl(1<<   26, drv_data->mmio_base + LRADC_CTRL0);
+	else
+		__mxs_clrl(1<<   26, drv_data->mmio_base + LRADC_CTRL0);
+
+	__mxs_clrl(0x3<<   8, drv_data->mmio_base + LRADC_CTRL3);
+	__mxs_setl(freq, drv_data->mmio_base + LRADC_CTRL3);
+
+	/* The delay channels constantly retrigger themself */
+	for (i = 0; i<   4; i++)
+		__mxs_setl(LRADC_DELAY_KICK |
+			(1<<   (LRADC_DELAY_TRIGGER_DELAYS_OFFSET + i)) |
+			0x7ff,	/* FIXME */
+			drv_data->mmio_base + LRADC_DELAY(i));
+
+	/* Start temperature sensing */
+	writel(0, drv_data->mmio_base + LRADC_CTRL2);
+}
+
+/*static void mxs_lradc_config(struct mxs_lradc_pdata *pdata)
+{
+
+}
+*/
+static int __devinit mxs_lradc_probe(struct platform_device *pdev)
+{
+	struct mxs_lradc_data *data[4];
+	struct mxs_lradc_drv_data *drv_data;
+	struct iio_dev *iio;
+	struct resource *r;
+	int ret = 0;
+	int irq;
+	int i;
+
+	/*
+	 * DEVM management
+	 */
+	if (!devres_open_group(&pdev->dev, mxs_lradc_probe, GFP_KERNEL)) {
+		dev_err(&pdev->dev, "Can't open resource group\n");
+		goto err0;
+	}
+
+	drv_data = devm_kzalloc(&pdev->dev, sizeof(*drv_data), GFP_KERNEL);
+	if (!drv_data) {
+		dev_err(&pdev->dev, "Failed to allocate driver data\n");
+		ret = -ENOMEM;
+		goto err0;
+	}
+
+	spin_lock_init(&drv_data->lock);
+
+	/*
+	 * IIO ops
+	 */
+	for (i = 0; i<   4; i++) {
+		iio = iio_allocate_device(sizeof(*data));
+		if (!iio) {
+			dev_err(&pdev->dev,
+				"Failed to allocate IIO device %i\n", i);
+			ret = -ENOMEM;
+			goto err1;
+		}
+
+		iio->name = pdev->name;
You will probably want to add an id to that name to distinguish between
the difference instances.
Even better would be to have a text description of each one.

+		iio->dev.parent =&pdev->dev;
+		iio->info =&mxs_lradc_iio_info;
+		iio->modes = INDIO_DIRECT_MODE;
+		/* Channels */
+		iio->channels = mxs_lradc_chan_spec;
+		iio->num_channels = ARRAY_SIZE(mxs_lradc_chan_spec);
+		iio->attrs = mxs_lradc_attributes;
+
Given data[i] is only used in here, why not just have one of them?
These are actually four distinct private data for four distinct iio-devs.
Agreed. I was just observing that the local copy of iio_priv is only used here so you could copy each one in turn into the same local variable rather than using different elements of
a local array of pointers.

+		data[i] = iio_priv(iio);
+		data[i]->drv_data = drv_data;
+		data[i]->id = i;
+
+		spin_lock_init(&data[i]->lock);
+
+		drv_data->iio[i] = iio;
+	}
+
+	for (i = 0; i<   8; i++)
+		init_waitqueue_head(&drv_data->ch[i].wq);
+
+	dev_set_drvdata(&pdev->dev, drv_data);
+
+	/*
+	 * Allocate address space
+	 */
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (r == NULL) {
+		dev_err(&pdev->dev, "No I/O memory resource defined\n");
+		ret = -ENODEV;
+		goto err1;
+	}
+
+	r = devm_request_mem_region(&pdev->dev, r->start,
+				resource_size(r), pdev->name);
+	if (r == NULL) {
+		dev_err(&pdev->dev, "Failed to request I/O memory\n");
+		ret = -EBUSY;
+		goto err1;
+	}
+
+	drv_data->mmio_base = devm_ioremap(&pdev->dev, r->start,
resource_size(r)); +	if (!drv_data->mmio_base) {
+		dev_err(&pdev->dev, "Failed to map I/O memory\n");
+		ret = -EBUSY;
+		goto err1;
+	}
+
+	/*
+	 * Allocate IRQ
+	 */
+	for (irq = 0; irq<   8; irq++) {
+		r = platform_get_resource(pdev, IORESOURCE_IRQ, irq);
+		if (r == NULL) {
+			dev_err(&pdev->dev, "No IRQ resource defined\n");
+			ret = -ENODEV;
+			goto err1;
+		}
+
+		ret = request_irq(r->start, mxs_lradc_handle_irq, 0, r->name,
drv_data); +		if (ret) {
+			dev_err(&pdev->dev, "request_irq %i failed: %d\n", irq,
ret);
+			ret = -EBUSY;
+			goto err1;
+		}
+	}
+
+	/*
+	 * Register IIO device
+	 */
+	for (i = 0; i<   4; i++) {
+		ret = iio_device_register(drv_data->iio[i]);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to register IIO device\n");
+			ret = -EBUSY;
+			goto err2;
+		}
+	}
+
+	devres_remove_group(&pdev->dev, mxs_lradc_probe);
+
+	mxs_lradc_config(drv_data);
+
+	return 0;
+
+err2:
+	while (--i>= 0)
+
Clearing out irqs?
Missed that one with all the devm stuff, thanks :)

		iio_device_unregister(drv_data->iio[i]);

+err1:
+	for (i = 0; i<   4; i++)
+		if (drv_data->iio[i])
+			iio_free_device(drv_data->iio[i]);
+err0:
+	devres_release_group(&pdev->dev, mxs_lradc_probe);
+	return ret;
+}
+
+static int __devexit mxs_lradc_remove(struct platform_device *pdev)
+{
+	struct mxs_lradc_drv_data *drv_data = dev_get_drvdata(&pdev->dev);
+	struct resource *r;
+	int i, irq;
+
      Obvious but use ARRAY_SIZE(drv_data->iio) instead of 4.
This stupidity is not present only here. The driver is still full of arbitrary
constants and crud.

Thanks for the guidance!

M

+	for (i = 0; i<   4; i++)
+		iio_device_unregister(drv_data->iio[i]);
+
+	for (irq = 0; irq<   8; irq++) {
+		r = platform_get_resource(pdev, IORESOURCE_IRQ, irq);
+		if (r != NULL)
+			free_irq(r->start, drv_data);
+	}
+
+	for (i = 0; i<   4; i++)
+		iio_free_device(drv_data->iio[i]);
+
+	return 0;
+}
+
+static struct platform_driver mxs_lradc_driver = {
+	.driver = {
+		.name = "mxs-lradc",
+	},
+	.probe = mxs_lradc_probe,
+	.remove = __devexit_p(mxs_lradc_remove),
+};
+
+module_platform_driver(mxs_lradc_driver);
+
+MODULE_AUTHOR("Marek Vasut<marek.vasut@xxxxxxxxx>");
+MODULE_DESCRIPTION("Freescale i.MX23/i.MX28 LRADC driver");
+MODULE_LICENSE("GPL v2");

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Free Online Dating]     [Linux Kernel]     [Linux SCSI]     [XFree86]

Add to Google Powered by Linux