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

[Jonathan Cameron <jic23@cam.ac.uk>]

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<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=
 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<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 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