From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ppsw-41.csi.cam.ac.uk ([131.111.8.141]:54734 "EHLO ppsw-41.csi.cam.ac.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751425Ab1FGQRV (ORCPT ); Tue, 7 Jun 2011 12:17:21 -0400 Message-ID: <4DEE50F5.6030409@cam.ac.uk> Date: Tue, 07 Jun 2011 17:25:25 +0100 From: Jonathan Cameron MIME-Version: 1.0 To: michael.hennerich@analog.com 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> <4DEDF4BB.3040900@analog.com> In-Reply-To: <4DEDF4BB.3040900@analog.com> Content-Type: text/plain; charset=windows-1252 Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.kernel.org On 06/07/11 10:51, Michael Hennerich wrote: > 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= didn't warn in iio_dev. >=20 > drivers/staging/iio/adc/../iio.h:263: warning: =91struct iio_trigger=92= declared inside parameter list > drivers/staging/iio/adc/../iio.h:263: warning: its scope is only this= definition or declaration, which is probably not what you want > In file included from drivers/staging/iio/industrialio-trigger.c:19: >=20 > So I included trigger.g in iio.h =46irstly, please don't do that. I'd like to keep triggers as separate = and opaque to simple drivers as possible. Hence just put a struct iio_trigger; somewhere ab= ove where it is needed. >=20 > Then these warnings appeared - > The question is not why those appear - this is quite obvious.. > However why haven't we seen these before??? Weird indeed. Please do fix them, but in a separate patch from the one= adding filtering. You can trigger the same warnings by putting iio.h after trigger.h in a= ny of the drivers. Given the wonders of cut and paste that never happened before. Someone= more bored than me can figure out what wonderful route meant these were defined wh= en used... >=20 >=20 > 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= declared inside parameter list > drivers/staging/iio/trigger.h:101: warning: its scope is only this de= finition or declaration, which is probably not what you want > drivers/staging/iio/trigger.h:110: warning: =91struct iio_poll_func=92= declared inside parameter list >=20 > 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= declared inside parameter list > drivers/staging/iio/trigger.h:101: warning: its scope is only this de= finition or declaration, which is probably not what you want > drivers/staging/iio/trigger.h:110: warning: =91struct iio_poll_func=92= declared inside parameter list > drivers/staging/iio/industrialio-trigger.c:226: error: conflicting ty= pes for =91iio_trigger_attach_poll_func=92 > drivers/staging/iio/trigger.h:100: error: previous declaration of =91= iio_trigger_attach_poll_func=92 was here > drivers/staging/iio/industrialio-trigger.c:242: error: conflicting ty= pes for =91iio_trigger_attach_poll_func=92 > drivers/staging/iio/trigger.h:100: error: previous declaration of =91= iio_trigger_attach_poll_func=92 was here > drivers/staging/iio/industrialio-trigger.c:244: error: conflicting ty= pes for =91iio_trigger_dettach_poll_func=92 > drivers/staging/iio/trigger.h:109: error: previous declaration of =91= iio_trigger_dettach_poll_func=92 was here > drivers/staging/iio/industrialio-trigger.c:263: error: conflicting ty= pes for =91iio_trigger_dettach_poll_func=92 > drivers/staging/iio/trigger.h:109: error: previous declaration of =91= iio_trigger_dettach_poll_func=92 was here > make[4]: *** [drivers/staging/iio/industrialio-trigger.o] Error 1 >=20 >=20 >> 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 revie= w will use them. >=20 >> 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 whi= ch event. >>> * @write_event_value: write the value associate with the e= vent. >>> * Meaning is event dependent. >>> + * @validate_trigger: function to validate the trigger when= the >>> + * 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/s= taging/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= device *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(struc= t device *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, tr= ig); >>> + 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/tr= igger.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 o= n demand >>> * @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 tr= igger. >>> * @subirqs: [INTERN] information about the 'child' irqs. >>> @@ -48,6 +50,8 @@ struct iio_trigger { >>> >>> int (*set_trigger_state)(struct iio_trigger *trig, bool stat= e); >>> 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 = poll func) >>> + * @h: the function that is actually= run on trigger >>> + * @thread: threaded interrupt part >>> + * @type: the type of interrupt (basically if o= neshot) >>> + * @name: name used to identify the trigger con= sumer. >>> + * @irq: the corresponding irq as allocated fr= om the >>> + * trigger pool >>> + * @timestamp: some devices need a timestamp= grabbed as soon >>> + * as possible after the trigger - hence= handler >>> + * 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 = poll func) >>> - * @h: the function that is actually= run on trigger >>> - * @thread: threaded interrupt part >>> - * @type: the type of interrupt (basically if o= neshot) >>> - * @name: name used to identify the trigger con= sumer. >>> - * @irq: the corresponding irq as allocated fr= om the >>> - * trigger pool >>> - * @timestamp: some devices need a timestamp= grabbed as soon >>> - * as possible after the trigger - hence= handler >>> - * 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 >=20