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

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

On 06/06/11 12:56, Hennerich, Michael wrote:
> Jonathan Cameron wrote on 2011-06-06:
>> On 06/06/11 12:40, Jonathan Cameron wrote:
>>> On 06/06/11 12:01, michael.hennerich@analog.com wrote:
>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>
>>>> Allow devices to reject triggers and vice versa.
>>> The iio_trigger changes clash with a cleanup I just finished.
>>> What mine is doing is adding an iio_trigger_ops structure which has
>>> all of the per driver constant stuff (.owner and callbacks) lifted
>>> from struct iio_trigger.
>>>
>>> I'll apply yours before mine and update those changes before posting.
>>>
>>> Basically what you have here is sound.  The bit that was in your
>>> earlier patch will get eaten up by git anyway so that doesn't really
>>> matter.
>>>
>>> Please adjust so as to pass back return values from the callbacks in
>>> the error case.
>>
>> Actually, one more thing.  iio_validate_trigger in iio_dev is I guess
>> constant per driver? (or at least a set of parts supported by a driver?)
> 
> sure
> 
>> If so, please put it in iio_info rather than iio_dev.
>>
>> Lets keep as much constant as possible!
>>
>> For that matter, they could both do with documenting in the kernel-doc
>> for the structures as well.
> 
> Are you asking for an follow up patch?
Easier just to roll it into this one.  I'm only talking about documenting
these in the headers.  We can deal with our main documentation at some later
point.
> 
>> Jonathan
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>>
>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>> ---
>>>>  drivers/staging/iio/iio.h                  |    2 ++
>>>>  drivers/staging/iio/industrialio-trigger.c |   16 +++++++++++++++-
>>>>  drivers/staging/iio/trigger.h              |    2 ++
>>>>  3 files changed, 19 insertions(+), 1 deletions(-)
>>>> diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
>>>> index 78a0927..8ec4f41 100644
>>>> --- a/drivers/staging/iio/iio.h
>>>> +++ b/drivers/staging/iio/iio.h
>>>> @@ -293,6 +293,8 @@ struct iio_dev {
>>>>
>>>>     u32                             *available_scan_masks;
>>>>     struct iio_trigger              *trig;
>>>> +   int (*iio_validate_trigger)     (struct iio_dev *indio_dev,
>>>> +                                    struct iio_trigger *trig);
>>>>     struct iio_poll_func            *pollfunc;
>>>>
>>>>     struct iio_chan_spec const *channels; diff --git
>>>> a/drivers/staging/iio/industrialio-trigger.c
>>>> b/drivers/staging/iio/industrialio-trigger.c
>>>> index 6159023..1f6bc65 100644
>>>> --- a/drivers/staging/iio/industrialio-trigger.c
>>>> +++ b/drivers/staging/iio/industrialio-trigger.c
>>>> @@ -294,6 +294,7 @@ struct iio_poll_func
>>>>     pf->h = h;
>>>>     pf->thread = thread;
>>>>     pf->type = type;
>>>> +   pf->private_data = private;
>>> That's already in the patch you sent to Greg last week. I'm holding
>>> that one in my local tree, and I'll guess Greg will pick it up and
>>> push to Linus next time he is working with the staging tree.
>>>
>>>>
>>>>     return pf;
>>>>  }
>>>> @@ -339,6 +340,8 @@ 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;
>>>> +
>>>>     mutex_lock(&dev_info->mlock);
>>>>     if (dev_info->currentmode == INDIO_RING_TRIGGERED) {
>>>>             mutex_unlock(&dev_info->mlock);
>>>> @@ -346,7 +349,18 @@ 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->iio_validate_trigger &&
>>>> +           dev_info->iio_validate_trigger(dev_info, trig)) +               return -EINVAL;
>>>> I'd rather see error returned from the callback passed on. + +      if
>>>> (trig && trig->iio_validate_device &&
>>>> +           trig->iio_validate_device(trig, dev_info)) +            return -EINVAL; same
>>>> here. + +   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..9218200 100644
>>>> --- a/drivers/staging/iio/trigger.h
>>>> +++ b/drivers/staging/iio/trigger.h
>>>> @@ -48,6 +48,8 @@ struct iio_trigger {
>>>>
>>>>     int (*set_trigger_state)(struct iio_trigger *trig, bool state);
>>>>     int (*try_reenable)(struct iio_trigger *trig);
>>>> +   int (*iio_validate_device)(struct iio_trigger *trig,
>>>> +                              struct iio_dev *indio_dev);
>>>>
>>>>     struct irq_chip                 subirq_chip;
>>>>     int                             subirq_base;
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>> info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> 
> 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
> 
> 
> 
>