From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <4DEDF4BB.3040900@analog.com> Date: Tue, 7 Jun 2011 11:51:55 +0200 From: Michael Hennerich Reply-To: MIME-Version: 1.0 To: Jonathan Cameron CC: "linux-iio@vger.kernel.org" , "device-drivers-devel@blackfin.uclinux.org" , Drivers Subject: Re: [PATCH] iio: trigger: Add filter callbac References: <1307389953-15467-1-git-send-email-michael.hennerich@analog.com> <4DEDE5C5.6080309@cam.ac.uk> In-Reply-To: <4DEDE5C5.6080309@cam.ac.uk> Content-Type: text/plain; charset="windows-1252"; format=flowed List-ID: On 06/07/2011 10:48 AM, Jonathan Cameron wrote: > On 06/06/11 20:52, michael.hennerich@analog.com wrote: >> From: Michael Hennerich >> >> Allow devices to reject triggers and vice versa. >> >> Changes since V1: >> Added kernel-doc >> Moved callback into iio_info >> Changed function naming >> Revised return value passing >> Included trigger.h to avoid warnings >> Moved definition of struct iio_poll_func to avoid warnings > This last change should be a separate patch. If nothing else > it ought to go into mainline asap, whereas the rest of this > isn't quite so obviously a fix. Also, what is the warning? > (nought showing up here) The warnings started with putting the callback into iio_info while it=20 didn't warn in iio_dev. drivers/staging/iio/adc/../iio.h:263: warning: =91struct iio_trigger=92=20 declared inside parameter list drivers/staging/iio/adc/../iio.h:263: warning: its scope is only this=20 definition or declaration, which is probably not what you want In file included from drivers/staging/iio/industrialio-trigger.c:19: So I included trigger.g in iio.h Then these warnings appeared - The question is not why those appear - this is quite obvious.. However why haven't we seen these before??? In file included from drivers/staging/iio/iio.h:18, from drivers/staging/iio/industrialio-core.c:24: drivers/staging/iio/trigger.h:101: warning: =91struct iio_poll_func=92=20 declared inside parameter list drivers/staging/iio/trigger.h:101: warning: its scope is only this=20 definition or declaration, which is probably not what you want drivers/staging/iio/trigger.h:110: warning: =91struct iio_poll_func=92=20 declared inside parameter list In file included from drivers/staging/iio/iio.h:18, from drivers/staging/iio/industrialio-trigger.c:19: drivers/staging/iio/trigger.h:101: warning: =91struct iio_poll_func=92=20 declared inside parameter list drivers/staging/iio/trigger.h:101: warning: its scope is only this=20 definition or declaration, which is probably not what you want drivers/staging/iio/trigger.h:110: warning: =91struct iio_poll_func=92=20 declared inside parameter list drivers/staging/iio/industrialio-trigger.c:226: error: conflicting types=20 for =91iio_trigger_attach_poll_func=92 drivers/staging/iio/trigger.h:100: error: previous declaration of=20 =91iio_trigger_attach_poll_func=92 was here drivers/staging/iio/industrialio-trigger.c:242: error: conflicting types=20 for =91iio_trigger_attach_poll_func=92 drivers/staging/iio/trigger.h:100: error: previous declaration of=20 =91iio_trigger_attach_poll_func=92 was here drivers/staging/iio/industrialio-trigger.c:244: error: conflicting types=20 for =91iio_trigger_dettach_poll_func=92 drivers/staging/iio/trigger.h:109: error: previous declaration of=20 =91iio_trigger_dettach_poll_func=92 was here drivers/staging/iio/industrialio-trigger.c:263: error: conflicting types=20 for =91iio_trigger_dettach_poll_func=92 drivers/staging/iio/trigger.h:109: error: previous declaration of=20 =91iio_trigger_dettach_poll_func=92 was here make[4]: *** [drivers/staging/iio/industrialio-trigger.o] Error 1 > Hence, if you have time please break that bit out. Ack is > for both patches if you do this. > > Otherwise, all looks good to me - I'll pull into my local > tree to rebase the trigger cleanups on top, but feel free > to push to Greg as well. I'm waiting on a few tests of non > ADI parts before pushing out the dev_data removal set. > > The addition of these functions would also be slightly nicer > if in a set with a patch that actually uses them. Well - AD7793 driver I'm going to send out for another round of review=20 will use them. > Thanks, > > Jonathan >> Signed-off-by: Michael Hennerich > Acked-by: Jonathan Cameron >> --- >> drivers/staging/iio/iio.h | 6 +++ >> drivers/staging/iio/industrialio-trigger.c | 20 ++++++++++- >> drivers/staging/iio/trigger.h | 52 +++++++++++++++---= ---------- >> 3 files changed, 53 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h >> index 78a0927..9bf8b11 100644 >> --- a/drivers/staging/iio/iio.h >> +++ b/drivers/staging/iio/iio.h >> @@ -15,6 +15,7 @@ >> #include >> #include "sysfs.h" >> #include "chrdev.h" >> +#include "trigger.h" >> >> /* IIO TODO LIST */ >> /* >> @@ -224,6 +225,8 @@ static inline s64 iio_get_time_ns(void) >> * is event dependant. event_code specifies which = event. >> * @write_event_value: write the value associate with the even= t. >> * Meaning is event dependent. >> + * @validate_trigger: function to validate the trigger when th= e >> + * current trigger gets changed. >> **/ >> struct iio_info { >> struct module *driver_module; >> @@ -256,6 +259,9 @@ struct iio_info { >> int (*write_event_value)(struct iio_dev *indio_dev, >> int event_code, >> int val); >> + int (*validate_trigger)(struct iio_dev *indio_dev, >> + struct iio_trigger *trig); >> + >> }; >> >> /** >> diff --git a/drivers/staging/iio/industrialio-trigger.c b/drivers/stag= ing/iio/industrialio-trigger.c >> index d504aa2..90ca2df 100644 >> --- a/drivers/staging/iio/industrialio-trigger.c >> +++ b/drivers/staging/iio/industrialio-trigger.c >> @@ -340,6 +340,9 @@ static ssize_t iio_trigger_write_current(struct de= vice *dev, >> { >> struct iio_dev *dev_info =3D dev_get_drvdata(dev); >> struct iio_trigger *oldtrig =3D dev_info->trig; >> + struct iio_trigger *trig; >> + int ret; >> + >> mutex_lock(&dev_info->mlock); >> if (dev_info->currentmode =3D=3D INDIO_RING_TRIGGERED) { >> mutex_unlock(&dev_info->mlock); >> @@ -347,7 +350,22 @@ static ssize_t iio_trigger_write_current(struct d= evice *dev, >> } >> mutex_unlock(&dev_info->mlock); >> >> - dev_info->trig =3D iio_trigger_find_by_name(buf, len); >> + trig =3D iio_trigger_find_by_name(buf, len); >> + >> + if (trig&& dev_info->info->validate_trigger) { >> + ret =3D dev_info->info->validate_trigger(dev_info, trig)= ; >> + if (ret) >> + return ret; >> + } >> + >> + if (trig&& trig->validate_device) { >> + ret =3D trig->validate_device(trig, dev_info); >> + if (ret) >> + return ret; >> + } >> + >> + dev_info->trig =3D trig; >> + >> if (oldtrig&& dev_info->trig !=3D oldtrig) >> iio_put_trigger(oldtrig); >> if (dev_info->trig) >> diff --git a/drivers/staging/iio/trigger.h b/drivers/staging/iio/trigg= er.h >> index f329fe1..e0b58ed 100644 >> --- a/drivers/staging/iio/trigger.h >> +++ b/drivers/staging/iio/trigger.h >> @@ -29,6 +29,8 @@ struct iio_subirq { >> * @set_trigger_state: [DRIVER] switch on/off the trigger on d= emand >> * @try_reenable: function to reenable the trigger when the >> * use count is zero (may be NULL) >> + * @validate_device: function to validate the device when the >> + * current trigger gets changed. >> * @subirq_chip: [INTERN] associate 'virtual' irq chip. >> * @subirq_base: [INTERN] base number for irqs provided by trigg= er. >> * @subirqs: [INTERN] information about the 'child' irqs. >> @@ -48,6 +50,8 @@ struct iio_trigger { >> >> int (*set_trigger_state)(struct iio_trigger *trig, bool state); >> int (*try_reenable)(struct iio_trigger *trig); >> + int (*validate_device)(struct iio_trigger *trig, >> + struct iio_dev *indio_dev); >> >> struct irq_chip subirq_chip; >> int subirq_base; >> @@ -57,6 +61,30 @@ struct iio_trigger { >> struct mutex pool_lock; >> }; >> >> +/** >> + * struct iio_poll_func - poll function pair >> + * >> + * @private_data: data specific to device (passed into pol= l func) >> + * @h: the function that is actually ru= n on trigger >> + * @thread: threaded interrupt part >> + * @type: the type of interrupt (basically if ones= hot) >> + * @name: name used to identify the trigger consum= er. >> + * @irq: the corresponding irq as allocated from = the >> + * trigger pool >> + * @timestamp: some devices need a timestamp gr= abbed as soon >> + * as possible after the trigger - hence ha= ndler >> + * passes it via here. >> + **/ >> +struct iio_poll_func { >> + void *private_data; >> + irqreturn_t (*h)(int irq, void *p); >> + irqreturn_t (*thread)(int irq, void *p); >> + int type; >> + char *name; >> + int irq; >> + s64 timestamp; >> +}; >> + >> static inline struct iio_trigger *to_iio_trigger(struct device *d) >> { >> return container_of(d, struct iio_trigger, dev); >> @@ -136,30 +164,6 @@ static inline void iio_trigger_put_irq(struct iio= _trigger *trig, int irq) >> mutex_unlock(&trig->pool_lock); >> }; >> >> -/** >> - * struct iio_poll_func - poll function pair >> - * >> - * @private_data: data specific to device (passed into pol= l func) >> - * @h: the function that is actually ru= n on trigger >> - * @thread: threaded interrupt part >> - * @type: the type of interrupt (basically if ones= hot) >> - * @name: name used to identify the trigger consum= er. >> - * @irq: the corresponding irq as allocated from = the >> - * trigger pool >> - * @timestamp: some devices need a timestamp gr= abbed as soon >> - * as possible after the trigger - hence ha= ndler >> - * passes it via here. >> - **/ >> -struct iio_poll_func { >> - void *private_data; >> - irqreturn_t (*h)(int irq, void *p); >> - irqreturn_t (*thread)(int irq, void *p); >> - int type; >> - char *name; >> - int irq; >> - s64 timestamp; >> -}; >> - >> struct iio_poll_func >> *iio_alloc_pollfunc(irqreturn_t (*h)(int irq, void *p), >> irqreturn_t (*thread)(int irq, void *p), > --=20 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