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@xxxxxxx>
> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>
> Cc: Wolfgang Denk <wd@xxxxxxx>
> Cc: Juergen Beisert <jbe@xxxxxxxxxxxxxx>
> [...]
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;
> +}
> +
> [...]
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
