| Message ID | 1341368129-20468-1-git-send-email-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Marek, In message <1341368129-20468-1-git-send-email-marex@denx.de> you wrote: > This driver is very basic. It supports userland trigger, buffer and > raw access to channels. The support for delay channels is missing > altogether. ... > +#define LRADC_CH_NUM_SAMPLES_MASK (0x1f << 24) > +#define LRADC_CH_NUM_SAMPLES_OFFSET 24 #define LRADC_CH_NUM_SAMPLES_OFFSET 24 #define LRADC_CH_NUM_SAMPLES_MASK (0x1f << LRADC_CH_NUM_SAMPLES_OFFSET) > +#define LRADC_DELAY_TRIGGER_LRADCS_MASK (0xff << 24) > +#define LRADC_DELAY_TRIGGER_LRADCS_OFFSET 24 Ditto. > +#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 Ditto. Please use the symbolic names you define. Best regards, Wolfgang Denk
On 07/04/2012 04:15 AM, Marek Vasut wrote: > This driver is very basic. It supports userland trigger, buffer and > raw access to channels. The support for delay channels is missing > altogether. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jonathan Cameron <jic23@kernel.org> > Cc: Wolfgang Denk <wd@denx.de> > Cc: Juergen Beisert <jbe@pengutronix.de> > [...] The overall structure looks good, but a few things need to be addressed. I also think the driver doesn't have to go through staging and could be put in to the out-of-staging part of iio. > +struct mxs_lradc { > + struct iio_dev *iio; > + struct device *dev; > + void __iomem *base; > + int irq[13]; > + int idis; > + > + uint32_t *buffer; > + struct iio_trigger *trig; > + > + struct mutex map_lock; > + struct mxs_lradc_chan channel[LRADC_MAX_TOTAL_CHANS]; > + unsigned long slot_map; > + > + wait_queue_head_t wq_data_avail[LRADC_MAX_MAPPED_CHANS]; > + bool wq_done[LRADC_MAX_MAPPED_CHANS]; This and it's usage looks a bit like an open-coded struct completion. > +}; > + > +struct mxs_lradc_data { > + struct mxs_lradc *lradc; > +}; Is there any reason not to use the mxs_lradc struct directly has the iio data? >[...] > + > +/* > + * Channel management > + * > + * This involves crazy mapping between 8 virtual channels the CTRL4 register > + * can harbor and 16 channels total this hardware supports. > + */ I suppose this means only a maximum 8 channels can be active at a time. I've recently posted a patch which makes it easy to implement such a restriction. http://www.spinics.net/lists/linux-iio/msg05936.html and http://www.spinics.net/lists/linux-iio/msg05935.html for an example validate callback implementation. In your case you'd check for bitmap_weight(...) <= 8. Those patches are not yet in IIO though. > +static int mxs_lradc_map_channel(struct mxs_lradc *lradc, > + uint8_t channel, uint8_t flags) > +{ > + int ret; > + > + /* Invalid channel */ > + if (channel > LRADC_MAX_TOTAL_CHANS) > + return -EINVAL; > + > + mutex_lock(&lradc->map_lock); > + > + /* Channel is already mapped */ > + if (lradc->channel[channel].flags) { > + > + /* Channel is mapped to requested delay slot */ > + lradc->channel[channel].flags |= flags; > + ret = lradc->channel[channel].slot; > + goto exit; > + > + } else { /* Channel isn't mapped yet */ > + /* 8 channels already mapped, can't map any more */ > + if (bitmap_full(&lradc->slot_map, LRADC_MAX_MAPPED_CHANS)) { > + ret = -EINVAL; > + goto exit; > + } > + > + ret = find_first_zero_bit(&lradc->slot_map, > + LRADC_MAX_MAPPED_CHANS); > + > + lradc->channel[channel].slot = ret; > + lradc->channel[channel].flags |= flags; > + lradc->slot_map |= 1 << ret; > + > + writel(LRADC_CTRL4_LRADCSELECT_MASK(ret), > + lradc->base + LRADC_CTRL4 + STMP_OFFSET_REG_CLR); > + writel(channel << LRADC_CTRL4_LRADCSELECT_OFFSET(ret), > + lradc->base + LRADC_CTRL4 + STMP_OFFSET_REG_SET); > + > + writel(LRADC_CTRL1_LRADC_IRQ_EN(ret), > + lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_SET); > + > + writel(0, lradc->base + LRADC_CH(ret)); > + } > + > +exit: > + mutex_unlock(&lradc->map_lock); > + > + /* Return the slot the channel was mapped to */ > + return ret; > +} > + [...] > +/* > + * Raw 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 *lradc = data->lradc; > + int ret, slot; > + > + if (m != 0) I'd use the symbolic constant here IIO_CHAN_INFO_RAW > + return -EINVAL; > + > + /* Map channel. */ > + slot = mxs_lradc_map_channel(lradc, chan->channel, LRADC_CHAN_FLAG_RAW); > + if (slot < 0) > + return slot; > + > + /* Start sampling. */ > + mxs_lradc_run_slots(lradc, 1 << slot); > + > + /* Wait for completion on the channel, 1 second max. */ > + lradc->wq_done[slot] = false; > + ret = wait_event_interruptible_timeout(lradc->wq_data_avail[slot], > + lradc->wq_done[slot], > + msecs_to_jiffies(1000)); > + if (!ret) { > + ret = -ETIMEDOUT; > + goto err; > + } > + How well will this work if the device is currrently in buffered mode? Maybe it's better do disablow it by checking iio_buffer_enabled and return -EBUSY if it returns true; > + /* Read the data. */ > + *val = readl(lradc->base + LRADC_CH(slot)) & LRADC_CH_VALUE_MASK; > + ret = IIO_VAL_INT; > + > +err: > + /* Unmap the channel. */ > + mxs_lradc_unmap_channel(lradc, chan->channel, LRADC_CHAN_FLAG_RAW); > + > + return ret; > +} > + > +static const struct iio_info mxs_lradc_iio_info = { > + .driver_module = THIS_MODULE, > + .read_raw = mxs_lradc_read_raw, > +}; > + > +/* > + * IRQ Handling > + */ > +static irqreturn_t mxs_lradc_handle_irq(int irq, void *data) > +{ > + struct mxs_lradc *lradc = data; > + int bit; > + unsigned long reg = readl(lradc->base + LRADC_CTRL1); > + > + /* > + * Touchscreen IRQ handling code shall probably have priority > + * and therefore shall be placed here. > + */ > + > + /* Wake up each LRADC channel. */ > + for_each_set_bit(bit, ®, 8) { > + lradc->wq_done[bit] = true; > + wake_up_interruptible(&lradc->wq_data_avail[bit]); > + } > + > + if (iio_buffer_enabled(lradc->iio)) { > + disable_irq_nosync(irq); > + lradc->idis = irq; The whole disable_irq/enable_irq thing is a bit ugly, is it really necessary? > + iio_trigger_poll(lradc->iio->trig, iio_get_time_ns()); > + } > + > + writel(0x1fff, lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_CLR); > + > + return IRQ_HANDLED; > +} > + > +/* > + * Trigger handling > + */ > +static irqreturn_t mxs_lradc_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *iio = pf->indio_dev; > + struct mxs_lradc_data *data = iio_priv(iio); > + struct mxs_lradc *lradc = data->lradc; > + struct iio_buffer *buffer = iio->buffer; > + int i, j = 0, slot; > + > + for (i = 0; i < iio->masklength; i++) { > + if (!test_bit(i, iio->active_scan_mask)) > + continue; for_each_set_bit > + > + slot = lradc->channel[i].slot; > + > + lradc->buffer[j] = readl(lradc->base + LRADC_CH(slot)); > + lradc->buffer[j] &= LRADC_CH_VALUE_MASK; > + > + j++; > + } > + > + if (iio->scan_timestamp) { > + s64 *timestamp = (s64 *)((u8 *)lradc->buffer + > + ALIGN(j, sizeof(s64))); > + *timestamp = pf->timestamp; > + } > + > + iio_push_to_buffer(buffer, (u8 *)lradc->buffer, pf->timestamp); > + > + iio_trigger_notify_done(iio->trig); > + > + enable_irq(lradc->idis); > + > + return IRQ_HANDLED; > +} > + > +static int mxs_lradc_configure_trigger(struct iio_trigger *trig, bool state) > +{ > + struct iio_dev *iio = trig->private_data; > + struct mxs_lradc_data *data = iio_priv(iio); > + struct mxs_lradc *lradc = data->lradc; > + struct iio_buffer *buffer = iio->buffer; > + int slot, bit, ret = 0; > + uint8_t map = 0; > + unsigned long chans = 0; > + > + if (!state) > + goto exit; > + > + lradc->buffer = kmalloc(iio->scan_bytes, GFP_KERNEL); > + if (!lradc->buffer) > + return -ENOMEM; > + > + for_each_set_bit(bit, buffer->scan_mask, LRADC_MAX_TOTAL_CHANS) { > + slot = mxs_lradc_map_channel(lradc, bit, LRADC_CHAN_FLAG_BUF); > + > + if (slot < 0) { > + ret = -EINVAL; > + goto exit; > + } > + map |= 1 << slot; > + chans |= 1 << bit; > + } I think this should go into the buffer preenable and postdisable callbacks. > + > + mxs_lradc_run_slots(lradc, map); > + > + return 0; > + > +exit: > + for_each_set_bit(bit, &chans, LRADC_MAX_TOTAL_CHANS) > + mxs_lradc_unmap_channel(lradc, bit, LRADC_CHAN_FLAG_BUF); > + > + kfree(lradc->buffer); > + > + return ret; > +} > + > [...] > +/* > + * Buffer ops > + */ > +static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = { > + .preenable = &iio_sw_buffer_preenable, > + .postenable = &iio_triggered_buffer_postenable, > + .predisable = &iio_triggered_buffer_predisable, > +}; This is unused. > + > [...] > +static int __devinit mxs_lradc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct mxs_lradc_data *data; > + struct mxs_lradc *lradc; > + struct iio_dev *iio; > + struct resource *iores; > + int ret = 0; > + int i; > + > + lradc = devm_kzalloc(&pdev->dev, sizeof(*lradc), GFP_KERNEL); > + if (!lradc) > + return -ENOMEM; > + > + /* Grab the memory area */ > + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + lradc->base = devm_request_and_ioremap(dev, iores); > + if (!lradc->base) > + return -EADDRNOTAVAIL; I think this is a network error code, NXIO would be more approriate. [...] > + return ret; > +} > + > [...]
Dear Lars-Peter Clausen, > On 07/04/2012 04:15 AM, Marek Vasut wrote: > > This driver is very basic. It supports userland trigger, buffer and > > raw access to channels. The support for delay channels is missing > > altogether. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Jonathan Cameron <jic23@kernel.org> > > Cc: Wolfgang Denk <wd@denx.de> > > Cc: Juergen Beisert <jbe@pengutronix.de> > > [...] > > The overall structure looks good, but a few things need to be addressed. > I also think the driver doesn't have to go through staging and could be put > in to the out-of-staging part of iio. Come and rip me to shreds ;-) [...] > > + wait_queue_head_t wq_data_avail[LRADC_MAX_MAPPED_CHANS]; > > + bool wq_done[LRADC_MAX_MAPPED_CHANS]; > > This and it's usage looks a bit like an open-coded struct completion. Indeed, completion is good for this :) > > +}; > > + > > +struct mxs_lradc_data { > > + struct mxs_lradc *lradc; > > +}; > > Is there any reason not to use the mxs_lradc struct directly has the iio > data? I explained it below in the patch. I hope we'll eventually support the delay triggers, which will need 4 separate IIO devices. And this is where this structure will be augmented by other components. > >[...] > > > > + > > +/* > > + * Channel management > > + * > > + * This involves crazy mapping between 8 virtual channels the CTRL4 > > register + * can harbor and 16 channels total this hardware supports. > > + */ > > I suppose this means only a maximum 8 channels can be active at a time. > I've recently posted a patch which makes it easy to implement such a > restriction. http://www.spinics.net/lists/linux-iio/msg05936.html and > http://www.spinics.net/lists/linux-iio/msg05935.html for an example > validate callback implementation. In your case you'd check for > bitmap_weight(...) <= 8. Those patches are not yet in IIO though. This is good. When do you expect this to hit linux-next possibly? Or at least linux-iio? [..] > > +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 *lradc = data->lradc; > > + int ret, slot; > > + > > + if (m != 0) > > I'd use the symbolic constant here IIO_CHAN_INFO_RAW > > > + return -EINVAL; > > + > > + /* Map channel. */ > > + slot = mxs_lradc_map_channel(lradc, chan->channel, > > LRADC_CHAN_FLAG_RAW); + if (slot < 0) > > + return slot; > > + > > + /* Start sampling. */ > > + mxs_lradc_run_slots(lradc, 1 << slot); > > + > > + /* Wait for completion on the channel, 1 second max. */ > > + lradc->wq_done[slot] = false; > > + ret = wait_event_interruptible_timeout(lradc->wq_data_avail[slot], > > + lradc->wq_done[slot], > > + msecs_to_jiffies(1000)); > > + if (!ret) { > > + ret = -ETIMEDOUT; > > + goto err; > > + } > > + > > How well will this work if the device is currrently in buffered mode? Maybe > it's better do disablow it by checking iio_buffer_enabled and return -EBUSY > if it returns true; I believe this should work perfectly OK, why won't it ? I tried to avoid such artificial limitation. [...] > > + if (iio_buffer_enabled(lradc->iio)) { > > + disable_irq_nosync(irq); > > + lradc->idis = irq; > > The whole disable_irq/enable_irq thing is a bit ugly, is it really > necessary? I was wondering myself, I'll investigate further. This was inspired by the AT91 driver for the most part. [...] > > +static int mxs_lradc_configure_trigger(struct iio_trigger *trig, bool > > state) +{ > > + struct iio_dev *iio = trig->private_data; > > + struct mxs_lradc_data *data = iio_priv(iio); > > + struct mxs_lradc *lradc = data->lradc; > > + struct iio_buffer *buffer = iio->buffer; > > + int slot, bit, ret = 0; > > + uint8_t map = 0; > > + unsigned long chans = 0; > > + > > + if (!state) > > + goto exit; > > + > > + lradc->buffer = kmalloc(iio->scan_bytes, GFP_KERNEL); > > + if (!lradc->buffer) > > + return -ENOMEM; > > + > > + for_each_set_bit(bit, buffer->scan_mask, LRADC_MAX_TOTAL_CHANS) { > > + slot = mxs_lradc_map_channel(lradc, bit, LRADC_CHAN_FLAG_BUF); > > + > > + if (slot < 0) { > > + ret = -EINVAL; > > + goto exit; > > + } > > + map |= 1 << slot; > > + chans |= 1 << bit; > > + } > > I think this should go into the buffer preenable and postdisable callbacks. How much of it, only the slot mapping or the allocation too ? > > + > > + mxs_lradc_run_slots(lradc, map); > > + > > + return 0; > > + > > +exit: > > + for_each_set_bit(bit, &chans, LRADC_MAX_TOTAL_CHANS) > > + mxs_lradc_unmap_channel(lradc, bit, LRADC_CHAN_FLAG_BUF); > > + > > + kfree(lradc->buffer); > > + > > + return ret; > > +} > > + > > [...] > > +/* > > + * Buffer ops > > + */ > > +static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = { > > + .preenable = &iio_sw_buffer_preenable, > > + .postenable = &iio_triggered_buffer_postenable, > > + .predisable = &iio_triggered_buffer_predisable, > > +}; > > This is unused. Bah, you're right. Shame on me :-( > > [...] > > +static int __devinit mxs_lradc_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct mxs_lradc_data *data; > > + struct mxs_lradc *lradc; > > + struct iio_dev *iio; > > + struct resource *iores; > > + int ret = 0; > > + int i; > > + > > + lradc = devm_kzalloc(&pdev->dev, sizeof(*lradc), GFP_KERNEL); > > + if (!lradc) > > + return -ENOMEM; > > + > > + /* Grab the memory area */ > > + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + lradc->base = devm_request_and_ioremap(dev, iores); > > + if (!lradc->base) > > + return -EADDRNOTAVAIL; > > I think this is a network error code, NXIO would be more approriate. Will fix. Thanks for the review!
On 07/05/2012 01:48 AM, Marek Vasut wrote: > Dear Lars-Peter Clausen, > >> On 07/04/2012 04:15 AM, Marek Vasut wrote: >>> This driver is very basic. It supports userland trigger, buffer and >>> raw access to channels. The support for delay channels is missing >>> altogether. >>> >>> Signed-off-by: Marek Vasut <marex@denx.de> >>> Cc: Jonathan Cameron <jic23@kernel.org> >>> Cc: Wolfgang Denk <wd@denx.de> >>> Cc: Juergen Beisert <jbe@pengutronix.de> >>> [...] >> >> The overall structure looks good, but a few things need to be addressed. >> I also think the driver doesn't have to go through staging and could be put >> in to the out-of-staging part of iio. > > Come and rip me to shreds ;-) > > [...] > >>> + wait_queue_head_t wq_data_avail[LRADC_MAX_MAPPED_CHANS]; >>> + bool wq_done[LRADC_MAX_MAPPED_CHANS]; >> >> This and it's usage looks a bit like an open-coded struct completion. > > Indeed, completion is good for this :) > >>> +}; >>> + >>> +struct mxs_lradc_data { >>> + struct mxs_lradc *lradc; >>> +}; >> >> Is there any reason not to use the mxs_lradc struct directly has the iio >> data? > > I explained it below in the patch. I hope we'll eventually support the delay > triggers, which will need 4 separate IIO devices. And this is where this > structure will be augmented by other components. Ok I saw the comment, but it wasn't obvious to me that delay channels will require multiple IIO devices. Might make sense to state this explicitly. > >>> [...] >>> >>> + >>> +/* >>> + * Channel management >>> + * >>> + * This involves crazy mapping between 8 virtual channels the CTRL4 >>> register + * can harbor and 16 channels total this hardware supports. >>> + */ >> >> I suppose this means only a maximum 8 channels can be active at a time. >> I've recently posted a patch which makes it easy to implement such a >> restriction. http://www.spinics.net/lists/linux-iio/msg05936.html and >> http://www.spinics.net/lists/linux-iio/msg05935.html for an example >> validate callback implementation. In your case you'd check for >> bitmap_weight(...) <= 8. Those patches are not yet in IIO though. > > This is good. When do you expect this to hit linux-next possibly? Or at least > linux-iio? > soon, hopefully. > [..] > >>> +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 *lradc = data->lradc; >>> + int ret, slot; >>> + >>> + if (m != 0) >> >> I'd use the symbolic constant here IIO_CHAN_INFO_RAW >> >>> + return -EINVAL; >>> + >>> + /* Map channel. */ >>> + slot = mxs_lradc_map_channel(lradc, chan->channel, >>> LRADC_CHAN_FLAG_RAW); + if (slot < 0) >>> + return slot; >>> + >>> + /* Start sampling. */ >>> + mxs_lradc_run_slots(lradc, 1 << slot); >>> + >>> + /* Wait for completion on the channel, 1 second max. */ >>> + lradc->wq_done[slot] = false; >>> + ret = wait_event_interruptible_timeout(lradc->wq_data_avail[slot], >>> + lradc->wq_done[slot], >>> + msecs_to_jiffies(1000)); >>> + if (!ret) { Just noticed this, wait_event_interruptible_timeout can return an error value, e.g. if it has been interrupted. >>> + ret = -ETIMEDOUT; >>> + goto err; >>> + } >>> + >> >> How well will this work if the device is currrently in buffered mode? Maybe >> it's better do disablow it by checking iio_buffer_enabled and return -EBUSY >> if it returns true; > > I believe this should work perfectly OK, why won't it ? I tried to avoid such > artificial limitation. > I suppose it is fine, but not knowing the hardware, what does mxs_lradc_run_slots do exactly? > [...] > >>> +static int mxs_lradc_configure_trigger(struct iio_trigger *trig, bool >>> state) +{ >>> + struct iio_dev *iio = trig->private_data; >>> + struct mxs_lradc_data *data = iio_priv(iio); >>> + struct mxs_lradc *lradc = data->lradc; >>> + struct iio_buffer *buffer = iio->buffer; >>> + int slot, bit, ret = 0; >>> + uint8_t map = 0; >>> + unsigned long chans = 0; >>> + >>> + if (!state) >>> + goto exit; >>> + >>> + lradc->buffer = kmalloc(iio->scan_bytes, GFP_KERNEL); >>> + if (!lradc->buffer) >>> + return -ENOMEM; >>> + >>> + for_each_set_bit(bit, buffer->scan_mask, LRADC_MAX_TOTAL_CHANS) { >>> + slot = mxs_lradc_map_channel(lradc, bit, LRADC_CHAN_FLAG_BUF); >>> + >>> + if (slot < 0) { >>> + ret = -EINVAL; >>> + goto exit; >>> + } >>> + map |= 1 << slot; >>> + chans |= 1 << bit; >>> + } >> >> I think this should go into the buffer preenable and postdisable callbacks. > > How much of it, only the slot mapping or the allocation too ? > Yeah I guess it is a bit tricky in this case. A good rule of thumb is to ask yourself whether the driver will (hypothetically) still work if another trigger is used. So the buffer allocation should certainly be handled by the buffer code, either the prenable or the update_scan_mode callback. The channel mapping part is not so obvious, but I'd put it in the preenable/postdisable callbacks as well.
Dear Lars-Peter Clausen, > On 07/05/2012 01:48 AM, Marek Vasut wrote: > > Dear Lars-Peter Clausen, > > > >> On 07/04/2012 04:15 AM, Marek Vasut wrote: > >>> This driver is very basic. It supports userland trigger, buffer and > >>> raw access to channels. The support for delay channels is missing > >>> altogether. > >>> > >>> Signed-off-by: Marek Vasut <marex@denx.de> > >>> Cc: Jonathan Cameron <jic23@kernel.org> > >>> Cc: Wolfgang Denk <wd@denx.de> > >>> Cc: Juergen Beisert <jbe@pengutronix.de> > >>> [...] > >> > >> The overall structure looks good, but a few things need to be addressed. > >> I also think the driver doesn't have to go through staging and could be > >> put in to the out-of-staging part of iio. > > > > Come and rip me to shreds ;-) > > > > [...] > > > >>> + wait_queue_head_t wq_data_avail[LRADC_MAX_MAPPED_CHANS]; > >>> + bool wq_done[LRADC_MAX_MAPPED_CHANS]; > >> > >> This and it's usage looks a bit like an open-coded struct completion. > > > > Indeed, completion is good for this :) > > > >>> +}; > >>> + > >>> +struct mxs_lradc_data { > >>> + struct mxs_lradc *lradc; > >>> +}; > >> > >> Is there any reason not to use the mxs_lradc struct directly has the iio > >> data? > > > > I explained it below in the patch. I hope we'll eventually support the > > delay triggers, which will need 4 separate IIO devices. And this is > > where this structure will be augmented by other components. > > Ok I saw the comment, but it wasn't obvious to me that delay channels will > require multiple IIO devices. Might make sense to state this explicitly. Certainly. We had a discussion with jonathan about that, it's not completely settled. Maybe I'll just do it your way. afterall. > >>> [...] > >>> > >>> + > >>> +/* > >>> + * Channel management > >>> + * > >>> + * This involves crazy mapping between 8 virtual channels the CTRL4 > >>> register + * can harbor and 16 channels total this hardware supports. > >>> + */ > >> > >> I suppose this means only a maximum 8 channels can be active at a time. > >> I've recently posted a patch which makes it easy to implement such a > >> restriction. http://www.spinics.net/lists/linux-iio/msg05936.html and > >> http://www.spinics.net/lists/linux-iio/msg05935.html for an example > >> validate callback implementation. In your case you'd check for > >> bitmap_weight(...) <= 8. Those patches are not yet in IIO though. > > > > This is good. When do you expect this to hit linux-next possibly? Or at > > least linux-iio? > > soon, hopefully. > > > [..] > > > >>> +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 *lradc = data->lradc; > >>> + int ret, slot; > >>> + > >>> + if (m != 0) > >> > >> I'd use the symbolic constant here IIO_CHAN_INFO_RAW > >> > >>> + return -EINVAL; > >>> + > >>> + /* Map channel. */ > >>> + slot = mxs_lradc_map_channel(lradc, chan->channel, > >>> LRADC_CHAN_FLAG_RAW); + if (slot < 0) > >>> + return slot; > >>> + > >>> + /* Start sampling. */ > >>> + mxs_lradc_run_slots(lradc, 1 << slot); > >>> + > >>> + /* Wait for completion on the channel, 1 second max. */ > >>> + lradc->wq_done[slot] = false; > >>> + ret = wait_event_interruptible_timeout(lradc->wq_data_avail[slot], > >>> + lradc->wq_done[slot], > >>> + msecs_to_jiffies(1000)); > >>> + if (!ret) { > > Just noticed this, wait_event_interruptible_timeout can return an error > value, e.g. if it has been interrupted. I replaced this with killable completion. > >>> + ret = -ETIMEDOUT; > >>> + goto err; > >>> + } > >>> + > >> > >> How well will this work if the device is currrently in buffered mode? > >> Maybe it's better do disablow it by checking iio_buffer_enabled and > >> return -EBUSY if it returns true; > > > > I believe this should work perfectly OK, why won't it ? I tried to avoid > > such artificial limitation. > > I suppose it is fine, but not knowing the hardware, what does > mxs_lradc_run_slots do exactly? Starts the sampling of the channels that are specific in the mask passed to this function. But they must be mapped first. > > [...] > > > >>> +static int mxs_lradc_configure_trigger(struct iio_trigger *trig, bool > >>> state) +{ > >>> + struct iio_dev *iio = trig->private_data; > >>> + struct mxs_lradc_data *data = iio_priv(iio); > >>> + struct mxs_lradc *lradc = data->lradc; > >>> + struct iio_buffer *buffer = iio->buffer; > >>> + int slot, bit, ret = 0; > >>> + uint8_t map = 0; > >>> + unsigned long chans = 0; > >>> + > >>> + if (!state) > >>> + goto exit; > >>> + > >>> + lradc->buffer = kmalloc(iio->scan_bytes, GFP_KERNEL); > >>> + if (!lradc->buffer) > >>> + return -ENOMEM; > >>> + > >>> + for_each_set_bit(bit, buffer->scan_mask, LRADC_MAX_TOTAL_CHANS) { > >>> + slot = mxs_lradc_map_channel(lradc, bit, LRADC_CHAN_FLAG_BUF); > >>> + > >>> + if (slot < 0) { > >>> + ret = -EINVAL; > >>> + goto exit; > >>> + } > >>> + map |= 1 << slot; > >>> + chans |= 1 << bit; > >>> + } > >> > >> I think this should go into the buffer preenable and postdisable > >> callbacks. > > > > How much of it, only the slot mapping or the allocation too ? > > Yeah I guess it is a bit tricky in this case. A good rule of thumb is to > ask yourself whether the driver will (hypothetically) still work if > another trigger is used. So the buffer allocation should certainly be > handled by the buffer code, either the prenable or the update_scan_mode > callback. The channel mapping part is not so obvious, but I'd put it in > the > preenable/postdisable callbacks as well. All right, I'll look into this in a bit :) Best regards, Marek Vasut
Hi Marek, Marek Vasut wrote: > This driver is very basic. It supports userland trigger, buffer and > raw access to channels. The support for delay channels is missing > altogether. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jonathan Cameron <jic23@kernel.org> > Cc: Wolfgang Denk <wd@denx.de> > Cc: Juergen Beisert <jbe@pengutronix.de> > --- > drivers/staging/iio/adc/Kconfig | 12 + > drivers/staging/iio/adc/Makefile | 1 + > drivers/staging/iio/adc/mxs-lradc.c | 619 +++++++++++++++++++++++++++++++++++ > 3 files changed, 632 insertions(+) > create mode 100644 drivers/staging/iio/adc/mxs-lradc.c > > [...] sorry for the delay, too much work at the same time. When I try to compile your code I get: drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error: linux/iio/triggered_buffer.h: No such file or directory Regards, Juergen
On 07/09/2012 11:19 AM, Juergen Beisert wrote: > Hi Marek, > > Marek Vasut wrote: >> This driver is very basic. It supports userland trigger, buffer and >> raw access to channels. The support for delay channels is missing >> altogether. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Jonathan Cameron <jic23@kernel.org> >> Cc: Wolfgang Denk <wd@denx.de> >> Cc: Juergen Beisert <jbe@pengutronix.de> >> --- >> drivers/staging/iio/adc/Kconfig | 12 + >> drivers/staging/iio/adc/Makefile | 1 + >> drivers/staging/iio/adc/mxs-lradc.c | 619 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 632 insertions(+) >> create mode 100644 drivers/staging/iio/adc/mxs-lradc.c >> >> [...] > > sorry for the delay, too much work at the same time. When I try to compile > your code I get: > > drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error: linux/iio/triggered_buffer.h: No such file or directory > This file only got added recently, right now it is only in staging/staging-next - Lars
Dear Juergen Beisert, > Hi Marek, > > Marek Vasut wrote: > > This driver is very basic. It supports userland trigger, buffer and > > raw access to channels. The support for delay channels is missing > > altogether. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Jonathan Cameron <jic23@kernel.org> > > Cc: Wolfgang Denk <wd@denx.de> > > Cc: Juergen Beisert <jbe@pengutronix.de> > > --- > > > > drivers/staging/iio/adc/Kconfig | 12 + > > drivers/staging/iio/adc/Makefile | 1 + > > drivers/staging/iio/adc/mxs-lradc.c | 619 > > +++++++++++++++++++++++++++++++++++ 3 files changed, 632 insertions(+) > > create mode 100644 drivers/staging/iio/adc/mxs-lradc.c > > > > [...] > > sorry for the delay, too much work at the same time. You tell me. > When I try to compile > your code I get: > > drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error: > linux/iio/triggered_buffer.h: No such file or directory You need this patches: iio:kfifo_buf Take advantage of the fixed record size used in IIO iio: kfifo - add poll support. And use latest -next. > > Regards, > Juergen Best regards, Marek Vasut
Hi Marek, > > When I try to compile > > your code I get: > > > > drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error: > > linux/iio/triggered_buffer.h: No such file or directory > > You need this patches: > iio:kfifo_buf Take advantage of the fixed record size used in IIO > iio: kfifo - add poll support. > > And use latest -next. Thanks for the hints. Now it compiles and the driver seems to work. One thing I do not understand: It does not matter what channel I read ('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts up. Intended? Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19 20 21 22 23 24 25>" to the corresponding device tree entry? Regards, Juergen
Dear Juergen Beisert, > Hi Marek, > > > > When I try to compile > > > your code I get: > > > > > > drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error: > > > linux/iio/triggered_buffer.h: No such file or directory > > > > You need this patches: > > iio:kfifo_buf Take advantage of the fixed record size used in IIO > > iio: kfifo - add poll support. > > > > And use latest -next. > > Thanks for the hints. Now it compiles and the driver seems to work. > > One thing I do not understand: It does not matter what channel I read > ('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts up. > Intended? > Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19 20 21 > 22 23 24 25>" to the corresponding device tree entry? They're wrong lradc@80050000 { compatible = "fsl,imx28-lradc"; reg = <0x80050000 2000>; interrupts = <10 14 15 16 17 18 19 20 21 22 23 24 25>; status = "disabled"; }; > Regards, > Juergen Best regards, Marek Vasut
Hi Marek, Marek Vasut wrote: > > > > When I try to compile > > > > your code I get: > > > > > > > > drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error: > > > > linux/iio/triggered_buffer.h: No such file or directory > > > > > > You need this patches: > > > iio:kfifo_buf Take advantage of the fixed record size used in IIO > > > iio: kfifo - add poll support. > > > > > > And use latest -next. > > > > Thanks for the hints. Now it compiles and the driver seems to work. > > > > One thing I do not understand: It does not matter what channel I read > > ('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts up. > > Intended? > > Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19 20 > > 21 22 23 24 25>" to the corresponding device tree entry? > > They're wrong > > lradc@80050000 { > compatible = "fsl,imx28-lradc"; > reg = <0x80050000 2000>; > interrupts = <10 14 15 16 17 18 19 > 20 21 22 23 24 25>; > status = "disabled"; > }; Ups, thanks. But still the same behaviour: $ cat /proc/interrupts [...] 10: 0 - mxs-lradc-touchscreen 14: 0 - mxs-lradc-thresh0 15: 0 - mxs-lradc-thresh1 16: 0 - mxs-lradc-channel0 17: 0 - mxs-lradc-channel1 18: 0 - mxs-lradc-channel2 19: 0 - mxs-lradc-channel3 20: 0 - mxs-lradc-channel4 21: 0 - mxs-lradc-channel5 22: 0 - mxs-lradc-channel6 23: 0 - mxs-lradc-channel7 24: 0 - mxs-lradc-button0 25: 0 - mxs-lradc-button1 [...] $ cat in_voltage0_raw 524 $ cat in_voltage1_raw 96 $ cat in_voltage2_raw 1261 $ cat /proc/interrupts [...] 10: 0 - mxs-lradc-touchscreen 14: 0 - mxs-lradc-thresh0 15: 0 - mxs-lradc-thresh1 16: 3 - mxs-lradc-channel0 17: 0 - mxs-lradc-channel1 18: 0 - mxs-lradc-channel2 19: 0 - mxs-lradc-channel3 20: 0 - mxs-lradc-channel4 21: 0 - mxs-lradc-channel5 22: 0 - mxs-lradc-channel6 23: 0 - mxs-lradc-channel7 24: 0 - mxs-lradc-button0 25: 0 - mxs-lradc-button1 [...] Intended in this way? Regards, Juergen
Dear Juergen Beisert, > Hi Marek, > > Marek Vasut wrote: > > > > > When I try to compile > > > > > your code I get: > > > > > > > > > > drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error: > > > > > linux/iio/triggered_buffer.h: No such file or directory > > > > > > > > You need this patches: > > > > iio:kfifo_buf Take advantage of the fixed record size used in IIO > > > > iio: kfifo - add poll support. > > > > > > > > And use latest -next. > > > > > > Thanks for the hints. Now it compiles and the driver seems to work. > > > > > > One thing I do not understand: It does not matter what channel I read > > > ('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts > > > up. Intended? > > > Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19 > > > 20 21 22 23 24 25>" to the corresponding device tree entry? > > > > They're wrong > > > > lradc@80050000 { > > > > compatible = "fsl,imx28-lradc"; > > reg = <0x80050000 2000>; > > interrupts = <10 14 15 16 17 18 19 > > > > 20 21 22 23 24 25>; > > > > status = "disabled"; > > > > }; > > Ups, thanks. But still the same behaviour: > > $ cat /proc/interrupts > [...] > 10: 0 - mxs-lradc-touchscreen > 14: 0 - mxs-lradc-thresh0 > 15: 0 - mxs-lradc-thresh1 > 16: 0 - mxs-lradc-channel0 > 17: 0 - mxs-lradc-channel1 > 18: 0 - mxs-lradc-channel2 > 19: 0 - mxs-lradc-channel3 > 20: 0 - mxs-lradc-channel4 > 21: 0 - mxs-lradc-channel5 > 22: 0 - mxs-lradc-channel6 > 23: 0 - mxs-lradc-channel7 > 24: 0 - mxs-lradc-button0 > 25: 0 - mxs-lradc-button1 > [...] > > $ cat in_voltage0_raw > 524 > $ cat in_voltage1_raw > 96 > $ cat in_voltage2_raw > 1261 > > $ cat /proc/interrupts > [...] > 10: 0 - mxs-lradc-touchscreen > 14: 0 - mxs-lradc-thresh0 > 15: 0 - mxs-lradc-thresh1 > 16: 3 - mxs-lradc-channel0 > 17: 0 - mxs-lradc-channel1 > 18: 0 - mxs-lradc-channel2 > 19: 0 - mxs-lradc-channel3 > 20: 0 - mxs-lradc-channel4 > 21: 0 - mxs-lradc-channel5 > 22: 0 - mxs-lradc-channel6 > 23: 0 - mxs-lradc-channel7 > 24: 0 - mxs-lradc-button0 > 25: 0 - mxs-lradc-button1 > [...] > > Intended in this way? But wait, you're getting interrupts on channel 0. Doesn't seem quite right. Did you happen to poke into the code and see where the issue might be? > Regards, > Juergen Best regards, Marek Vasut
Marek Vasut wrote: > Dear Juergen Beisert, > > > Hi Marek, > > > > Marek Vasut wrote: > > > > > > When I try to compile > > > > > > your code I get: > > > > > > > > > > > > drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error: > > > > > > linux/iio/triggered_buffer.h: No such file or directory > > > > > > > > > > You need this patches: > > > > > iio:kfifo_buf Take advantage of the fixed record size used in > > > > > IIO iio: kfifo - add poll support. > > > > > > > > > > And use latest -next. > > > > > > > > Thanks for the hints. Now it compiles and the driver seems to work. > > > > > > > > One thing I do not understand: It does not matter what channel I read > > > > ('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts > > > > up. Intended? > > > > Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19 > > > > 20 21 22 23 24 25>" to the corresponding device tree entry? > > > > > > They're wrong > > > > > > lradc@80050000 { > > > > > > compatible = "fsl,imx28-lradc"; > > > reg = <0x80050000 2000>; > > > interrupts = <10 14 15 16 17 18 19 > > > > > > 20 21 22 23 24 25>; > > > > > > status = "disabled"; > > > > > > }; > > > > Ups, thanks. But still the same behaviour: > > > > $ cat /proc/interrupts > > [...] > > 10: 0 - mxs-lradc-touchscreen > > 14: 0 - mxs-lradc-thresh0 > > 15: 0 - mxs-lradc-thresh1 > > 16: 0 - mxs-lradc-channel0 > > 17: 0 - mxs-lradc-channel1 > > 18: 0 - mxs-lradc-channel2 > > 19: 0 - mxs-lradc-channel3 > > 20: 0 - mxs-lradc-channel4 > > 21: 0 - mxs-lradc-channel5 > > 22: 0 - mxs-lradc-channel6 > > 23: 0 - mxs-lradc-channel7 > > 24: 0 - mxs-lradc-button0 > > 25: 0 - mxs-lradc-button1 > > [...] > > > > $ cat in_voltage0_raw > > 524 > > $ cat in_voltage1_raw > > 96 > > $ cat in_voltage2_raw > > 1261 > > > > $ cat /proc/interrupts > > [...] > > 10: 0 - mxs-lradc-touchscreen > > 14: 0 - mxs-lradc-thresh0 > > 15: 0 - mxs-lradc-thresh1 > > 16: 3 - mxs-lradc-channel0 > > 17: 0 - mxs-lradc-channel1 > > 18: 0 - mxs-lradc-channel2 > > 19: 0 - mxs-lradc-channel3 > > 20: 0 - mxs-lradc-channel4 > > 21: 0 - mxs-lradc-channel5 > > 22: 0 - mxs-lradc-channel6 > > 23: 0 - mxs-lradc-channel7 > > 24: 0 - mxs-lradc-button0 > > 25: 0 - mxs-lradc-button1 > > [...] > > > > Intended in this way? > > But wait, you're getting interrupts on channel 0. Doesn't seem quite right. > Did you happen to poke into the code and see where the issue might be? No yet. But if you think this is not the intended behaviour of your driver I will do a deeper look. Regards, Juergen
On 07/10/2012 12:26 PM, Juergen Beisert wrote: > Marek Vasut wrote: >> Dear Juergen Beisert, >> >>> Hi Marek, >>> >>> Marek Vasut wrote: >>>>>>> When I try to compile >>>>>>> your code I get: >>>>>>> >>>>>>> drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error: >>>>>>> linux/iio/triggered_buffer.h: No such file or directory >>>>>> >>>>>> You need this patches: >>>>>> iio:kfifo_buf Take advantage of the fixed record size used in >>>>>> IIO iio: kfifo - add poll support. >>>>>> >>>>>> And use latest -next. >>>>> >>>>> Thanks for the hints. Now it compiles and the driver seems to work. >>>>> >>>>> One thing I do not understand: It does not matter what channel I read >>>>> ('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts >>>>> up. Intended? >>>>> Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19 >>>>> 20 21 22 23 24 25>" to the corresponding device tree entry? >>>> >>>> They're wrong >>>> >>>> lradc@80050000 { >>>> >>>> compatible = "fsl,imx28-lradc"; >>>> reg = <0x80050000 2000>; >>>> interrupts = <10 14 15 16 17 18 19 >>>> >>>> 20 21 22 23 24 25>; >>>> >>>> status = "disabled"; >>>> >>>> }; >>> >>> Ups, thanks. But still the same behaviour: >>> >>> $ cat /proc/interrupts >>> [...] >>> 10: 0 - mxs-lradc-touchscreen >>> 14: 0 - mxs-lradc-thresh0 >>> 15: 0 - mxs-lradc-thresh1 >>> 16: 0 - mxs-lradc-channel0 >>> 17: 0 - mxs-lradc-channel1 >>> 18: 0 - mxs-lradc-channel2 >>> 19: 0 - mxs-lradc-channel3 >>> 20: 0 - mxs-lradc-channel4 >>> 21: 0 - mxs-lradc-channel5 >>> 22: 0 - mxs-lradc-channel6 >>> 23: 0 - mxs-lradc-channel7 >>> 24: 0 - mxs-lradc-button0 >>> 25: 0 - mxs-lradc-button1 >>> [...] >>> >>> $ cat in_voltage0_raw >>> 524 >>> $ cat in_voltage1_raw >>> 96 >>> $ cat in_voltage2_raw >>> 1261 >>> >>> $ cat /proc/interrupts >>> [...] >>> 10: 0 - mxs-lradc-touchscreen >>> 14: 0 - mxs-lradc-thresh0 >>> 15: 0 - mxs-lradc-thresh1 >>> 16: 3 - mxs-lradc-channel0 >>> 17: 0 - mxs-lradc-channel1 >>> 18: 0 - mxs-lradc-channel2 >>> 19: 0 - mxs-lradc-channel3 >>> 20: 0 - mxs-lradc-channel4 >>> 21: 0 - mxs-lradc-channel5 >>> 22: 0 - mxs-lradc-channel6 >>> 23: 0 - mxs-lradc-channel7 >>> 24: 0 - mxs-lradc-button0 >>> 25: 0 - mxs-lradc-button1 >>> [...] >>> >>> Intended in this way? >> >> But wait, you're getting interrupts on channel 0. Doesn't seem quite right. >> Did you happen to poke into the code and see where the issue might be? > > No yet. But if you think this is not the intended behaviour of your driver I > will do a deeper look. > I don't know the hardware, but from what I've seen in the driver this behavior looks correct. When reading a physical channel the channel will be mapped to the first free virtual channel, afterward it is unmapped again. So if you are only reading a single channel it will always be mapped to first virtual channel. If you'd be using buffered mode and read multiple channels at once you'll probably get multiple interrupts on different virtual channels. - Lars
Lars-Peter Clausen wrote: > On 07/10/2012 12:26 PM, Juergen Beisert wrote: > > Marek Vasut wrote: > >> Dear Juergen Beisert, > >> > >>> Hi Marek, > >>> > >>> Marek Vasut wrote: > >>>>>>> When I try to compile > >>>>>>> your code I get: > >>>>>>> > >>>>>>> drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error: > >>>>>>> linux/iio/triggered_buffer.h: No such file or directory > >>>>>> > >>>>>> You need this patches: > >>>>>> iio:kfifo_buf Take advantage of the fixed record size used in > >>>>>> IIO iio: kfifo - add poll support. > >>>>>> > >>>>>> And use latest -next. > >>>>> > >>>>> Thanks for the hints. Now it compiles and the driver seems to work. > >>>>> > >>>>> One thing I do not understand: It does not matter what channel I read > >>>>> ('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts > >>>>> up. Intended? > >>>>> Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19 > >>>>> 20 21 22 23 24 25>" to the corresponding device tree entry? > >>>> > >>>> They're wrong > >>>> > >>>> lradc@80050000 { > >>>> > >>>> compatible = "fsl,imx28-lradc"; > >>>> reg = <0x80050000 2000>; > >>>> interrupts = <10 14 15 16 17 18 19 > >>>> > >>>> 20 21 22 23 24 25>; > >>>> > >>>> status = "disabled"; > >>>> > >>>> }; > >>> > >>> Ups, thanks. But still the same behaviour: > >>> > >>> $ cat /proc/interrupts > >>> [...] > >>> 10: 0 - mxs-lradc-touchscreen > >>> 14: 0 - mxs-lradc-thresh0 > >>> 15: 0 - mxs-lradc-thresh1 > >>> 16: 0 - mxs-lradc-channel0 > >>> 17: 0 - mxs-lradc-channel1 > >>> 18: 0 - mxs-lradc-channel2 > >>> 19: 0 - mxs-lradc-channel3 > >>> 20: 0 - mxs-lradc-channel4 > >>> 21: 0 - mxs-lradc-channel5 > >>> 22: 0 - mxs-lradc-channel6 > >>> 23: 0 - mxs-lradc-channel7 > >>> 24: 0 - mxs-lradc-button0 > >>> 25: 0 - mxs-lradc-button1 > >>> [...] > >>> > >>> $ cat in_voltage0_raw > >>> 524 > >>> $ cat in_voltage1_raw > >>> 96 > >>> $ cat in_voltage2_raw > >>> 1261 > >>> > >>> $ cat /proc/interrupts > >>> [...] > >>> 10: 0 - mxs-lradc-touchscreen > >>> 14: 0 - mxs-lradc-thresh0 > >>> 15: 0 - mxs-lradc-thresh1 > >>> 16: 3 - mxs-lradc-channel0 > >>> 17: 0 - mxs-lradc-channel1 > >>> 18: 0 - mxs-lradc-channel2 > >>> 19: 0 - mxs-lradc-channel3 > >>> 20: 0 - mxs-lradc-channel4 > >>> 21: 0 - mxs-lradc-channel5 > >>> 22: 0 - mxs-lradc-channel6 > >>> 23: 0 - mxs-lradc-channel7 > >>> 24: 0 - mxs-lradc-button0 > >>> 25: 0 - mxs-lradc-button1 > >>> [...] > >>> > >>> Intended in this way? > >> > >> But wait, you're getting interrupts on channel 0. Doesn't seem quite > >> right. Did you happen to poke into the code and see where the issue > >> might be? > > > > No yet. But if you think this is not the intended behaviour of your > > driver I will do a deeper look. > > I don't know the hardware, but from what I've seen in the driver this > behavior looks correct. When reading a physical channel the channel will be > mapped to the first free virtual channel, afterward it is unmapped again. Makes sense. > So if you are only reading a single channel it will always be mapped to > first virtual channel. If you'd be using buffered mode and read multiple > channels at once you'll probably get multiple interrupts on different > virtual channels. So, the driver's behaviour is intended. Sorry for the noise. Regards, Juergen
Dear Juergen Beisert, [...] > > >> But wait, you're getting interrupts on channel 0. Doesn't seem quite > > >> right. Did you happen to poke into the code and see where the issue > > >> might be? > > > > > > No yet. But if you think this is not the intended behaviour of your > > > driver I will do a deeper look. > > > > I don't know the hardware, but from what I've seen in the driver this > > behavior looks correct. When reading a physical channel the channel will > > be mapped to the first free virtual channel, afterward it is unmapped > > again. > > Makes sense. > > > So if you are only reading a single channel it will always be mapped to > > first virtual channel. If you'd be using buffered mode and read multiple > > channels at once you'll probably get multiple interrupts on different > > virtual channels. > > So, the driver's behaviour is intended. Sorry for the noise. Heh, my own driver confused me. I really better get back to this thing, I even have the touchscreen part in the works. Sorry for the confusion. > Regards, > Juergen Best regards, Marek Vasut
Dear Lars-Peter Clausen, Sorry for my late reply, got busy with other crazy stuff. [...] > >> Is there any reason not to use the mxs_lradc struct directly has the iio > >> data? > > > > I explained it below in the patch. I hope we'll eventually support the > > delay triggers, which will need 4 separate IIO devices. And this is > > where this structure will be augmented by other components. > > Ok I saw the comment, but it wasn't obvious to me that delay channels will > require multiple IIO devices. Might make sense to state this explicitly. Fixed. > >>> [...] > >>> > >>> + > >>> +/* > >>> + * Channel management > >>> + * > >>> + * This involves crazy mapping between 8 virtual channels the CTRL4 > >>> register + * can harbor and 16 channels total this hardware supports. > >>> + */ > >> > >> I suppose this means only a maximum 8 channels can be active at a time. > >> I've recently posted a patch which makes it easy to implement such a > >> restriction. http://www.spinics.net/lists/linux-iio/msg05936.html and > >> http://www.spinics.net/lists/linux-iio/msg05935.html for an example > >> validate callback implementation. In your case you'd check for > >> bitmap_weight(...) <= 8. Those patches are not yet in IIO though. > > > > This is good. When do you expect this to hit linux-next possibly? Or at > > least linux-iio? > > soon, hopefully. So I checked this, not sure how it'll help me though. > > [..] > > > >>> +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 *lradc = data->lradc; > >>> + int ret, slot; > >>> + > >>> + if (m != 0) > >> > >> I'd use the symbolic constant here IIO_CHAN_INFO_RAW > >> > >>> + return -EINVAL; > >>> + > >>> + /* Map channel. */ > >>> + slot = mxs_lradc_map_channel(lradc, chan->channel, > >>> LRADC_CHAN_FLAG_RAW); + if (slot < 0) > >>> + return slot; > >>> + > >>> + /* Start sampling. */ > >>> + mxs_lradc_run_slots(lradc, 1 << slot); > >>> + > >>> + /* Wait for completion on the channel, 1 second max. */ > >>> + lradc->wq_done[slot] = false; > >>> + ret = wait_event_interruptible_timeout(lradc->wq_data_avail[slot], > >>> + lradc->wq_done[slot], > >>> + msecs_to_jiffies(1000)); > >>> + if (!ret) { > > Just noticed this, wait_event_interruptible_timeout can return an error > value, e.g. if it has been interrupted. Fixed > >>> + ret = -ETIMEDOUT; > >>> + goto err; > >>> + } > >>> + > >> > >> How well will this work if the device is currrently in buffered mode? > >> Maybe it's better do disablow it by checking iio_buffer_enabled and > >> return -EBUSY if it returns true; > > > > I believe this should work perfectly OK, why won't it ? I tried to avoid > > such artificial limitation. > > I suppose it is fine, but not knowing the hardware, what does > mxs_lradc_run_slots do exactly? Execute the sampling on the mapped slots. > > [...] > > > >>> +static int mxs_lradc_configure_trigger(struct iio_trigger *trig, bool > >>> state) +{ > >>> + struct iio_dev *iio = trig->private_data; > >>> + struct mxs_lradc_data *data = iio_priv(iio); > >>> + struct mxs_lradc *lradc = data->lradc; > >>> + struct iio_buffer *buffer = iio->buffer; > >>> + int slot, bit, ret = 0; > >>> + uint8_t map = 0; > >>> + unsigned long chans = 0; > >>> + > >>> + if (!state) > >>> + goto exit; > >>> + > >>> + lradc->buffer = kmalloc(iio->scan_bytes, GFP_KERNEL); > >>> + if (!lradc->buffer) > >>> + return -ENOMEM; > >>> + > >>> + for_each_set_bit(bit, buffer->scan_mask, LRADC_MAX_TOTAL_CHANS) { > >>> + slot = mxs_lradc_map_channel(lradc, bit, LRADC_CHAN_FLAG_BUF); > >>> + > >>> + if (slot < 0) { > >>> + ret = -EINVAL; > >>> + goto exit; > >>> + } > >>> + map |= 1 << slot; > >>> + chans |= 1 << bit; > >>> + } > >> > >> I think this should go into the buffer preenable and postdisable > >> callbacks. > > > > How much of it, only the slot mapping or the allocation too ? > > Yeah I guess it is a bit tricky in this case. A good rule of thumb is to > ask yourself whether the driver will (hypothetically) still work if > another trigger is used. So the buffer allocation should certainly be > handled by the buffer code, either the prenable or the update_scan_mode > callback. The channel mapping part is not so obvious, but I'd put it in > the > preenable/postdisable callbacks as well. Lemme try. Best regards, Marek Vasut
On 07/19/2012 04:23 PM, Marek Vasut wrote: > Dear Lars-Peter Clausen, > > Sorry for my late reply, got busy with other crazy stuff. > > [...] > >>>> Is there any reason not to use the mxs_lradc struct directly has the iio >>>> data? >>> >>> I explained it below in the patch. I hope we'll eventually support the >>> delay triggers, which will need 4 separate IIO devices. And this is >>> where this structure will be augmented by other components. >> >> Ok I saw the comment, but it wasn't obvious to me that delay channels will >> require multiple IIO devices. Might make sense to state this explicitly. > > Fixed. > >>>>> [...] >>>>> >>>>> + >>>>> +/* >>>>> + * Channel management >>>>> + * >>>>> + * This involves crazy mapping between 8 virtual channels the CTRL4 >>>>> register + * can harbor and 16 channels total this hardware supports. >>>>> + */ >>>> >>>> I suppose this means only a maximum 8 channels can be active at a time. >>>> I've recently posted a patch which makes it easy to implement such a >>>> restriction. http://www.spinics.net/lists/linux-iio/msg05936.html and >>>> http://www.spinics.net/lists/linux-iio/msg05935.html for an example >>>> validate callback implementation. In your case you'd check for >>>> bitmap_weight(...) <= 8. Those patches are not yet in IIO though. >>> >>> This is good. When do you expect this to hit linux-next possibly? Or at >>> least linux-iio? >> >> soon, hopefully. > > So I checked this, not sure how it'll help me though. Right now with your driver you can enable any combination of channels. If more than 8 channels are enabled and you start sampling it will fail, since not all channels can be mapped. With those patch you can implement a validate callback and check for bitmap_weight(scan_mask) <= 8. This will ensure that it is not possible to select more than 8 channels at once, which again means that starting sampling can't fail of this. - Lars
Dear Lars-Peter Clausen, > On 07/19/2012 04:23 PM, Marek Vasut wrote: > > Dear Lars-Peter Clausen, > > > > Sorry for my late reply, got busy with other crazy stuff. > > > > [...] > > > >>>> Is there any reason not to use the mxs_lradc struct directly has the > >>>> iio data? > >>> > >>> I explained it below in the patch. I hope we'll eventually support the > >>> delay triggers, which will need 4 separate IIO devices. And this is > >>> where this structure will be augmented by other components. > >> > >> Ok I saw the comment, but it wasn't obvious to me that delay channels > >> will require multiple IIO devices. Might make sense to state this > >> explicitly. > > > > Fixed. > > > >>>>> [...] > >>>>> > >>>>> + > >>>>> +/* > >>>>> + * Channel management > >>>>> + * > >>>>> + * This involves crazy mapping between 8 virtual channels the CTRL4 > >>>>> register + * can harbor and 16 channels total this hardware supports. > >>>>> + */ > >>>> > >>>> I suppose this means only a maximum 8 channels can be active at a > >>>> time. I've recently posted a patch which makes it easy to implement > >>>> such a restriction. > >>>> http://www.spinics.net/lists/linux-iio/msg05936.html and > >>>> http://www.spinics.net/lists/linux-iio/msg05935.html for an example > >>>> validate callback implementation. In your case you'd check for > >>>> bitmap_weight(...) <= 8. Those patches are not yet in IIO though. > >>> > >>> This is good. When do you expect this to hit linux-next possibly? Or at > >>> least linux-iio? > >> > >> soon, hopefully. > > > > So I checked this, not sure how it'll help me though. > > Right now with your driver you can enable any combination of channels. If > more than 8 channels are enabled and you start sampling it will fail, since > not all channels can be mapped. With those patch you can implement a > validate callback and check for bitmap_weight(scan_mask) <= 8. This will > ensure that it is not possible to select more than 8 channels at once, > which again means that starting sampling can't fail of this. Thanks, got it ! Best regards, Marek Vasut
Dear Lars-Peter Clausen, > On 07/19/2012 04:23 PM, Marek Vasut wrote: > > Dear Lars-Peter Clausen, > > > > Sorry for my late reply, got busy with other crazy stuff. > > > > [...] > > > >>>> Is there any reason not to use the mxs_lradc struct directly has the > >>>> iio data? > >>> > >>> I explained it below in the patch. I hope we'll eventually support the > >>> delay triggers, which will need 4 separate IIO devices. And this is > >>> where this structure will be augmented by other components. > >> > >> Ok I saw the comment, but it wasn't obvious to me that delay channels > >> will require multiple IIO devices. Might make sense to state this > >> explicitly. > > > > Fixed. > > > >>>>> [...] > >>>>> > >>>>> + > >>>>> +/* > >>>>> + * Channel management > >>>>> + * > >>>>> + * This involves crazy mapping between 8 virtual channels the CTRL4 > >>>>> register + * can harbor and 16 channels total this hardware supports. > >>>>> + */ > >>>> > >>>> I suppose this means only a maximum 8 channels can be active at a > >>>> time. I've recently posted a patch which makes it easy to implement > >>>> such a restriction. > >>>> http://www.spinics.net/lists/linux-iio/msg05936.html and > >>>> http://www.spinics.net/lists/linux-iio/msg05935.html for an example > >>>> validate callback implementation. In your case you'd check for > >>>> bitmap_weight(...) <= 8. Those patches are not yet in IIO though. > >>> > >>> This is good. When do you expect this to hit linux-next possibly? Or at > >>> least linux-iio? > >> > >> soon, hopefully. > > > > So I checked this, not sure how it'll help me though. > > Right now with your driver you can enable any combination of channels. If > more than 8 channels are enabled and you start sampling it will fail, since > not all channels can be mapped. With those patch you can implement a > validate callback and check for bitmap_weight(scan_mask) <= 8. This will > ensure that it is not possible to select more than 8 channels at once, > which again means that starting sampling can't fail of this. Ok, I fixed this one. One last thing that I don't really understand is this. I run generic_buffer.c to source samples from the LRADC. Is there any way to continuously sample them? Because right now, I get one sample and that's it, no more samples happen later on (I can 0 data in subsequent read() call). Best regards, Marek Vasut
Dear Lars-Peter Clausen, > > On 07/19/2012 04:23 PM, Marek Vasut wrote: > > > Dear Lars-Peter Clausen, > > > > > > Sorry for my late reply, got busy with other crazy stuff. > > > > > > [...] > > > > > >>>> Is there any reason not to use the mxs_lradc struct directly has the > > >>>> iio data? > > >>> > > >>> I explained it below in the patch. I hope we'll eventually support > > >>> the delay triggers, which will need 4 separate IIO devices. And this > > >>> is where this structure will be augmented by other components. > > >> > > >> Ok I saw the comment, but it wasn't obvious to me that delay channels > > >> will require multiple IIO devices. Might make sense to state this > > >> explicitly. > > > > > > Fixed. > > > > > >>>>> [...] > > >>>>> > > >>>>> + > > >>>>> +/* > > >>>>> + * Channel management > > >>>>> + * > > >>>>> + * This involves crazy mapping between 8 virtual channels the > > >>>>> CTRL4 register + * can harbor and 16 channels total this hardware > > >>>>> supports. + */ > > >>>> > > >>>> I suppose this means only a maximum 8 channels can be active at a > > >>>> time. I've recently posted a patch which makes it easy to implement > > >>>> such a restriction. > > >>>> http://www.spinics.net/lists/linux-iio/msg05936.html and > > >>>> http://www.spinics.net/lists/linux-iio/msg05935.html for an example > > >>>> validate callback implementation. In your case you'd check for > > >>>> bitmap_weight(...) <= 8. Those patches are not yet in IIO though. > > >>> > > >>> This is good. When do you expect this to hit linux-next possibly? Or > > >>> at least linux-iio? > > >> > > >> soon, hopefully. > > > > > > So I checked this, not sure how it'll help me though. > > > > Right now with your driver you can enable any combination of channels. If > > more than 8 channels are enabled and you start sampling it will fail, > > since not all channels can be mapped. With those patch you can implement > > a validate callback and check for bitmap_weight(scan_mask) <= 8. This > > will ensure that it is not possible to select more than 8 channels at > > once, which again means that starting sampling can't fail of this. > > Ok, I fixed this one. One last thing that I don't really understand is > this. I run generic_buffer.c to source samples from the LRADC. Is there > any way to continuously sample them? Because right now, I get one sample > and that's it, no more samples happen later on (I can 0 data in subsequent > read() call). One more thing I'm curious about. There's another ADC block on the CPU, called HSADC (high-speed ADC). It can sample even up to 2Msamples/s. If I were to, say -- sample at 100kHz and be able to DMA the results into memory -- is there any way to push such results into userland somehow? Or how to operate such fast beast? Best regards, Marek Vasut
On Fri, Jul 20, 2012 at 04:18:41AM +0200, Marek Vasut wrote: > One more thing I'm curious about. There's another ADC block on the > CPU, called HSADC (high-speed ADC). It can sample even up to > 2Msamples/s. If I were to, say -- sample at 100kHz and be able to DMA > the results into memory -- is there any way to push such results into > userland somehow? Or how to operate such fast beast? As this unit is mainly intended for things like laser / barcode scanners, wouldn't v2l2 be a good abstraction for it (with one line and N pixels)? Just an idea, didn't look too closely. rsc
Dear Robert Schwebel, > On Fri, Jul 20, 2012 at 04:18:41AM +0200, Marek Vasut wrote: > > One more thing I'm curious about. There's another ADC block on the > > CPU, called HSADC (high-speed ADC). It can sample even up to > > 2Msamples/s. If I were to, say -- sample at 100kHz and be able to DMA > > the results into memory -- is there any way to push such results into > > userland somehow? Or how to operate such fast beast? > > As this unit is mainly intended for things like laser / barcode > scanners, wouldn't v2l2 be a good abstraction for it (with one line and > N pixels)? Just an idea, didn't look too closely. Considering it's not only for barcode scanners, but also some crazy sampling, I won't go for v4l2 Best regards, Marek Vasut
On 07/20/2012 04:18 AM, Marek Vasut wrote: >> Dear Lars-Peter Clausen, > [...] > > One more thing I'm curious about. There's another ADC block on the CPU, called > HSADC (high-speed ADC). It can sample even up to 2Msamples/s. If I were to, say > -- sample at 100kHz and be able to DMA the results into memory -- is there any > way to push such results into userland somehow? Or how to operate such fast > beast? Proper support for high speed sampling is still something that's missing from IIO. You can't mmap your buffers, you can't splice a IIO data stream to another file descriptor, there is no support for zero copy. So your bottleneck will become that you have to copy lots of data around. But it is certainly something that will be added at some point. So implementing the driver as a IIO driver is definitely the right direction. - Lars
On 07/19/2012 09:26 PM, Marek Vasut wrote: > Dear Lars-Peter Clausen, > >> On 07/19/2012 04:23 PM, Marek Vasut wrote: >>> Dear Lars-Peter Clausen, >>> >>> Sorry for my late reply, got busy with other crazy stuff. >>> >>> [...] >>> >>>>>> Is there any reason not to use the mxs_lradc struct directly has the >>>>>> iio data? >>>>> >>>>> I explained it below in the patch. I hope we'll eventually support the >>>>> delay triggers, which will need 4 separate IIO devices. And this is >>>>> where this structure will be augmented by other components. >>>> >>>> Ok I saw the comment, but it wasn't obvious to me that delay channels >>>> will require multiple IIO devices. Might make sense to state this >>>> explicitly. >>> >>> Fixed. >>> >>>>>>> [...] >>>>>>> >>>>>>> + >>>>>>> +/* >>>>>>> + * Channel management >>>>>>> + * >>>>>>> + * This involves crazy mapping between 8 virtual channels the CTRL4 >>>>>>> register + * can harbor and 16 channels total this hardware supports. >>>>>>> + */ >>>>>> >>>>>> I suppose this means only a maximum 8 channels can be active at a >>>>>> time. I've recently posted a patch which makes it easy to implement >>>>>> such a restriction. >>>>>> http://www.spinics.net/lists/linux-iio/msg05936.html and >>>>>> http://www.spinics.net/lists/linux-iio/msg05935.html for an example >>>>>> validate callback implementation. In your case you'd check for >>>>>> bitmap_weight(...) <= 8. Those patches are not yet in IIO though. >>>>> >>>>> This is good. When do you expect this to hit linux-next possibly? Or at >>>>> least linux-iio? >>>> >>>> soon, hopefully. >>> >>> So I checked this, not sure how it'll help me though. >> >> Right now with your driver you can enable any combination of channels. If >> more than 8 channels are enabled and you start sampling it will fail, since >> not all channels can be mapped. With those patch you can implement a >> validate callback and check for bitmap_weight(scan_mask) <= 8. This will >> ensure that it is not possible to select more than 8 channels at once, >> which again means that starting sampling can't fail of this. > > Ok, I fixed this one. One last thing that I don't really understand is this. I > run generic_buffer.c to source samples from the LRADC. Is there any way to > continuously sample them? Because right now, I get one sample and that's it, no > more samples happen later on (I can 0 data in subsequent read() call). > I'd consider that a bug in your driver ;) The intend with IIO is that once you start sampling by enabling the buffer you get a continuous data stream until sampling is stopped and this works fine with other drivers. - Lars
Dear Lars-Peter Clausen, > On 07/19/2012 09:26 PM, Marek Vasut wrote: > > Dear Lars-Peter Clausen, > > > >> On 07/19/2012 04:23 PM, Marek Vasut wrote: > >>> Dear Lars-Peter Clausen, > >>> > >>> Sorry for my late reply, got busy with other crazy stuff. > >>> > >>> [...] > >>> > >>>>>> Is there any reason not to use the mxs_lradc struct directly has the > >>>>>> iio data? > >>>>> > >>>>> I explained it below in the patch. I hope we'll eventually support > >>>>> the delay triggers, which will need 4 separate IIO devices. And this > >>>>> is where this structure will be augmented by other components. > >>>> > >>>> Ok I saw the comment, but it wasn't obvious to me that delay channels > >>>> will require multiple IIO devices. Might make sense to state this > >>>> explicitly. > >>> > >>> Fixed. > >>> > >>>>>>> [...] > >>>>>>> > >>>>>>> + > >>>>>>> +/* > >>>>>>> + * Channel management > >>>>>>> + * > >>>>>>> + * This involves crazy mapping between 8 virtual channels the > >>>>>>> CTRL4 register + * can harbor and 16 channels total this hardware > >>>>>>> supports. + */ > >>>>>> > >>>>>> I suppose this means only a maximum 8 channels can be active at a > >>>>>> time. I've recently posted a patch which makes it easy to implement > >>>>>> such a restriction. > >>>>>> http://www.spinics.net/lists/linux-iio/msg05936.html and > >>>>>> http://www.spinics.net/lists/linux-iio/msg05935.html for an example > >>>>>> validate callback implementation. In your case you'd check for > >>>>>> bitmap_weight(...) <= 8. Those patches are not yet in IIO though. > >>>>> > >>>>> This is good. When do you expect this to hit linux-next possibly? Or > >>>>> at least linux-iio? > >>>> > >>>> soon, hopefully. > >>> > >>> So I checked this, not sure how it'll help me though. > >> > >> Right now with your driver you can enable any combination of channels. > >> If more than 8 channels are enabled and you start sampling it will > >> fail, since not all channels can be mapped. With those patch you can > >> implement a validate callback and check for bitmap_weight(scan_mask) <= > >> 8. This will ensure that it is not possible to select more than 8 > >> channels at once, which again means that starting sampling can't fail > >> of this. > > > > Ok, I fixed this one. One last thing that I don't really understand is > > this. I run generic_buffer.c to source samples from the LRADC. Is there > > any way to continuously sample them? Because right now, I get one sample > > and that's it, no more samples happen later on (I can 0 data in > > subsequent read() call). > > I'd consider that a bug in your driver ;) > The intend with IIO is that once you start sampling by enabling the buffer > you get a continuous data stream until sampling is stopped and this works > fine with other drivers. Ah! Thanks, I'll poke into this. > - Lars Best regards, Marek Vasut
On 07/20/2012 03:09 PM, Lars-Peter Clausen wrote: > On 07/20/2012 04:18 AM, Marek Vasut wrote: >>> Dear Lars-Peter Clausen, > >> [...] >> >> One more thing I'm curious about. There's another ADC block on the CPU, called >> HSADC (high-speed ADC). It can sample even up to 2Msamples/s. If I were to, say >> -- sample at 100kHz and be able to DMA the results into memory -- is there any >> way to push such results into userland somehow? Or how to operate such fast >> beast? > > Proper support for high speed sampling is still something that's missing > from IIO. You can't mmap your buffers, you can't splice a IIO data stream to > another file descriptor, there is no support for zero copy. So your > bottleneck will become that you have to copy lots of data around. But it is > certainly something that will be added at some point. So implementing the > driver as a IIO driver is definitely the right direction. Lars has pretty much covered it. A long time back I had a cunning plan to wait for the tracing frameworks to agree on a one true ring buffer then leverage that work to get us everything we could want. Some of those buffers are extremely fast versatile and do fun things like splicing. Also we have very little chance of introducing yet another complex buffer implementation to the kernel (which incidentally is why we are ditching sw_ring - other than it being rubbish ;) There were various articles on lwn about this work, but I haven't heard anything recently. If anyone does want (and crucially have time) to look into high performance (and large) buffer options, that would be excellent! It is definitely something we want to have. Jonathan > > - Lars >
diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig index 67711b7..97ca697 100644 --- a/drivers/staging/iio/adc/Kconfig +++ b/drivers/staging/iio/adc/Kconfig @@ -200,6 +200,18 @@ config LPC32XX_ADC activate only one via device tree selection. Provides direct access via sysfs. +config MXS_LRADC + tristate "Freescale i.MX23/i.MX28 LRADC" + depends on ARCH_MXS + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER + 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. + config SPEAR_ADC tristate "ST SPEAr ADC" depends on PLAT_SPEAR diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile index 14e98b6..ecac9a0 100644 --- a/drivers/staging/iio/adc/Makefile +++ b/drivers/staging/iio/adc/Makefile @@ -38,4 +38,5 @@ obj-$(CONFIG_ADT7310) += adt7310.o obj-$(CONFIG_ADT7410) += adt7410.o obj-$(CONFIG_AD7280) += ad7280a.o obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o +obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o obj-$(CONFIG_SPEAR_ADC) += spear_adc.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..d4089c6 --- /dev/null +++ b/drivers/staging/iio/adc/mxs-lradc.c @@ -0,0 +1,619 @@ +/* + * Freescale i.MX233/i.MX28 LRADC driver + * + * Copyright (c) 2012 DENX Software Engineering, GmbH. + * Marek Vasut <marex@denx.de> + * + * 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; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/interrupt.h> +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/of.h> +#include <linux/of_device.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 <linux/stmp_device.h> +#include <linux/bitops.h> + +#include <mach/mxs.h> +#include <mach/common.h> + +#include <linux/iio/iio.h> +#include <linux/iio/buffer.h> +#include <linux/iio/trigger.h> +#include <linux/iio/trigger_consumer.h> +#include <linux/iio/triggered_buffer.h> + +#define DRIVER_NAME "mxs-lradc" + +#define LRADC_MAX_DELAY_CHANS 4 +#define LRADC_MAX_MAPPED_CHANS 8 +#define LRADC_MAX_TOTAL_CHANS 16 + +#define LRADC_DELAY_TIMER_HZ 2000 + +#define LRADC_CHAN_FLAG_RAW (1 << 0) +#define LRADC_CHAN_FLAG_BUF (1 << 1) + +static const char * const mxs_lradc_irq_name[] = { + "mxs-lradc-touchscreen", + "mxs-lradc-thresh0", + "mxs-lradc-thresh1", + "mxs-lradc-channel0", + "mxs-lradc-channel1", + "mxs-lradc-channel2", + "mxs-lradc-channel3", + "mxs-lradc-channel4", + "mxs-lradc-channel5", + "mxs-lradc-channel6", + "mxs-lradc-channel7", + "mxs-lradc-button0", + "mxs-lradc-button1", +}; + +struct mxs_lradc_chan { + uint8_t slot; + uint8_t flags; +}; + +struct mxs_lradc { + struct iio_dev *iio; + struct device *dev; + void __iomem *base; + int irq[13]; + int idis; + + uint32_t *buffer; + struct iio_trigger *trig; + + struct mutex map_lock; + struct mxs_lradc_chan channel[LRADC_MAX_TOTAL_CHANS]; + unsigned long slot_map; + + wait_queue_head_t wq_data_avail[LRADC_MAX_MAPPED_CHANS]; + bool wq_done[LRADC_MAX_MAPPED_CHANS]; +}; + +struct mxs_lradc_data { + struct mxs_lradc *lradc; +}; + +#define LRADC_CTRL0 0x00 +#define LRADC_CTRL0_TOUCH_DETECT_ENABLE (1 << 23) +#define LRADC_CTRL0_TOUCH_SCREEN_TYPE (1 << 22) + +#define LRADC_CTRL1 0x10 +#define LRADC_CTRL1_LRADC_IRQ(n) (1 << (n)) +#define LRADC_CTRL1_LRADC_IRQ_MASK 0x1fff +#define LRADC_CTRL1_LRADC_IRQ_EN(n) (1 << ((n) + 16)) +#define LRADC_CTRL1_LRADC_IRQ_EN_MASK (0x1fff << 16) + +#define LRADC_CTRL2 0x20 +#define LRADC_CTRL2_TEMPSENSE_PWD (1 << 15) + +#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_CTRL4 0x140 +#define LRADC_CTRL4_LRADCSELECT_MASK(n) (0xf << ((n) * 4)) +#define LRADC_CTRL4_LRADCSELECT_OFFSET(n) ((n) * 4) + +/* + * Channel management + * + * This involves crazy mapping between 8 virtual channels the CTRL4 register + * can harbor and 16 channels total this hardware supports. + */ +static int mxs_lradc_map_channel(struct mxs_lradc *lradc, + uint8_t channel, uint8_t flags) +{ + int ret; + + /* Invalid channel */ + if (channel > LRADC_MAX_TOTAL_CHANS) + return -EINVAL; + + mutex_lock(&lradc->map_lock); + + /* Channel is already mapped */ + if (lradc->channel[channel].flags) { + + /* Channel is mapped to requested delay slot */ + lradc->channel[channel].flags |= flags; + ret = lradc->channel[channel].slot; + goto exit; + + } else { /* Channel isn't mapped yet */ + /* 8 channels already mapped, can't map any more */ + if (bitmap_full(&lradc->slot_map, LRADC_MAX_MAPPED_CHANS)) { + ret = -EINVAL; + goto exit; + } + + ret = find_first_zero_bit(&lradc->slot_map, + LRADC_MAX_MAPPED_CHANS); + + lradc->channel[channel].slot = ret; + lradc->channel[channel].flags |= flags; + lradc->slot_map |= 1 << ret; + + writel(LRADC_CTRL4_LRADCSELECT_MASK(ret), + lradc->base + LRADC_CTRL4 + STMP_OFFSET_REG_CLR); + writel(channel << LRADC_CTRL4_LRADCSELECT_OFFSET(ret), + lradc->base + LRADC_CTRL4 + STMP_OFFSET_REG_SET); + + writel(LRADC_CTRL1_LRADC_IRQ_EN(ret), + lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_SET); + + writel(0, lradc->base + LRADC_CH(ret)); + } + +exit: + mutex_unlock(&lradc->map_lock); + + /* Return the slot the channel was mapped to */ + return ret; +} + +static int mxs_lradc_unmap_channel(struct mxs_lradc *lradc, + uint8_t channel, uint8_t flags) +{ + int slot; + + /* Invalid channel */ + if (channel > LRADC_MAX_TOTAL_CHANS) + return -EINVAL; + + mutex_lock(&lradc->map_lock); + + /* Channel not mapped */ + if (!lradc->channel[channel].flags) { + mutex_unlock(&lradc->map_lock); + return -EINVAL; + } + + lradc->channel[channel].flags &= ~flags; + slot = lradc->channel[channel].slot; + + /* No more users of this channel, unmap it */ + if (!lradc->channel[channel].flags) { + lradc->slot_map &= ~(1 << slot); + + writel(LRADC_CTRL1_LRADC_IRQ_EN(slot), + lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_CLR); + } + + mutex_unlock(&lradc->map_lock); + + return slot; +} + +static void mxs_lradc_run_slots(struct mxs_lradc *lradc, uint8_t slots) +{ + writel(slots, lradc->base + LRADC_CTRL0 + STMP_OFFSET_REG_SET); +} + +/* + * Raw 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 *lradc = data->lradc; + int ret, slot; + + if (m != 0) + return -EINVAL; + + /* Map channel. */ + slot = mxs_lradc_map_channel(lradc, chan->channel, LRADC_CHAN_FLAG_RAW); + if (slot < 0) + return slot; + + /* Start sampling. */ + mxs_lradc_run_slots(lradc, 1 << slot); + + /* Wait for completion on the channel, 1 second max. */ + lradc->wq_done[slot] = false; + ret = wait_event_interruptible_timeout(lradc->wq_data_avail[slot], + lradc->wq_done[slot], + msecs_to_jiffies(1000)); + if (!ret) { + ret = -ETIMEDOUT; + goto err; + } + + /* Read the data. */ + *val = readl(lradc->base + LRADC_CH(slot)) & LRADC_CH_VALUE_MASK; + ret = IIO_VAL_INT; + +err: + /* Unmap the channel. */ + mxs_lradc_unmap_channel(lradc, chan->channel, LRADC_CHAN_FLAG_RAW); + + return ret; +} + +static const struct iio_info mxs_lradc_iio_info = { + .driver_module = THIS_MODULE, + .read_raw = mxs_lradc_read_raw, +}; + +/* + * IRQ Handling + */ +static irqreturn_t mxs_lradc_handle_irq(int irq, void *data) +{ + struct mxs_lradc *lradc = data; + int bit; + unsigned long reg = readl(lradc->base + LRADC_CTRL1); + + /* + * Touchscreen IRQ handling code shall probably have priority + * and therefore shall be placed here. + */ + + /* Wake up each LRADC channel. */ + for_each_set_bit(bit, ®, 8) { + lradc->wq_done[bit] = true; + wake_up_interruptible(&lradc->wq_data_avail[bit]); + } + + if (iio_buffer_enabled(lradc->iio)) { + disable_irq_nosync(irq); + lradc->idis = irq; + iio_trigger_poll(lradc->iio->trig, iio_get_time_ns()); + } + + writel(0x1fff, lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_CLR); + + return IRQ_HANDLED; +} + +/* + * Trigger handling + */ +static irqreturn_t mxs_lradc_trigger_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *iio = pf->indio_dev; + struct mxs_lradc_data *data = iio_priv(iio); + struct mxs_lradc *lradc = data->lradc; + struct iio_buffer *buffer = iio->buffer; + int i, j = 0, slot; + + for (i = 0; i < iio->masklength; i++) { + if (!test_bit(i, iio->active_scan_mask)) + continue; + + slot = lradc->channel[i].slot; + + lradc->buffer[j] = readl(lradc->base + LRADC_CH(slot)); + lradc->buffer[j] &= LRADC_CH_VALUE_MASK; + + j++; + } + + if (iio->scan_timestamp) { + s64 *timestamp = (s64 *)((u8 *)lradc->buffer + + ALIGN(j, sizeof(s64))); + *timestamp = pf->timestamp; + } + + iio_push_to_buffer(buffer, (u8 *)lradc->buffer, pf->timestamp); + + iio_trigger_notify_done(iio->trig); + + enable_irq(lradc->idis); + + return IRQ_HANDLED; +} + +static int mxs_lradc_configure_trigger(struct iio_trigger *trig, bool state) +{ + struct iio_dev *iio = trig->private_data; + struct mxs_lradc_data *data = iio_priv(iio); + struct mxs_lradc *lradc = data->lradc; + struct iio_buffer *buffer = iio->buffer; + int slot, bit, ret = 0; + uint8_t map = 0; + unsigned long chans = 0; + + if (!state) + goto exit; + + lradc->buffer = kmalloc(iio->scan_bytes, GFP_KERNEL); + if (!lradc->buffer) + return -ENOMEM; + + for_each_set_bit(bit, buffer->scan_mask, LRADC_MAX_TOTAL_CHANS) { + slot = mxs_lradc_map_channel(lradc, bit, LRADC_CHAN_FLAG_BUF); + + if (slot < 0) { + ret = -EINVAL; + goto exit; + } + map |= 1 << slot; + chans |= 1 << bit; + } + + mxs_lradc_run_slots(lradc, map); + + return 0; + +exit: + for_each_set_bit(bit, &chans, LRADC_MAX_TOTAL_CHANS) + mxs_lradc_unmap_channel(lradc, bit, LRADC_CHAN_FLAG_BUF); + + kfree(lradc->buffer); + + return ret; +} + +static const struct iio_trigger_ops mxs_lradc_trigger_ops = { + .owner = THIS_MODULE, + .set_trigger_state = &mxs_lradc_configure_trigger, +}; + +static int mxs_lradc_trigger_init(struct iio_dev *iio) +{ + int ret; + struct iio_trigger *trig; + + trig = iio_trigger_alloc("%s-dev%i", iio->name, iio->id); + if (trig == NULL) + return -ENOMEM; + + trig->dev.parent = iio->dev.parent; + trig->private_data = iio; + trig->ops = &mxs_lradc_trigger_ops; + + ret = iio_trigger_register(trig); + if (ret) { + iio_trigger_free(trig); + return ret; + } + + iio->trig = trig; + + return 0; +} + +static void mxs_lradc_trigger_remove(struct iio_dev *iio) +{ + iio_trigger_unregister(iio->trig); + iio_trigger_free(iio->trig); +} + +/* + * Buffer ops + */ +static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = { + .preenable = &iio_sw_buffer_preenable, + .postenable = &iio_triggered_buffer_postenable, + .predisable = &iio_triggered_buffer_predisable, +}; + +/* + ***************************************************************************** + * DRIVER INIT STUFF + ***************************************************************************** + */ + +#define MXS_ADC_CHAN(idx, chan_type) { \ + .type = (chan_type), \ + .indexed = 1, \ + .scan_index = (idx), \ + .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT, \ + .channel = (idx), \ + .scan_type = { \ + .sign = 'u', \ + .realbits = 18, \ + .storagebits = 32, \ + }, \ +} + +static const struct iio_chan_spec mxs_lradc_chan_spec[] = { + MXS_ADC_CHAN(0, IIO_VOLTAGE), + MXS_ADC_CHAN(1, IIO_VOLTAGE), + MXS_ADC_CHAN(2, IIO_VOLTAGE), + MXS_ADC_CHAN(3, IIO_VOLTAGE), + MXS_ADC_CHAN(4, IIO_VOLTAGE), + MXS_ADC_CHAN(5, IIO_VOLTAGE), + MXS_ADC_CHAN(6, IIO_VOLTAGE), + MXS_ADC_CHAN(7, IIO_VOLTAGE), /* VBATT */ + MXS_ADC_CHAN(8, IIO_TEMP), /* Temp sense 0 */ + MXS_ADC_CHAN(9, IIO_TEMP), /* Temp sense 1 */ + MXS_ADC_CHAN(10, IIO_VOLTAGE), /* VDDIO */ + MXS_ADC_CHAN(11, IIO_VOLTAGE), /* VTH */ + MXS_ADC_CHAN(12, IIO_VOLTAGE), /* VDDA */ + MXS_ADC_CHAN(13, IIO_VOLTAGE), /* VDDD */ + MXS_ADC_CHAN(14, IIO_VOLTAGE), /* VBG */ + MXS_ADC_CHAN(15, IIO_VOLTAGE), /* VDD5V */ +}; + +static void mxs_lradc_hw_init(struct mxs_lradc *lradc) +{ + int i; + + stmp_reset_block(lradc->base); + + for (i = 0; i < LRADC_MAX_DELAY_CHANS; i++) + writel(0, lradc->base + LRADC_DELAY(i)); + + /* Start internal temperature sensing. */ + writel(0, lradc->base + LRADC_CTRL2); +} + +static void mxs_lradc_hw_stop(struct mxs_lradc *lradc) +{ + writel(LRADC_CTRL1_LRADC_IRQ_EN_MASK, + lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_CLR); +} + +static int __devinit mxs_lradc_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct mxs_lradc_data *data; + struct mxs_lradc *lradc; + struct iio_dev *iio; + struct resource *iores; + int ret = 0; + int i; + + lradc = devm_kzalloc(&pdev->dev, sizeof(*lradc), GFP_KERNEL); + if (!lradc) + return -ENOMEM; + + /* Grab the memory area */ + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); + lradc->base = devm_request_and_ioremap(dev, iores); + if (!lradc->base) + return -EADDRNOTAVAIL; + + /* Grab all IRQ sources */ + for (i = 0; i < 13; i++) { + lradc->irq[i] = platform_get_irq(pdev, i); + if (lradc->irq[i] < 0) + return -EINVAL; + + ret = devm_request_irq(dev, lradc->irq[i], + mxs_lradc_handle_irq, 0, + mxs_lradc_irq_name[i], lradc); + if (ret) + return ret; + } + + /* Init workqueues, one per channel */ + for (i = 0; i < LRADC_MAX_MAPPED_CHANS; i++) + init_waitqueue_head(&lradc->wq_data_avail[i]); + + dev_set_drvdata(&pdev->dev, lradc); + + mutex_init(&lradc->map_lock); + + /* Allocate the IIO device. */ + iio = iio_device_alloc(sizeof(*data)); + if (!iio) { + dev_err(dev, "Failed to allocate IIO device %i\n", i); + return -ENOMEM; + } + + lradc->iio = iio; + + iio->name = pdev->name; + iio->dev.parent = &pdev->dev; + iio->info = &mxs_lradc_iio_info; + iio->modes = INDIO_DIRECT_MODE; + iio->channels = mxs_lradc_chan_spec; + iio->num_channels = ARRAY_SIZE(mxs_lradc_chan_spec); + + /* + * This here is intentional. Once we'll support the delay channels this + * hardware provides, we will need to store more data in IIO devices' + * private data. + */ + data = iio_priv(iio); + data->lradc = lradc; + + ret = iio_triggered_buffer_setup(iio, &iio_pollfunc_store_time, + &mxs_lradc_trigger_handler, NULL); + if (ret) + goto err0; + + ret = mxs_lradc_trigger_init(iio); + if (ret) + goto err1; + + /* Configure the hardware. */ + mxs_lradc_hw_init(lradc); + + /* + * Register IIO device + */ + ret = iio_device_register(iio); + if (ret) { + dev_err(dev, "Failed to register IIO device\n"); + goto err2; + } + + return 0; + +err2: + mxs_lradc_trigger_remove(iio); +err1: + iio_triggered_buffer_cleanup(lradc->iio); +err0: + iio_device_free(iio); + return ret; +} + +static int __devexit mxs_lradc_remove(struct platform_device *pdev) +{ + struct mxs_lradc *lradc = dev_get_drvdata(&pdev->dev); + + mxs_lradc_hw_stop(lradc); + + iio_device_unregister(lradc->iio); + iio_triggered_buffer_cleanup(lradc->iio); + mxs_lradc_trigger_remove(lradc->iio); + iio_device_free(lradc->iio); + + return 0; +} + +static const struct of_device_id mxs_lradc_dt_ids[] = { + { .compatible = "fsl,imx28-lradc", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids); + +static struct platform_driver mxs_lradc_driver = { + .driver = { + .name = DRIVER_NAME, + .owner = THIS_MODULE, + .of_match_table = mxs_lradc_dt_ids, + }, + .probe = mxs_lradc_probe, + .remove = __devexit_p(mxs_lradc_remove), +}; + +module_platform_driver(mxs_lradc_driver); + +MODULE_AUTHOR("Marek Vasut <marex@denx.de>"); +MODULE_DESCRIPTION("Freescale i.MX23/i.MX28 LRADC driver"); +MODULE_LICENSE("GPL v2");
This driver is very basic. It supports userland trigger, buffer and raw access to channels. The support for delay channels is missing altogether. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Jonathan Cameron <jic23@kernel.org> Cc: Wolfgang Denk <wd@denx.de> Cc: Juergen Beisert <jbe@pengutronix.de> --- drivers/staging/iio/adc/Kconfig | 12 + drivers/staging/iio/adc/Makefile | 1 + drivers/staging/iio/adc/mxs-lradc.c | 619 +++++++++++++++++++++++++++++++++++ 3 files changed, 632 insertions(+) create mode 100644 drivers/staging/iio/adc/mxs-lradc.c Jonathan, you can now rip me to shreds for not understanding your teachings. Please kindly review the products of my labor ;-)