>>> +static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>>> +{
>>> + struct iio_poll_func *pf = p;
>>> + struct iio_dev *idev = pf->indio_dev;
>>> + struct at91_adc_state *st = iio_priv(idev);
>>> + struct iio_buffer *buffer = idev->buffer;
>>> + int len = 0;
>>> +
>> This doesn't look like stuff that should be in the fast patch.
>> Could you cache the register to be read in the update_scan_mask
>> callback then just run over them here? Feels like that might
>> be cleaner and lead to reads occuring faster.
>
> Hmm, I don't see any update_scan_mask callback neither in the code nor
> in the patches sent to the ml, am I missing something ?
sorry, update_scan_mode... Went in with a patch called
staging:iio: add hook to allow core to perform scan related config.
I think that merged a couple of days back..
>
>>> + if (!bitmap_empty(idev->active_scan_mask, idev->masklength)) {
>>> + int i, j;
>>> + for (i = 0, j = 0;
>>> + i < bitmap_weight(idev->active_scan_mask,
>>> + idev->masklength);
>>> + i++) {
>>> + j = find_next_bit(buffer->scan_mask,
>>> + idev->masklength,
>>> + j);
>>> + st->buffer[i] = at91_adc_reg_read(st, AT91_ADC_CHR(j));
>>> + j++;
>>> + len += 2;
>>> + }
>>> + }
>>> +
>>> + if (buffer->scan_timestamp) {
>>> + s64 *timestamp = (s64 *)((u8 *)st->buffer + ALIGN(len,
>>> + sizeof(s64)));
>>> + *timestamp = pf->timestamp;
>>> + }
>>> +
>>> + buffer_store_to(buffer, (u8 *)st->buffer, pf->timestamp);
>>> +
>>> + iio_trigger_notify_done(idev->trig);
>>> + st->irq_enabled = true;
>>> +
>>> + /* Needed to ACK the DRDY interruption */
>>> + at91_adc_reg_read(st, AT91_ADC_LCDR);
>> Still unsure why not threaded_irq and IRQF_ONESHOT.
>
> Oops, forgot to change it, sorry.
>
>> + I'd prefer to see that ack in try_reenable callback.
>> Also, if we need to ack, then why do we need to disable
>> the irq at all? Surely it won't reoccur until we have
>> acked?
>
> If we don't ACK the interrupt, it will be replayed again and again.
Level interrupt? May be some issues as it will occur prior to ack then...
> Moreover, if we don't disable the interrupt, as we have no guarantee
> about when the reading will actually occur, won't this screw things up
> if the triggers keeps firing and we never enter in the handler that
> store data in the buffer ?
The irqf_oneshot stuff handles this fine. Means only one instance is
running at a time (done by masking the interrupt). Also ensures you
don't miss any.
>
>>> +
>>> + enable_irq(st->irq);
>>> +
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> static irqreturn_t at91_adc_eoc_trigger(int irq, void *private)
>>> {
>>> struct iio_dev *idev = private;
>>> @@ -92,13 +184,16 @@ static irqreturn_t at91_adc_eoc_trigger(int irq, void *private)
>>> if (!(status & AT91_ADC_DRDY))
>>> return IRQ_HANDLED;
>>>
>>> - if (status & st->channels_mask) {
>>> - st->done = true;
>>> + if (iio_buffer_enabled(idev)) {
>>> + disable_irq_nosync(irq);
>>> + st->irq_enabled = false;
>>> + iio_trigger_poll(idev->trig, iio_get_time_ns());
>>> + } else {
>>> st->last_value = at91_adc_reg_read(st, AT91_ADC_LCDR);
>>> + st->done = true;
>>> + wake_up_interruptible(&st->wq_data_avail);
>>> }
>>>
>>> - wake_up_interruptible(&st->wq_data_avail);
>>> -
>>> return IRQ_HANDLED;
>>> }
>>>
>>> @@ -110,8 +205,9 @@ static int at91_adc_channel_init(struct iio_dev *idev,
>>> int bit, idx = 0;
>>>
>>> idev->num_channels = bitmap_weight(&pdata->channels_used,
>>> - st->desc->num_channels);
>>> - chan_array = kcalloc(idev->num_channels, sizeof(struct iio_chan_spec),
>>> + st->desc->num_channels) + 1;
>>> + chan_array = kcalloc(idev->num_channels + 1,
>>> + sizeof(struct iio_chan_spec),
>>> GFP_KERNEL);
>> Cosmetic change that should not be in this patch...
>
> Well, the number of allocated channels has been incremented by 1 for the
> timestamp in the process, and while the diff makes it look that way, I
> don't see any cosmetic change here.
Ooops, missed that change in there. Sorry!
>
>>>
>>> if (chan_array == NULL)
>>> @@ -122,6 +218,7 @@ static int at91_adc_channel_init(struct iio_dev *idev,
>>> chan->type = IIO_VOLTAGE;
>>> chan->indexed = 1;
>>> chan->channel = bit;
>>> + chan->scan_index = idx;
>>> chan->scan_type.sign = 'u';
>>> chan->scan_type.realbits = 10;
>>> chan->scan_type.storagebits = 16;
>>> @@ -129,6 +226,13 @@ static int at91_adc_channel_init(struct iio_dev *idev,
>>> idx++;
>>> }
>>>
>>> + (chan_array + idx)->type = IIO_TIMESTAMP;
>>> + (chan_array + idx)->channel = -1;
>>> + (chan_array + idx)->scan_index = idx;
>>> + (chan_array + idx)->scan_type.sign = 's';
>>> + (chan_array + idx)->scan_type.realbits = 64;
>>> + (chan_array + idx)->scan_type.storagebits = 64;
>>> +
>>> idev->channels = chan_array;
>>> return idev->num_channels;
>>> }
>>> @@ -138,6 +242,152 @@ static void at91_adc_channel_remove(struct iio_dev *idev)
>>> kfree(idev->channels);
>>> }
>>>
>>> +static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>>> +{
>>> + struct iio_dev *idev = trig->private_data;
>>> + struct at91_adc_state *st = iio_priv(idev);
>>> + struct iio_buffer *buffer = idev->buffer;
>>> + u32 status = at91_adc_reg_read(st, AT91_ADC_MR);
>>> + u8 bit;
>>> +
>>> + if (state) {
>>> + size_t datasize = buffer_get_bytes_per_datum(buffer);
>>> + int i;
>>> +
>>> + st->buffer = kmalloc(datasize, GFP_KERNEL);
>>> + if (st->buffer == NULL)
>>> + return -ENOMEM;
>>> +
>>> + for (i = 0; (st->desc->triggers + i) != NULL; i++) {
>> I kind of feels here like we ought to be able to do better than matching
>> by name. After all, both ends of this match are controlled by this driver.
>> Perhaps embed the iio_trigger structure in a device specific one and
>> store some sort of reference in that? Just feels a little odd as it
>> currently stands...
>
> I agree on having a better dispatch mechanism, but maybe we could just
> add a integer ID in the iio_trigger structure directly instead of
> relying on some "heavy" structure manipulation. Of course, this ID would
> have to be set in stone by the driver, juste like the channel part in
> iio_chan_spec.
Maybe... I'd prefer a containing structure for now just because I don't
have a feel for how common. In other cases where we have a single
driver with multiple triggers (the userspace one, rtc etc) we handle
this by having the private pointer point at the trigger rather than a
containing structure. I can see why you haven't done this here though...
>
>>> + char *name = kasprintf(GFP_KERNEL,
>>> + "%s-dev%d-%s",
>>> + idev->name,
>>> + idev->id,
>>> + st->desc->triggers[i].name);
>>> + if (name == NULL)
>>> + return -ENOMEM;
>>> +
>>> + if (strcmp(idev->trig->name, name) == 0) {
>>> + status |= st->desc->triggers[i].value;
>>> + kfree(name);
>>> + break;
>>> + }
>>> +
>>> + kfree(name);
>>> + }
>>> +
>>> + if ((st->desc->triggers + i) == NULL)
>>> + return -EINVAL;
>>> +
>>> + at91_adc_reg_write(st, AT91_ADC_MR, status);
>>> +
>>> + for_each_set_bit(bit, buffer->scan_mask,
>>> + st->desc->num_channels) {
>>> + struct iio_chan_spec const *chan = idev->channels + bit;
>>> + at91_adc_reg_write(st, AT91_ADC_CHER,
>>> + AT91_ADC_CH(chan->channel));
>>> + }
>>> +
>>> + at91_adc_reg_write(st, AT91_ADC_IER, AT91_ADC_DRDY);
>>> +
>>> + } else {
>>> + at91_adc_reg_write(st, AT91_ADC_IDR, AT91_ADC_DRDY);
>>> + at91_adc_reg_write(st, AT91_ADC_MR,
>>> + status & ~AT91_ADC_TRGEN);
>>> +
>>> + for_each_set_bit(bit, buffer->scan_mask,
>>> + st->desc->num_channels) {
>>> + at91_adc_reg_write(st, AT91_ADC_CHDR,
>>> + AT91_ADC_CH(bit));
>> Why do we need to look up the channel to turn it on, and not
>> to turn it off? Looks suspicious!
>
> Oh, right.
>
>> beware that things may well go wrong when we introduce multiple
>> buffers. I'm not sure that the scan configuration (which is
>> what is actually happening here) should be in the trigger
>> enable disable at all. Is this just to get us back to a
>> consistent state when not in triggered mode or is it
>> needed to stop triggers occuring?
>
> Both actually. It sets us back into a state were we can safely either
> use software triggers or setup a new capture. If we remove the setup of
> the channels from here and put it into some callback, I'm quite certain
> we will reach some corner cases when we come back from triggered mode,
> with channels still enabled in IIO (and presumably in the hardware too)
> and read into in_voltageX_raw...
Then you make the in_voltageX_raw call do the channel setup rather than
the other way around. Lots of drivers doing precisely that...
That way no corner cases matter as we sanity check that the setup is
correct for whatever we want to do. Tends to lead to simpler code as
well overall.
--
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
