Re: [PATCH] iio: trigger: Add filter callbac

[Michael Hennerich <michael.hennerich@analog.com>]

On 06/07/2011 10:48 AM, Jonathan Cameron wrote:
> On 06/06/11 20:52, michael.hennerich@analog.com wrote:
>> From: Michael Hennerich<michael.hennerich@analog.com>
>>
>> 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<michael.hennerich@analog.com>
> Acked-by: Jonathan Cameron<jic23@cam.ac.uk>
>> ---
>>   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 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