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@xxxxxxxxxx wrote: >>> From: Michael Hennerich<michael.hennerich@xxxxxxxxxx> >>> >>> 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. > > drivers/staging/iio/adc/../iio.h:263: warning: ‘struct iio_trigger’ 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: > > So I included trigger.g in iio.h Firstly, 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 above where it is needed. > > 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 any 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 when used... > > > 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: ‘struct iio_poll_func’ declared inside parameter list > drivers/staging/iio/trigger.h:101: warning: its scope is only this definition or declaration, which is probably not what you want > drivers/staging/iio/trigger.h:110: warning: ‘struct iio_poll_func’ 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: ‘struct iio_poll_func’ declared inside parameter list > drivers/staging/iio/trigger.h:101: warning: its scope is only this definition or declaration, which is probably not what you want > drivers/staging/iio/trigger.h:110: warning: ‘struct iio_poll_func’ declared inside parameter list > drivers/staging/iio/industrialio-trigger.c:226: error: conflicting types for ‘iio_trigger_attach_poll_func’ > drivers/staging/iio/trigger.h:100: error: previous declaration of ‘iio_trigger_attach_poll_func’ was here > drivers/staging/iio/industrialio-trigger.c:242: error: conflicting types for ‘iio_trigger_attach_poll_func’ > drivers/staging/iio/trigger.h:100: error: previous declaration of ‘iio_trigger_attach_poll_func’ was here > drivers/staging/iio/industrialio-trigger.c:244: error: conflicting types for ‘iio_trigger_dettach_poll_func’ > drivers/staging/iio/trigger.h:109: error: previous declaration of ‘iio_trigger_dettach_poll_func’ was here > drivers/staging/iio/industrialio-trigger.c:263: error: conflicting types for ‘iio_trigger_dettach_poll_func’ > drivers/staging/iio/trigger.h:109: error: previous declaration of ‘iio_trigger_dettach_poll_func’ 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 will use them. > >> Thanks, >> >> Jonathan >>> Signed-off-by: Michael Hennerich<michael.hennerich@xxxxxxxxxx> >> Acked-by: Jonathan Cameron<jic23@xxxxxxxxx> >>> --- >>> 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<linux/irq.h> >>> #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 event. >>> * 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/staging/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 = dev_get_drvdata(dev); >>> struct iio_trigger *oldtrig = dev_info->trig; >>> + struct iio_trigger *trig; >>> + int ret; >>> + >>> mutex_lock(&dev_info->mlock); >>> if (dev_info->currentmode == INDIO_RING_TRIGGERED) { >>> mutex_unlock(&dev_info->mlock); >>> @@ -347,7 +350,22 @@ static ssize_t iio_trigger_write_current(struct device *dev, >>> } >>> mutex_unlock(&dev_info->mlock); >>> >>> - dev_info->trig = iio_trigger_find_by_name(buf, len); >>> + trig = iio_trigger_find_by_name(buf, len); >>> + >>> + if (trig&& dev_info->info->validate_trigger) { >>> + ret = dev_info->info->validate_trigger(dev_info, trig); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + if (trig&& trig->validate_device) { >>> + ret = trig->validate_device(trig, dev_info); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + dev_info->trig = trig; >>> + >>> if (oldtrig&& dev_info->trig != oldtrig) >>> iio_put_trigger(oldtrig); >>> if (dev_info->trig) >>> diff --git a/drivers/staging/iio/trigger.h b/drivers/staging/iio/trigger.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 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 trigger. >>> * @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 poll func) >>> + * @h: the function that is actually run on trigger >>> + * @thread: threaded interrupt part >>> + * @type: the type of interrupt (basically if oneshot) >>> + * @name: name used to identify the trigger consumer. >>> + * @irq: the corresponding irq as allocated from 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 oneshot) >>> - * @name: name used to identify the trigger consumer. >>> - * @irq: the corresponding irq as allocated from 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), >> > > -- 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