Re: [PATCH 05/12] staging:iio:adis16400 replace unnecessary event line registration.

[Jonathan Cameron <jic23@cam.ac.uk>]

...
>>  	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);
>>  }
>>   
> 
>