From: Michael Hennerich <michael.hennerich@analog.com>
To: Jonathan Cameron <jic23@cam.ac.uk>
Cc: "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>
Subject: Re: [PATCH 05/12] staging:iio:adis16400 replace unnecessary event line registration.
Date: Wed, 13 Apr 2011 15:55:43 +0200 [thread overview]
Message-ID: <4DA5AB5F.2060000@analog.com> (raw)
In-Reply-To: <1302702187-953-6-git-send-email-jic23@cam.ac.uk>
On 04/13/2011 03:43 PM, Jonathan Cameron wrote:
> Whilst the adis16400 does indeed support events, currently the driver
> does not. The trigger code should never use that infrastructure.
>
> Untested
>
> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
> drivers/staging/iio/imu/adis16400_core.c | 26 +--------------
> drivers/staging/iio/imu/adis16400_trigger.c | 47 +++++++++++---------------
> 2 files changed, 21 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/staging/iio/imu/adis16400_core.c b/drivers/staging/iio/imu/adis16400_core.c
> index 3439239..b8831dc 100644
> --- a/drivers/staging/iio/imu/adis16400_core.c
> +++ b/drivers/staging/iio/imu/adis16400_core.c
> @@ -552,14 +552,6 @@ static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("409 546 819 1638");
>
> static IIO_CONST_ATTR_NAME("adis16400");
>
> -static struct attribute *adis16400_event_attributes[] = {
> - NULL
> -};
> -
> -static struct attribute_group adis16400_event_attribute_group = {
> - .attrs = adis16400_event_attributes,
> -};
> -
> static struct attribute *adis16400_attributes[] = {
> &iio_dev_attr_gyro_x_calibbias.dev_attr.attr,
> &iio_dev_attr_gyro_y_calibbias.dev_attr.attr,
> @@ -629,8 +621,6 @@ static int __devinit adis16400_probe(struct spi_device *spi)
> }
>
> st->indio_dev->dev.parent = &spi->dev;
> - st->indio_dev->num_interrupt_lines = 1;
> - st->indio_dev->event_attrs = &adis16400_event_attribute_group;
> st->indio_dev->attrs = &adis16400_attribute_group;
> st->indio_dev->dev_data = (void *)(st);
> st->indio_dev->driver_module = THIS_MODULE;
> @@ -652,17 +642,9 @@ static int __devinit adis16400_probe(struct spi_device *spi)
> }
>
> if (spi->irq && gpio_is_valid(irq_to_gpio(spi->irq)) > 0) {
> - ret = iio_register_interrupt_line(spi->irq,
> - st->indio_dev,
> - 0,
> - IRQF_TRIGGER_RISING,
> - "adis16400");
> - if (ret)
> - goto error_uninitialize_ring;
> -
> ret = adis16400_probe_trigger(st->indio_dev);
> if (ret)
> - goto error_unregister_line;
> + goto error_uninitialize_ring;
> }
>
> /* Get the device into a sane initial state */
> @@ -674,9 +656,6 @@ static int __devinit adis16400_probe(struct spi_device *spi)
> error_remove_trigger:
> if (st->indio_dev->modes & INDIO_RING_TRIGGERED)
> adis16400_remove_trigger(st->indio_dev);
> -error_unregister_line:
> - if (st->indio_dev->modes & INDIO_RING_TRIGGERED)
> - iio_unregister_interrupt_line(st->indio_dev, 0);
> error_uninitialize_ring:
> iio_ring_buffer_unregister(st->indio_dev->ring);
> error_unreg_ring_funcs:
> @@ -710,9 +689,6 @@ static int adis16400_remove(struct spi_device *spi)
> flush_scheduled_work();
>
> adis16400_remove_trigger(indio_dev);
> - if (spi->irq && gpio_is_valid(irq_to_gpio(spi->irq)) > 0)
> - iio_unregister_interrupt_line(indio_dev, 0);
> -
> iio_ring_buffer_unregister(st->indio_dev->ring);
> adis16400_unconfigure_ring(indio_dev);
> 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?
> 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);
> }
>
--
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
next prev parent reply other threads:[~2011-04-13 13:55 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-13 13:42 [PATCH 00/12] IIO: precursor cleanups to even system rewrite Jonathan Cameron
2011-04-13 13:42 ` [PATCH 01/12] staging:iio:tsl2563 take advantage of new iio_device_allocate private data Jonathan Cameron
2011-04-13 13:42 ` [PATCH 02/12] staging:iio:light:tsl2563 constify gain level table Jonathan Cameron
2011-04-13 13:42 ` [PATCH 03/12] staging:iio:adis16300 replace unnecessary event line registration Jonathan Cameron
2011-04-13 13:42 ` [PATCH 04/12] staging:iio:adis16350 " Jonathan Cameron
2011-04-13 13:43 ` [PATCH 05/12] staging:iio:adis16400 " Jonathan Cameron
2011-04-13 13:55 ` Michael Hennerich [this message]
2011-04-13 14:45 ` Jonathan Cameron
2011-04-17 18:41 ` Jonathan Cameron
2011-04-18 7:16 ` Hennerich, Michael
2011-04-13 13:43 ` [PATCH 06/12] staging:iio:adis16260 " Jonathan Cameron
2011-04-13 13:43 ` [PATCH 07/12] staging:iio:adis16203 " Jonathan Cameron
2011-04-13 13:43 ` [PATCH 08/12] staging:iio:adis16204 " Jonathan Cameron
2011-04-13 13:43 ` [PATCH 09/12] staging:iio:adis16201 " Jonathan Cameron
2011-04-13 13:43 ` [PATCH 10/12] staging:iio:adis16240 " Jonathan Cameron
2011-04-13 13:43 ` [PATCH 11/12] staging:iio:adis16209 " Jonathan Cameron
2011-04-13 13:43 ` [PATCH 12/12] staging:iio:ade7758 " Jonathan Cameron
2011-04-18 11:59 ` [PATCH 00/12] IIO: precursor cleanups to even system rewrite Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4DA5AB5F.2060000@analog.com \
--to=michael.hennerich@analog.com \
--cc=jic23@cam.ac.uk \
--cc=linux-iio@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox