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