> 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 ? ;-)
> 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.
> 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.
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.
> > +
> > +/*
> > + * 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.
>
> > +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.
>
> > + 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?
>
> > +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.
>
> > + 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
