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@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.
> >
> > 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
--
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
