IIO: Add basic MXS LRADC driver
Subsystems (4)
PATCH 1/1 IIO: Add basic MXS LRADC driver
This driver is very basic. It supports userland trigger, buffer and raw access to channels. The support for delay channels is missing altogether.
--- 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 ;-)
drivers/staging/iio/adc/Kconfig +12 -0
@@ -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
drivers/staging/iio/adc/Makefile +1 -0
@@ -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
drivers/staging/iio/adc/mxs-lradc.c new file +619 -0
619 lines not on this page · raw message
Signature · 1 line
1.7.10
PATCH 1/1 IIO: Add basic MXS LRADC driver
Dear Marek,
Quoted 3 lines · In message <[email protected]> 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.
...
drivers/staging/iio/adc/mxs-lradc.c line 114
Quoted 2 lines
drivers/staging/iio/adc/mxs-lradc.c new file +2
-0+#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)
drivers/staging/iio/adc/mxs-lradc.c line 120
Quoted 2 lines
drivers/staging/iio/adc/mxs-lradc.c new file +2
-0+#define LRADC_DELAY_TRIGGER_LRADCS_MASK (0xff << 24) +#define LRADC_DELAY_TRIGGER_LRADCS_OFFSET 24
Ditto.
drivers/staging/iio/adc/mxs-lradc.c line 125
Quoted 4 lines
drivers/staging/iio/adc/mxs-lradc.c new file +4
-0+#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
Signature · 4 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de The speed of time is one second per second.
PATCH 1/1 IIO: Add basic MXS LRADC driver
In reply to Marek VasutQuoted 9 lines · Marek Vasut
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 <[email protected]>Cc Jonathan Cameron <[email protected]>Cc Wolfgang Denk <[email protected]>Cc Juergen Beisert <[email protected]>[...]
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.
Quoted 6 lines
drivers/staging/iio/adc/mxs-lradc.c new file +6
-0+struct mxs_lradc { +struct iio_dev *iio; +struct device *dev; +void __iomem *base; +int irq[13 ]; +int idis;
drivers/staging/iio/adc/mxs-lradc.c line 91
Quoted 10 lines
drivers/staging/iio/adc/mxs-lradc.c new file +10
-0+ +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.
drivers/staging/iio/adc/mxs-lradc.c line 96
Quoted 5 lines
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+}; + +struct mxs_lradc_data { +struct mxs_lradc *lradc; +};
Is there any reason not to use the mxs_lradc struct directly has the iio data?
drivers/staging/iio/adc/mxs-lradc.c line 138
Quoted 8 lines
[...]
drivers/staging/iio/adc/mxs-lradc.c new file +7
-0+ +/* +* 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.
Quoted 51 lines
drivers/staging/iio/adc/mxs-lradc.c new file +51
-0+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; +} +
[...]
drivers/staging/iio/adc/mxs-lradc.c line 239
Quoted 12 lines
drivers/staging/iio/adc/mxs-lradc.c new file +12
-0+/* +* 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
Quoted 10 lines
drivers/staging/iio/adc/mxs-lradc.c new file +10
-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); +
drivers/staging/iio/adc/mxs-lradc.c line 259
Quoted 10 lines
drivers/staging/iio/adc/mxs-lradc.c new file +10
-0+/* 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;
Quoted 29 lines
drivers/staging/iio/adc/mxs-lradc.c new file +29
-0+/* 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. +*/
drivers/staging/iio/adc/mxs-lradc.c line 298
Quoted 10 lines
drivers/staging/iio/adc/mxs-lradc.c new file +10
-0+ +/* 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?
Quoted 13 lines
drivers/staging/iio/adc/mxs-lradc.c new file +13
-0+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) +{
drivers/staging/iio/adc/mxs-lradc.c line 321
Quoted 10 lines
drivers/staging/iio/adc/mxs-lradc.c new file +10
-0+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
Quoted 41 lines
drivers/staging/iio/adc/mxs-lradc.c new file +41
-0+ + 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 ; +
drivers/staging/iio/adc/mxs-lradc.c line 372
Quoted 10 lines
drivers/staging/iio/adc/mxs-lradc.c new file +10
-0+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.
Quoted 13 lines
drivers/staging/iio/adc/mxs-lradc.c new file +13
-0+ +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; +}
drivers/staging/iio/adc/mxs-lradc.c line 429
Quoted 10 lines
drivers/staging/iio/adc/mxs-lradc.c new file +9
-0+ +/* +* 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.
Quoted 11 lines
drivers/staging/iio/adc/mxs-lradc.c new file +10
-0+ +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;
drivers/staging/iio/adc/mxs-lradc.c line 506
Quoted 10 lines
drivers/staging/iio/adc/mxs-lradc.c new file +10
-0+ + 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.
[...]
Quoted 4 lines
drivers/staging/iio/adc/mxs-lradc.c new file +3
-0+return ret; +} +
PATCH 1/1 IIO: Add basic MXS LRADC driver
Dear Lars-Peter Clausen,
Quoted 11 lines · Marek Vasut
Quoted 9 lines
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 <[email protected]>Cc Jonathan Cameron <[email protected]>Cc Wolfgang Denk <[email protected]>Cc Juergen Beisert <[email protected]>[...]
Quoted 3 lines
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 ;-)
[...]
Quoted 4 lines
drivers/staging/iio/adc/mxs-lradc.c line 91
Quoted 2 lines
drivers/staging/iio/adc/mxs-lradc.c new file +2
-0+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 :)
Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 96
Quoted 5 lines
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+}; + +struct mxs_lradc_data { +struct mxs_lradc *lradc; +};Is there any reason not to use the
mxs_lradcstruct 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.
Quoted 16 lines
Quoted 9 lines
[...]
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+ +/* +* 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 forbitmap_weight(...)<= 8. Those patches are not yet in IIO though.
This is good. When do you expect this to hit linux-next possibly? Or@least linux-iio?
[..]
Quoted 28 lines
drivers/staging/iio/adc/mxs-lradc.c line 239
Quoted 9 lines
drivers/staging/iio/adc/mxs-lradc.c new file +9
-0+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_RAWQuoted 16 lines
drivers/staging/iio/adc/mxs-lradc.c new file +4
-0+return -EINVAL ; + +/* Map channel. */ + slot = mxs_lradc_map_channel(lradc, chan->channel,
LRADC_CHAN_FLAG_RAW); + if (slot < 0)drivers/staging/iio/adc/mxs-lradc.c new file +11
-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) {
Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 259
Quoted 4 lines
drivers/staging/iio/adc/mxs-lradc.c new file +4
-0+ 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_enabledand return-EBUSYif it returns true;
I believe this should work perfectly OK, why won't it ? I tried to avoid such artificial limitation.
[...]
Quoted 6 lines
drivers/staging/iio/adc/mxs-lradc.c line 298
Quoted 3 lines
drivers/staging/iio/adc/mxs-lradc.c new file +3
-0+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.
[...]
Quoted 21 lines
Quoted 21 lines
+static int
mxs_lradc_configure_trigger(struct iio_trigger *trig, bool state)+{drivers/staging/iio/adc/mxs-lradc.c new file +19
-0+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 ) {
Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 372
Quoted 6 lines
drivers/staging/iio/adc/mxs-lradc.c new file +6
-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 ?
Quoted 17 lines
Quoted 17 lines
drivers/staging/iio/adc/mxs-lradc.c new file +16
-0+ +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
Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 429
Quoted 6 lines
+ */
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+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 :-(
Quoted 14 lines
Quoted 14 lines
[...]
drivers/staging/iio/adc/mxs-lradc.c new file +13
-0+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 ;
Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 506
Quoted 6 lines
drivers/staging/iio/adc/mxs-lradc.c new file +6
-0+ +/* 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!
PATCH 1/1 IIO: Add basic MXS LRADC driver
Quoted 37 lines · Marek Vasut
Dear Lars-Peter Clausen,
Quoted 11 lines · Marek Vasut
Quoted 9 lines
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 <[email protected]>Cc Jonathan Cameron <[email protected]>Cc Wolfgang Denk <[email protected]>Cc Juergen Beisert <[email protected]>[...]Quoted 3 lines
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 ;-)
[...]
Quoted 4 lines
drivers/staging/iio/adc/mxs-lradc.c line 91
Quoted 2 lines
drivers/staging/iio/adc/mxs-lradc.c new file +2
-0+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 :)
Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 96
Quoted 5 lines
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+}; + +struct mxs_lradc_data { +struct mxs_lradc *lradc; +};Is there any reason not to use the
mxs_lradcstruct directly has the iio data?
Quoted 3 lines
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.
Quoted 18 lines
Quoted 17 lines
Quoted 10 lines
[...]
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+ +/* +* 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 forbitmap_weight(...)<= 8. Those patches are not yet in IIO though.
Quoted 3 lines
This is good. When do you expect this to hit linux-next possibly? Or at least linux-iio?
soon, hopefully.
Quoted 14 lines
[..]
Quoted 12 lines
drivers/staging/iio/adc/mxs-lradc.c line 239
Quoted 9 lines
drivers/staging/iio/adc/mxs-lradc.c new file +9
-0+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
Quoted 16 lines
Quoted 8 lines
Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c new file +4
-0+return -EINVAL ; + +/* Map channel. */ + slot = mxs_lradc_map_channel(lradc, chan->channel,
LRADC_CHAN_FLAG_RAW); + if (slot < 0)drivers/staging/iio/adc/mxs-lradc.c new file +3
-0+return slot; + +/* Start sampling. */ Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 255
Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c new file +8
-0+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.
Quoted 9 lines
Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 259
Quoted 4 lines
drivers/staging/iio/adc/mxs-lradc.c new file +4
-0+ 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_enabledand return-EBUSYif it returns true;
Quoted 3 lines
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?
Quoted 32 lines
[...]
Quoted 21 lines
Quoted 21 lines
+static int
mxs_lradc_configure_trigger(struct iio_trigger *trig, bool state)+{drivers/staging/iio/adc/mxs-lradc.c new file +19
-0+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 ) {Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 372
Quoted 6 lines
drivers/staging/iio/adc/mxs-lradc.c new file +6
-0+ ret = -EINVAL ; + goto exit; + } + map |=1 << slot; + chans |=1 << bit; + }I think this should go into the buffer preenable and postdisable callbacks.
Quoted 2 lines
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.
PATCH 1/1 IIO: Add basic MXS LRADC driver
Dear Lars-Peter Clausen,
Quoted 38 lines · Marek Vasut
Quoted 37 lines
Dear Lars-Peter Clausen,
Quoted 11 lines · Marek Vasut
Quoted 9 lines
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 <[email protected]>Cc Jonathan Cameron <[email protected]>Cc Wolfgang Denk <[email protected]>Cc Juergen Beisert <[email protected]>[...]Quoted 3 lines
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 ;-)
[...]
Quoted 4 lines
drivers/staging/iio/adc/mxs-lradc.c line 91
Quoted 2 lines
drivers/staging/iio/adc/mxs-lradc.c new file +2
-0+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 :)
Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 96
Quoted 5 lines
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+}; + +struct mxs_lradc_data { +struct mxs_lradc *lradc; +};Is there any reason not to use the
mxs_lradcstruct directly has the iio data?
Quoted 6 lines
Quoted 3 lines
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.
Quoted 47 lines
Quoted 17 lines
Quoted 16 lines
Quoted 9 lines
[...]
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+ +/* +* 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 forbitmap_weight(...)<= 8. Those patches are not yet in IIO though.Quoted 2 lines
This is good. When do you expect this to hit linux-next possibly? Or at least linux-iio?
soon, hopefully.
Quoted 25 lines
[..]
Quoted 23 lines
drivers/staging/iio/adc/mxs-lradc.c line 239
Quoted 9 lines
drivers/staging/iio/adc/mxs-lradc.c new file +9
-0+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_RAWQuoted 11 lines
drivers/staging/iio/adc/mxs-lradc.c new file +4
-0+return -EINVAL ; + +/* Map channel. */ + slot = mxs_lradc_map_channel(lradc, chan->channel,
LRADC_CHAN_FLAG_RAW); + if (slot < 0)drivers/staging/iio/adc/mxs-lradc.c new file +6
-0+return slot; + +/* Start sampling. */ +mxs_lradc_run_slots (lradc,1 << slot); + +/* Wait for completion on the channel, 1 second max. */
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
drivers/staging/iio/adc/mxs-lradc.c line 255
Quoted 5 lines
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+ 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_timeoutcan return an error value, e.g. if it has been interrupted.
I replaced this with killable completion.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 259
Quoted 4 lines
drivers/staging/iio/adc/mxs-lradc.c new file +4
-0+ 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_enabledand return-EBUSYif it returns true;
Quoted 5 lines
Quoted 2 lines
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_slotsdo exactly?
Starts the sampling of the channels that are specific in the mask passed to this function. But they must be mapped first.
Quoted 35 lines
Quoted 33 lines
[...]
Quoted 22 lines
Quoted 22 lines
+static int
mxs_lradc_configure_trigger(struct iio_trigger *trig, bool state)+{drivers/staging/iio/adc/mxs-lradc.c new file +20
-0+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 ;Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 372
Quoted 5 lines
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+ goto exit; + } + map |=1 << slot; + chans |=1 << bit; + }I think this should go into the buffer preenable and postdisable callbacks.
Quoted 1 line
How much of it, only the slot mapping or the allocation too ?
Quoted 7 lines
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_modecallback. 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
PATCH 1/1 IIO: Add basic MXS LRADC driver
In reply to Lars-Peter ClausenDear Lars-Peter Clausen,
Sorry for my late reply, got busy with other crazy stuff.
[...]
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
Is there any reason not to use the
mxs_lradcstruct 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.
Quoted 17 lines
Quoted 17 lines
Quoted 16 lines
Quoted 9 lines
[...]
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+ +/* +* 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 forbitmap_weight(...)<= 8. Those patches are not yet in IIO though.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 25 lines
Quoted 25 lines
[..]
Quoted 23 lines
drivers/staging/iio/adc/mxs-lradc.c line 239
Quoted 9 lines
drivers/staging/iio/adc/mxs-lradc.c new file +9
-0+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_RAWQuoted 11 lines
drivers/staging/iio/adc/mxs-lradc.c new file +4
-0+return -EINVAL ; + +/* Map channel. */ + slot = mxs_lradc_map_channel(lradc, chan->channel,
LRADC_CHAN_FLAG_RAW); + if (slot < 0)drivers/staging/iio/adc/mxs-lradc.c new file +6
-0+return slot; + +/* Start sampling. */ +mxs_lradc_run_slots (lradc,1 << slot); + +/* Wait for completion on the channel, 1 second max. */
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
drivers/staging/iio/adc/mxs-lradc.c line 255
Quoted 5 lines
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+ 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_timeoutcan return an error value, e.g. if it has been interrupted.
Fixed
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 259
Quoted 4 lines
drivers/staging/iio/adc/mxs-lradc.c new file +4
-0+ 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_enabledand return-EBUSYif it returns true;
Quoted 5 lines
Quoted 2 lines
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_slotsdo exactly?
Execute the sampling on the mapped slots.
Quoted 35 lines
Quoted 33 lines
[...]
Quoted 22 lines
Quoted 22 lines
+static int
mxs_lradc_configure_trigger(struct iio_trigger *trig, bool state)+{drivers/staging/iio/adc/mxs-lradc.c new file +20
-0+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 ;Quoted 8 lines
drivers/staging/iio/adc/mxs-lradc.c line 372
Quoted 5 lines
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+ goto exit; + } + map |=1 << slot; + chans |=1 << bit; + }I think this should go into the buffer preenable and postdisable callbacks.
Quoted 1 line
How much of it, only the slot mapping or the allocation too ?
Quoted 7 lines
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_modecallback. 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
PATCH 1/1 IIO: Add basic MXS LRADC driver
Quoted 35 lines · Marek Vasut
Dear Lars-Peter Clausen,
Sorry for my late reply, got busy with other crazy stuff.
[...]
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
Is there any reason not to use the
mxs_lradcstruct 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.
Quoted 17 lines
Quoted 17 lines
Quoted 16 lines
Quoted 9 lines
[...]
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+ +/* +* 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 forbitmap_weight(...)<= 8. Those patches are not yet in IIO though.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
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@once, which again means that starting sampling can't fail of this.
- Lars
PATCH 1/1 IIO: Add basic MXS LRADC driver
Dear Lars-Peter Clausen,
Quoted 43 lines · Marek Vasut
Quoted 42 lines
Dear Lars-Peter Clausen,
Sorry for my late reply, got busy with other crazy stuff.
[...]
Quoted 10 lines
Quoted 6 lines
Quoted 2 lines
Is there any reason not to use the
mxs_lradcstruct 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.
Quoted 22 lines
Quoted 18 lines
Quoted 17 lines
Quoted 9 lines
[...]
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+ +/* +* 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 forbitmap_weight(...)<= 8. Those patches are not yet in IIO though.Quoted 2 lines
This is good. When do you expect this to hit linux-next possibly? Or at least linux-iio?
soon, hopefully.
Quoted 8 lines
Quoted 1 line
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
PATCH 1/1 IIO: Add basic MXS LRADC driver
In reply to Lars-Peter ClausenDear Lars-Peter Clausen,
Quoted 43 lines · Marek Vasut
Quoted 42 lines
Dear Lars-Peter Clausen,
Sorry for my late reply, got busy with other crazy stuff.
[...]
Quoted 10 lines
Quoted 6 lines
Quoted 2 lines
Is there any reason not to use the
mxs_lradcstruct 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.
Quoted 22 lines
Quoted 18 lines
Quoted 17 lines
Quoted 9 lines
[...]
drivers/staging/iio/adc/mxs-lradc.c new file +5
-0+ +/* +* 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 forbitmap_weight(...)<= 8. Those patches are not yet in IIO though.Quoted 2 lines
This is good. When do you expect this to hit linux-next possibly? Or at least linux-iio?
soon, hopefully.
Quoted 8 lines
Quoted 1 line
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
PATCH 1/1 IIO: Add basic MXS LRADC driver
Dear Lars-Peter Clausen,
Quoted 52 lines
Quoted 43 lines · Marek Vasut
Quoted 42 lines
Dear Lars-Peter Clausen,
Sorry for my late reply, got busy with other crazy stuff.
[...]
Quoted 10 lines
Quoted 6 lines
Quoted 2 lines
Is there any reason not to use the
mxs_lradcstruct 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.
Quoted 22 lines
Quoted 18 lines
Quoted 17 lines
Quoted 9 lines
[...]
(quoted diff) +5
-0+ +/* + * Channel management + * + * This involves crazy mapping between 8 virtual channels theCTRL4 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 forbitmap_weight(...)<= 8. Those patches are not yet in IIO though.Quoted 2 lines
This is good. When do you expect this to hit linux-next possibly? Or at least linux-iio?
soon, hopefully.
Quoted 8 lines
Quoted 1 line
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.
Quoted 5 lines
Ok, I fixed this one. One last thing that I don't really understand is this. I run
generic_buffer.cto 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 subsequentread()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
PATCH 1/1 IIO: Add basic MXS LRADC driver
Quoted 5 lines · Marek Vasut
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
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
PATCH 1/1 IIO: Add basic MXS LRADC driver
Dear Robert Schwebel,
Quoted 10 lines · Marek Vasut
Quoted 5 lines
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
PATCH 1/1 IIO: Add basic MXS LRADC driver
In reply to Marek VasutQuoted 9 lines · Marek Vasut
Quoted 1 line
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
Jonathan Cameron
maintainerPATCH 1/1 IIO: Add basic MXS LRADC driver
Quoted 11 lines · Lars-Peter Clausen
Quoted 9 lines · Marek Vasut
Quoted 1 line
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?
Quoted 6 lines
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
Quoted 3 lines
- Lars
PATCH 1/1 IIO: Add basic MXS LRADC driver
In reply to Marek VasutQuoted 54 lines · Marek Vasut
Dear Lars-Peter Clausen,
Quoted 43 lines · Marek Vasut
Quoted 42 lines
Dear Lars-Peter Clausen,
Sorry for my late reply, got busy with other crazy stuff.
[...]
Quoted 10 lines
Quoted 6 lines
Quoted 2 lines
Is there any reason not to use the
mxs_lradcstruct 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.
Quoted 22 lines
Quoted 18 lines
Quoted 17 lines
Quoted 9 lines
[...]
(quoted diff) +5
-0+ +/* + * Channel management + * + * This involves crazy mapping between 8 virtual channels the CTRL4register + * 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 forbitmap_weight(...)<= 8. Those patches are not yet in IIO though.Quoted 2 lines
This is good. When do you expect this to hit linux-next possibly? Or at least linux-iio?
soon, hopefully.
Quoted 8 lines
Quoted 1 line
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.
Quoted 5 lines
Ok, I fixed this one. One last thing that I don't really understand is this. I run
generic_buffer.cto 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 subsequentread()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
PATCH 1/1 IIO: Add basic MXS LRADC driver
Dear Lars-Peter Clausen,
Quoted 62 lines · Marek Vasut
Quoted 55 lines
Dear Lars-Peter Clausen,
Quoted 45 lines · Marek Vasut
Quoted 37 lines
Dear Lars-Peter Clausen,
Sorry for my late reply, got busy with other crazy stuff.
[...]
Quoted 10 lines
Quoted 6 lines
Quoted 2 lines
Is there any reason not to use the
mxs_lradcstruct 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.
Quoted 18 lines
Quoted 18 lines
Quoted 17 lines
Quoted 9 lines
[...]
(quoted diff) +5
-0+ +/* + * Channel management + * + * This involves crazy mapping between 8 virtual channels theCTRL4 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 forbitmap_weight(...)<= 8. Those patches are not yet in IIO though.Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 7 lines
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.Quoted 5 lines
Ok, I fixed this one. One last thing that I don't really understand is this. I run
generic_buffer.cto 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 subsequentread()call).
Quoted 4 lines
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.
Quoted 1 line
- Lars
Best regards,
Marek Vasut
PATCH 1/1 IIO: Add basic MXS LRADC driver
In reply to Marek VasutHi Marek,
Quoted 8 lines · Marek Vasut
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 <[email protected]>Cc Jonathan Cameron <[email protected]>Cc Wolfgang Denk <[email protected]>Cc Juergen Beisert <[email protected]>
Quoted 8 lines
--- 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
Signature · 2 lines
Pengutronix e.K. | Juergen Beisert | Linux Solutions for Science and Industry | http://www.pengutronix.de/ |
PATCH 1/1 IIO: Add basic MXS LRADC driver
Quoted 18 lines · Juergen Beisert
Hi Marek,
Quoted 15 lines · Marek Vasut
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 <[email protected]>Cc Jonathan Cameron <[email protected]>Cc Wolfgang Denk <[email protected]>Cc Juergen Beisert <[email protected]>--- 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
Quoted 7 lines
Quoted 1 line
[...]
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
PATCH 1/1 IIO: Add basic MXS LRADC driver
In reply to Juergen BeisertDear Juergen Beisert,
Quoted 19 lines
Hi Marek,
Quoted 16 lines · Marek Vasut
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 <[email protected]>Cc Jonathan Cameron <[email protected]>Cc Wolfgang Denk <[email protected]>Cc Juergen Beisert <[email protected]>---
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
Quoted 3 lines
Quoted 1 line
[...]
sorry for the delay, too much work at the same time.
You tell me.
Quoted 5 lines
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.
Quoted 3 lines
Regards,
Juergen
Best regards,
Marek Vasut
PATCH 1/1 IIO: Add basic MXS LRADC driver
Hi Marek,
Quoted 11 lines
Quoted 5 lines
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 directoryYou 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
Signature · 2 lines
Pengutronix e.K. | Juergen Beisert | Linux Solutions for Science and Industry | http://www.pengutronix.de/ |
PATCH 1/1 IIO: Add basic MXS LRADC driver
Dear Juergen Beisert,
Quoted 14 lines
Hi Marek,
Quoted 11 lines
Quoted 5 lines
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 directoryYou 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.
Quoted 7 lines
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 at 80050000 {
compatible = "fsl,imx28-lradc";
reg = <0x80050000 2000>;
interrupts = <10 14 15 16 17 18 19
20 21 22 23 24 25>;
status = "disabled";
};Quoted 2 lines
Regards,
Juergen
Best regards,
Marek Vasut
PATCH 1/1 IIO: Add basic MXS LRADC driver
Hi Marek,
Quoted 22 lines · Marek Vasut
Quoted 12 lines
Quoted 11 lines
Quoted 5 lines
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 directoryYou 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.
Quoted 7 lines
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
Quoted 7 lines
lradc at 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
Signature · 2 lines
Pengutronix e.K. | Juergen Beisert | Linux Solutions for Science and Industry | http://www.pengutronix.de/ |
PATCH 1/1 IIO: Add basic MXS LRADC driver
Dear Juergen Beisert,
Quoted 80 lines
Hi Marek,
Quoted 32 lines · Marek Vasut
Quoted 12 lines
Quoted 11 lines
Quoted 5 lines
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 directoryYou 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.
Quoted 7 lines
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 at 80050000 {
compatible = "fsl,imx28-lradc"; reg = <0x80050000 2000>; interrupts = <10 14 15 16 17 18 19 20 21 22 23 24 25>; status = "disabled";Quoted 1 line
};
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 [...]
Quoted 1 line
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?
Quoted 2 lines
Regards,
Juergen
Best regards,
Marek Vasut
PATCH 1/1 IIO: Add basic MXS LRADC driver
Quoted 82 lines · Marek Vasut
Dear Juergen Beisert,
Quoted 80 lines
Hi Marek,
Quoted 32 lines · Marek Vasut
Quoted 12 lines
Quoted 11 lines
Quoted 5 lines
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 directoryYou 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.
Quoted 7 lines
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 at 80050000 {
compatible = "fsl,imx28-lradc"; reg = <0x80050000 2000>; interrupts = <10 14 15 16 17 18 19 20 21 22 23 24 25>; status = "disabled";Quoted 1 line
};
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 [...]
Quoted 4 lines
Quoted 1 line
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
Signature · 2 lines
Pengutronix e.K. | Juergen Beisert | Linux Solutions for Science and Industry | http://www.pengutronix.de/ |
PATCH 1/1 IIO: Add basic MXS LRADC driver
Quoted 83 lines · Juergen Beisert
Quoted 82 lines · Marek Vasut
Dear Juergen Beisert,
Quoted 80 lines
Hi Marek,
Quoted 32 lines · Marek Vasut
Quoted 12 lines
Quoted 11 lines
Quoted 5 lines
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 directoryYou 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.
Quoted 7 lines
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";Quoted 1 line
};
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 [...]
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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
PATCH 1/1 IIO: Add basic MXS LRADC driver
Quoted 90 lines · Lars-Peter Clausen
Quoted 89 lines · Juergen Beisert
Quoted 87 lines · Marek Vasut
Dear Juergen Beisert,
Quoted 80 lines
Hi Marek,
Quoted 32 lines · Marek Vasut
Quoted 12 lines
Quoted 11 lines
Quoted 5 lines
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 directoryYou 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.
Quoted 7 lines
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 at 80050000 {
compatible = "fsl,imx28-lradc"; reg = <0x80050000 2000>; interrupts = <10 14 15 16 17 18 19 20 21 22 23 24 25>; status = "disabled";Quoted 1 line
};
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 [...]Quoted 1 line
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?
Quoted 6 lines
Quoted 2 lines
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.
Quoted 4 lines
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
Signature · 2 lines
Pengutronix e.K. | Juergen Beisert | Linux Solutions for Science and Industry | http://www.pengutronix.de/ |
PATCH 1/1 IIO: Add basic MXS LRADC driver
Dear Juergen Beisert,
[...]
Quoted 12 lines
Quoted 11 lines
Quoted 6 lines
Quoted 3 lines
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.
Quoted 8 lines
Makes sense.
Quoted 4 lines
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.
Quoted 2 lines
Regards,
Juergen
Best regards,
Marek Vasut