Jonathan Cameron wrote on 2011-04-17:
> Hi Michael,
>
> Did my reply answer your question such that you don't mind me sending
> these (well rebased version anyway) on to Greg?
Sure - it answered my question.
Acked-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
>
> I'm really not liking my 100 patch queue :(
>> ...
>>>> iio_device_unregister(indio_dev); diff --git
>>>> a/drivers/staging/iio/imu/adis16400_trigger.c
>>>> b/drivers/staging/iio/imu/adis16400_trigger.c
>>>> index 36b5ff5..afa5e74 100644
>>>> --- a/drivers/staging/iio/imu/adis16400_trigger.c
>>>> +++ b/drivers/staging/iio/imu/adis16400_trigger.c
>>>> @@ -15,21 +15,13 @@
>>>> /**
>>>> * adis16400_data_rdy_trig_poll() the event handler for the data rdy
>>>> trig **/
>>>> -static int adis16400_data_rdy_trig_poll(struct iio_dev *dev_info,
>>>> - int index,
>>>> - s64 timestamp,
>>>> - int no_test)
>>>> +static irqreturn_t adis16400_data_rdy_trig_poll(int irq, void
>>>> +*private)
>>>> {
>>>> - struct adis16400_state *st = iio_dev_get_devdata(dev_info);
>>>> - struct iio_trigger *trig = st->trig;
>>>> -
>>>> - iio_trigger_poll(trig, timestamp);
>>>> -
>>>> + disable_irq_nosync(irq);
>>>> + iio_trigger_poll(private, iio_get_time_ns());
>>>>
>>> Is it save to call iio_trigger_poll() from the Top Half Hard-IRQ?
>>> Instead of disable/enable irq shouldn't this be better a threaded_irq
>>> with IRQF_ONESHOT?
>> Yes. Actually after conversion to irq chips this is precisely what you
>> want to do. If you call it as threaded_irq with oneshot enabled, then
>> the triggered devices can't have top halves. That only really matters
>> if you either want a timestamp or you want to flip a gpio to trigger
>> the actual sampling. Note we can't do top halves at all for software
>> triggers. How to handle this is a corner I haven't cleaned up yet.
>>
>> When we switch over to that approach you don't disable this irq at all.
>> Rather you have the trigger_consumer as a threaded_irq with
>> IRQF_ONESHOT set. That serializes reads from the device and writing to
>> the buffer implementation. It's this serialization that motivates the
>> disabling of irq's here.
>>
>> Having said that, the reason it is like this here, is that it
>> replicates what was previously happening. Without the hardware I don't
>> want to mess with it too much + most of this code is going to go away
>> shortly anyway!
>>
>> To illustrate, take a look at accel/lis3l02dq (bit of a weird case) or
>> imu/adis16400 (untested right now) in
>> http://git.kernel.org/?p=linux/kernel/git/jic23/iio-onwards.git in a
>> few mins (just pushed).
>>
>> Ripping out the relevant bits...
>>
>> from adis16400_trigger.c
>>
>> static irqreturn_t adis16400_data_rdy_trig_poll(int irq, void
>> *private) {
>> iio_trigger_poll(private, iio_get_time_ns());
>> return IRQ_HANDLED;
>> }
>>
>> int adis16400_probe_trigger(struct iio_dev *indio_dev) { ...
>> ret = request_irq(st->us->irq,
>> adis16400_data_rdy_trig_poll,
>> IRQF_TRIGGER_RISING,
>> "adis16400",
>> st->trig);
>> ...
>> }
>>
>> from adis16400_ring.c
>>
>> static irqreturn_t adis16400_trigger_handler(int irq, void *p) {
>> struct iio_poll_func *pf = p;
>> struct iio_dev *indio_dev = pf->private_data;
>> struct adis16400_state *st = iio_dev_get_devdata(indio_dev);
>>
>> ... do stuff...
>>
>> iio_trigger_notify_done(st->indio_dev->trig);
>> ... clean up ...
>> return IRQ_HANDLED;
>> }
>>
>> int adis16400_configure_ring(struct iio_dev *indio_dev) { ...
>> indio_dev->pollfunc->private_data = indio_dev;
>> indio_dev->pollfunc->h = &iio_pollfunc_store_time;
>> indio_dev->pollfunc->thread = &adis16400_trigger_handler;
>> indio_dev->pollfunc->type = IRQF_ONESHOT;
>> indio_dev->pollfunc->name =
>> kasprintf(GFP_KERNEL, "adis16400_consumer%d", indio_dev- id);
>>
>> ...
>> }
>>
>> Sometimes some fiddling is needed in the reenable for the interrupt to
>> make sure we don't get stuck with it high (for interrupts that don't
>> clear unless a read occurs - see lis3l02dq).
>>
>>>
>>>> return IRQ_HANDLED;
>>>> }
>>>> -IIO_EVENT_SH(data_rdy_trig, &adis16400_data_rdy_trig_poll);
>>>> -
>>>> static IIO_TRIGGER_NAME_ATTR;
>>>>
>>>> static struct attribute *adis16400_trigger_attrs[] = { @@ -49,22
>>>> +41,9 @@ static int adis16400_data_rdy_trigger_set_state(struct
>>>> iio_trigger *trig, {
>>>> struct adis16400_state *st = trig->private_data;
>>>> struct iio_dev *indio_dev = st->indio_dev;
>>>> - int ret = 0;
>>>>
>>>> dev_dbg(&indio_dev->dev, "%s (%d)\n", __func__, state);
>>>> - ret = adis16400_set_irq(&st->indio_dev->dev, state);
>>>> - if (state == false) {
>>>> - iio_remove_event_from_list(&iio_event_data_rdy_trig,
>>>> - &indio_dev->interrupts[0]
>>>> - ->ev_list);
>>>> - /* possible quirk with handler currently worked around
>>>> - by ensuring the work queue is empty */
>>>> - flush_scheduled_work();
>>>> - } else {
>>>> - iio_add_event_to_list(&iio_event_data_rdy_trig,
>>>> - &indio_dev->interrupts[0]->ev_list);
>>>> - }
>>>> - return ret;
>>>> + return adis16400_set_irq(&st->indio_dev->dev, state);
>>>> }
>>>>
>>>> /**
>>>> @@ -85,12 +64,23 @@ int adis16400_probe_trigger(struct iio_dev
> *indio_dev)
>>>> struct adis16400_state *st = indio_dev->dev_data;
>>>>
>>>> st->trig = iio_allocate_trigger();
>>>> + if (st->trig == NULL) {
>>>> + ret = -ENOMEM;
>>>> + goto error_ret;
>>>> + }
>>>> + ret = request_irq(st->us->irq,
>>>> + adis16400_data_rdy_trig_poll,
>>>> + IRQF_TRIGGER_RISING,
>>>> + "adis16400",
>>>> + st->trig);
>>>>
>>>
>>> threaded_irq with IRQF_ONESHOT?
>>>
>>>> + if (ret)
>>>> + goto error_free_trig;
>>>> st->trig->name = kasprintf(GFP_KERNEL,
>>>> "adis16400-dev%d",
>>>> indio_dev->id);
>>>> if (!st->trig->name) {
>>>> ret = -ENOMEM;
>>>> - goto error_free_trig;
>>>> + goto error_free_irq;
>>>> }
>>>> st->trig->dev.parent = &st->us->dev;
>>>> st->trig->owner = THIS_MODULE;
>>>> @@ -109,9 +99,11 @@ int adis16400_probe_trigger(struct iio_dev
>>>> *indio_dev)
>>>>
>>>> error_free_trig_name:
>>>> kfree(st->trig->name);
>>>> +error_free_irq:
>>>> + free_irq(st->us->irq, st->trig);
>>>> error_free_trig:
>>>> iio_free_trigger(st->trig);
>>>> -
>>>> +error_ret:
>>>> return ret;
>>>> }
>>>> @@ -121,5 +113,6 @@ void adis16400_remove_trigger(struct iio_dev
>>>> *indio_dev)
>>>>
>>>> iio_trigger_unregister(state->trig); kfree(state->trig->name);
>>>> + free_irq(state->us->irq, state->trig);
>>>> iio_free_trigger(state->trig); }
>>>
>>>
>>
>> --
>> 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
>>
Greetings,
Michael
--
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif
--
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
