Re: [PATCH v13 11/25] v4l: async: Introduce helpers for calling async ops callbacks

2017-09-20 Thread Sakari Ailus 
Hi Laurent,

On Tue, Sep 19, 2017 at 07:27:17PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday, 19 September 2017 17:50:49 EEST Sakari Ailus wrote:
> > On Tue, Sep 19, 2017 at 03:43:48PM +0300, Laurent Pinchart wrote:
> > > On Tuesday, 19 September 2017 15:13:11 EEST Sakari Ailus wrote:
> > >> On Tue, Sep 19, 2017 at 03:01:14PM +0300, Laurent Pinchart wrote:
> > >>> On Friday, 15 September 2017 17:17:10 EEST Sakari Ailus wrote:
> >  Add three helper functions to call async operations callbacks.
> >  Besides simplifying callbacks, this allows async notifiers to have no
> >  ops set, i.e. it can be left NULL.
> >  
> >  Signed-off-by: Sakari Ailus 
> >  Acked-by: Hans Verkuil 
> >  ---
> >  
> >   drivers/media/v4l2-core/v4l2-async.c | 49 ++
> >   include/media/v4l2-async.h   |  1 +
> >   2 files changed, 37 insertions(+), 13 deletions(-)
> >  
> >  diff --git a/drivers/media/v4l2-core/v4l2-async.c
> >  b/drivers/media/v4l2-core/v4l2-async.c index
> >  7b2125b3d62f..c35d04b9122f
> >  100644
> >  --- a/drivers/media/v4l2-core/v4l2-async.c
> >  +++ b/drivers/media/v4l2-core/v4l2-async.c
> >  @@ -25,6 +25,34 @@
> >  
> >   #include 
> >   #include 
> >  
> >  +static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier
> >  *n,
> >  +struct v4l2_subdev *subdev,
> >  +struct v4l2_async_subdev *asd)
> >  +{
> >  +  if (!n->ops || !n->ops->bound)
> >  +  return 0;
> >  +
> >  +  return n->ops->bound(n, subdev, asd);
> >  +}
> >  +
> >  +static void v4l2_async_notifier_call_unbind(struct
> >  v4l2_async_notifier
> >  *n,
> >  +  struct v4l2_subdev *subdev,
> >  +  struct v4l2_async_subdev 
> >  *asd)
> >  +{
> >  +  if (!n->ops || !n->ops->unbind)
> >  +  return;
> >  +
> >  +  n->ops->unbind(n, subdev, asd);
> >  +}
> >  +
> >  +static int v4l2_async_notifier_call_complete(struct
> >  v4l2_async_notifier
> >  *n)
> >  +{
> >  +  if (!n->ops || !n->ops->complete)
> >  +  return 0;
> >  +
> >  +  return n->ops->complete(n);
> >  +}
> >  +
> > >>> 
> > >>> Wouldn't it be enough to add a single v4l2_async_notifier_call() macro
> > >>> ?
> > >>> 
> > >>> #define v4l2_async_notifier_call(n, op, args...) \
> > >>> 
> > >>> ((n)->ops && (n)->ops->op ? (n)->ops->op(n, ##args) : 0)
> > >> 
> > >> I actually had that in an earlier version but I changed it based on
> > >> review comments from Hans. A single macro isn't enough: some functions
> > >> have int return type. I think the way it is now is nicer.
> > > 
> > > What bothers me there is the overhead of a function call.
> > 
> > Overhead... of a function call?
> > 
> > Do you really mean what you're saying? :-) The functions will be called a
> > relatively small number of times during module loading / device probing.
> 
> That's why I haven't said it's a big deal :-) There's of course no need to 
> optimize that if the tradeoff is large, but if all operations had the same 
> return type a macro could have been useful (although in this very specific 
> case I'm more concerned about code size than about CPU cycles).

Code size in the async framework? Generally calling a function doesn't take
a lot of code, and the kernel is, well, full of function calls. I'm frankly
more concerned about the number of lines of code to maintain and
readability of that code.

> 
> > > By the way, what's the use case for ops being NULL ?
> > 
> > If a driver has no need for any of the callbacks, there's no benefit from
> > having to set ops struct either. This applies to devices that are
> > associated to the sensor, for instance.
> 
> So in that case the subdev notifier is only registered to delay the 
> complete() 
> callback until the flash and lens controllers are available, with the sensor 
> itself having no need to access the flash and lens controllers ?

Essentially yes. In the future we'll need to make use of the association
information to tell which devices are related but this shouldn't be a job
for individual drivers.

-- 
Regards,

Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH v13 11/25] v4l: async: Introduce helpers for calling async ops callbacks

2017-09-19 Thread Laurent Pinchart 
Hi Sakari,

On Tuesday, 19 September 2017 17:50:49 EEST Sakari Ailus wrote:
> On Tue, Sep 19, 2017 at 03:43:48PM +0300, Laurent Pinchart wrote:
> > On Tuesday, 19 September 2017 15:13:11 EEST Sakari Ailus wrote:
> >> On Tue, Sep 19, 2017 at 03:01:14PM +0300, Laurent Pinchart wrote:
> >>> On Friday, 15 September 2017 17:17:10 EEST Sakari Ailus wrote:
>  Add three helper functions to call async operations callbacks.
>  Besides simplifying callbacks, this allows async notifiers to have no
>  ops set, i.e. it can be left NULL.
>  
>  Signed-off-by: Sakari Ailus 
>  Acked-by: Hans Verkuil 
>  ---
>  
>   drivers/media/v4l2-core/v4l2-async.c | 49 ++
>   include/media/v4l2-async.h   |  1 +
>   2 files changed, 37 insertions(+), 13 deletions(-)
>  
>  diff --git a/drivers/media/v4l2-core/v4l2-async.c
>  b/drivers/media/v4l2-core/v4l2-async.c index
>  7b2125b3d62f..c35d04b9122f
>  100644
>  --- a/drivers/media/v4l2-core/v4l2-async.c
>  +++ b/drivers/media/v4l2-core/v4l2-async.c
>  @@ -25,6 +25,34 @@
>  
>   #include 
>   #include 
>  
>  +static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier
>  *n,
>  +  struct v4l2_subdev *subdev,
>  +  struct v4l2_async_subdev *asd)
>  +{
>  +if (!n->ops || !n->ops->bound)
>  +return 0;
>  +
>  +return n->ops->bound(n, subdev, asd);
>  +}
>  +
>  +static void v4l2_async_notifier_call_unbind(struct
>  v4l2_async_notifier
>  *n,
>  +struct v4l2_subdev *subdev,
>  +struct v4l2_async_subdev 
>  *asd)
>  +{
>  +if (!n->ops || !n->ops->unbind)
>  +return;
>  +
>  +n->ops->unbind(n, subdev, asd);
>  +}
>  +
>  +static int v4l2_async_notifier_call_complete(struct
>  v4l2_async_notifier
>  *n)
>  +{
>  +if (!n->ops || !n->ops->complete)
>  +return 0;
>  +
>  +return n->ops->complete(n);
>  +}
>  +
> >>> 
> >>> Wouldn't it be enough to add a single v4l2_async_notifier_call() macro
> >>> ?
> >>> 
> >>> #define v4l2_async_notifier_call(n, op, args...) \
> >>> 
> >>>   ((n)->ops && (n)->ops->op ? (n)->ops->op(n, ##args) : 0)
> >> 
> >> I actually had that in an earlier version but I changed it based on
> >> review comments from Hans. A single macro isn't enough: some functions
> >> have int return type. I think the way it is now is nicer.
> > 
> > What bothers me there is the overhead of a function call.
> 
> Overhead... of a function call?
> 
> Do you really mean what you're saying? :-) The functions will be called a
> relatively small number of times during module loading / device probing.

That's why I haven't said it's a big deal :-) There's of course no need to 
optimize that if the tradeoff is large, but if all operations had the same 
return type a macro could have been useful (although in this very specific 
case I'm more concerned about code size than about CPU cycles).

> > By the way, what's the use case for ops being NULL ?
> 
> If a driver has no need for any of the callbacks, there's no benefit from
> having to set ops struct either. This applies to devices that are
> associated to the sensor, for instance.

So in that case the subdev notifier is only registered to delay the complete() 
callback until the flash and lens controllers are available, with the sensor 
itself having no need to access the flash and lens controllers ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 11/25] v4l: async: Introduce helpers for calling async ops callbacks

2017-09-19 Thread Sakari Ailus 
Hi Laurent,

On Tue, Sep 19, 2017 at 03:43:48PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday, 19 September 2017 15:13:11 EEST Sakari Ailus wrote:
> > On Tue, Sep 19, 2017 at 03:01:14PM +0300, Laurent Pinchart wrote:
> > > On Friday, 15 September 2017 17:17:10 EEST Sakari Ailus wrote:
> > >> Add three helper functions to call async operations callbacks. Besides
> > >> simplifying callbacks, this allows async notifiers to have no ops set,
> > >> i.e. it can be left NULL.
> > >> 
> > >> Signed-off-by: Sakari Ailus 
> > >> Acked-by: Hans Verkuil 
> > >> ---
> > >> 
> > >>  drivers/media/v4l2-core/v4l2-async.c | 49 +
> > >>  include/media/v4l2-async.h   |  1 +
> > >>  2 files changed, 37 insertions(+), 13 deletions(-)
> > >> 
> > >> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > >> b/drivers/media/v4l2-core/v4l2-async.c index 7b2125b3d62f..c35d04b9122f
> > >> 100644
> > >> --- a/drivers/media/v4l2-core/v4l2-async.c
> > >> +++ b/drivers/media/v4l2-core/v4l2-async.c
> > >> @@ -25,6 +25,34 @@
> > >> 
> > >>  #include 
> > >>  #include 
> > >> 
> > >> +static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier
> > >> *n,
> > >> +  struct v4l2_subdev *subdev,
> > >> +  struct v4l2_async_subdev *asd)
> > >> +{
> > >> +if (!n->ops || !n->ops->bound)
> > >> +return 0;
> > >> +
> > >> +return n->ops->bound(n, subdev, asd);
> > >> +}
> > >> +
> > >> +static void v4l2_async_notifier_call_unbind(struct v4l2_async_notifier
> > >> *n,
> > >> +struct v4l2_subdev *subdev,
> > >> +struct v4l2_async_subdev 
> > >> *asd)
> > >> +{
> > >> +if (!n->ops || !n->ops->unbind)
> > >> +return;
> > >> +
> > >> +n->ops->unbind(n, subdev, asd);
> > >> +}
> > >> +
> > >> +static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier
> > >> *n)
> > >> +{
> > >> +if (!n->ops || !n->ops->complete)
> > >> +return 0;
> > >> +
> > >> +return n->ops->complete(n);
> > >> +}
> > >> +
> > > 
> > > Wouldn't it be enough to add a single v4l2_async_notifier_call() macro ?
> > > 
> > > #define v4l2_async_notifier_call(n, op, args...) \
> > > 
> > >   ((n)->ops && (n)->ops->op ? (n)->ops->op(n, ##args) : 0)
> > 
> > I actually had that in an earlier version but I changed it based on review
> > comments from Hans. A single macro isn't enough: some functions have int
> > return type. I think the way it is now is nicer.
> 
> What bothers me there is the overhead of a function call.

Overhead... of a function call?

Do you really mean what you're saying? :-) The functions will be called a
relatively small number of times during module loading / device probing.

> 
> By the way, what's the use case for ops being NULL ?

If a driver has no need for any of the callbacks, there's no benefit from
having to set ops struct either. This applies to devices that are
associated to the sensor, for instance.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v13 11/25] v4l: async: Introduce helpers for calling async ops callbacks

2017-09-19 Thread Laurent Pinchart 
Hi Sakari,

On Tuesday, 19 September 2017 15:13:11 EEST Sakari Ailus wrote:
> On Tue, Sep 19, 2017 at 03:01:14PM +0300, Laurent Pinchart wrote:
> > On Friday, 15 September 2017 17:17:10 EEST Sakari Ailus wrote:
> >> Add three helper functions to call async operations callbacks. Besides
> >> simplifying callbacks, this allows async notifiers to have no ops set,
> >> i.e. it can be left NULL.
> >> 
> >> Signed-off-by: Sakari Ailus 
> >> Acked-by: Hans Verkuil 
> >> ---
> >> 
> >>  drivers/media/v4l2-core/v4l2-async.c | 49 +
> >>  include/media/v4l2-async.h   |  1 +
> >>  2 files changed, 37 insertions(+), 13 deletions(-)
> >> 
> >> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> >> b/drivers/media/v4l2-core/v4l2-async.c index 7b2125b3d62f..c35d04b9122f
> >> 100644
> >> --- a/drivers/media/v4l2-core/v4l2-async.c
> >> +++ b/drivers/media/v4l2-core/v4l2-async.c
> >> @@ -25,6 +25,34 @@
> >> 
> >>  #include 
> >>  #include 
> >> 
> >> +static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier
> >> *n,
> >> +struct v4l2_subdev *subdev,
> >> +struct v4l2_async_subdev *asd)
> >> +{
> >> +  if (!n->ops || !n->ops->bound)
> >> +  return 0;
> >> +
> >> +  return n->ops->bound(n, subdev, asd);
> >> +}
> >> +
> >> +static void v4l2_async_notifier_call_unbind(struct v4l2_async_notifier
> >> *n,
> >> +  struct v4l2_subdev *subdev,
> >> +  struct v4l2_async_subdev *asd)
> >> +{
> >> +  if (!n->ops || !n->ops->unbind)
> >> +  return;
> >> +
> >> +  n->ops->unbind(n, subdev, asd);
> >> +}
> >> +
> >> +static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier
> >> *n)
> >> +{
> >> +  if (!n->ops || !n->ops->complete)
> >> +  return 0;
> >> +
> >> +  return n->ops->complete(n);
> >> +}
> >> +
> > 
> > Wouldn't it be enough to add a single v4l2_async_notifier_call() macro ?
> > 
> > #define v4l2_async_notifier_call(n, op, args...) \
> > 
> > ((n)->ops && (n)->ops->op ? (n)->ops->op(n, ##args) : 0)
> 
> I actually had that in an earlier version but I changed it based on review
> comments from Hans. A single macro isn't enough: some functions have int
> return type. I think the way it is now is nicer.

What bothers me there is the overhead of a function call.

By the way, what's the use case for ops being NULL ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 11/25] v4l: async: Introduce helpers for calling async ops callbacks

2017-09-19 Thread Sakari Ailus 
Hi Laurent,

On Tue, Sep 19, 2017 at 03:01:14PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 15 September 2017 17:17:10 EEST Sakari Ailus wrote:
> > Add three helper functions to call async operations callbacks. Besides
> > simplifying callbacks, this allows async notifiers to have no ops set,
> > i.e. it can be left NULL.
> > 
> > Signed-off-by: Sakari Ailus 
> > Acked-by: Hans Verkuil 
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 49 
> >  include/media/v4l2-async.h   |  1 +
> >  2 files changed, 37 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c index 7b2125b3d62f..c35d04b9122f
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -25,6 +25,34 @@
> >  #include 
> >  #include 
> > 
> > +static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier *n,
> > + struct v4l2_subdev *subdev,
> > + struct v4l2_async_subdev *asd)
> > +{
> > +   if (!n->ops || !n->ops->bound)
> > +   return 0;
> > +
> > +   return n->ops->bound(n, subdev, asd);
> > +}
> > +
> > +static void v4l2_async_notifier_call_unbind(struct v4l2_async_notifier *n,
> > +   struct v4l2_subdev *subdev,
> > +   struct v4l2_async_subdev *asd)
> > +{
> > +   if (!n->ops || !n->ops->unbind)
> > +   return;
> > +
> > +   n->ops->unbind(n, subdev, asd);
> > +}
> > +
> > +static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier *n)
> > +{
> > +   if (!n->ops || !n->ops->complete)
> > +   return 0;
> > +
> > +   return n->ops->complete(n);
> > +}
> > +
> 
> Wouldn't it be enough to add a single v4l2_async_notifier_call() macro ?
> 
> #define v4l2_async_notifier_call(n, op, args...) \
>   ((n)->ops && (n)->ops->op ? (n)->ops->op(n, ##args) : 0)

I actually had that in an earlier version but I changed it based on review
comments from Hans. A single macro isn't enough: some functions have int
return type. I think the way it is now is nicer.

> 
> Apart from that,
> 
> Reviewed-by: Laurent Pinchart 
> 
> >  static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev
> > *asd) {
> >  #if IS_ENABLED(CONFIG_I2C)
> > @@ -102,16 +130,13 @@ static int v4l2_async_match_notify(struct
> > v4l2_async_notifier *notifier, {
> > int ret;
> > 
> > -   if (notifier->ops->bound) {
> > -   ret = notifier->ops->bound(notifier, sd, asd);
> > -   if (ret < 0)
> > -   return ret;
> > -   }
> > +   ret = v4l2_async_notifier_call_bound(notifier, sd, asd);
> > +   if (ret < 0)
> > +   return ret;
> > 
> > ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> > if (ret < 0) {
> > -   if (notifier->ops->unbind)
> > -   notifier->ops->unbind(notifier, sd, asd);
> > +   v4l2_async_notifier_call_unbind(notifier, sd, asd);
> > return ret;
> > }
> > 
> > @@ -123,8 +148,8 @@ static int v4l2_async_match_notify(struct
> > v4l2_async_notifier *notifier, /* Move from the global subdevice list to
> > notifier's done */
> > list_move(>async_list, >done);
> > 
> > -   if (list_empty(>waiting) && notifier->ops->complete)
> > -   return notifier->ops->complete(notifier);
> > +   if (list_empty(>waiting))
> > +   return v4l2_async_notifier_call_complete(notifier);
> > 
> > return 0;
> >  }
> > @@ -210,8 +235,7 @@ void v4l2_async_notifier_unregister(struct
> > v4l2_async_notifier *notifier) list_for_each_entry_safe(sd, tmp,
> > >done, async_list) { v4l2_async_cleanup(sd);
> > 
> > -   if (notifier->ops->unbind)
> > -   notifier->ops->unbind(notifier, sd, sd->asd);
> > +   v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
> > }
> > 
> > mutex_unlock(_lock);
> > @@ -300,8 +324,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev
> > *sd)
> > 
> > v4l2_async_cleanup(sd);
> > 
> > -   if (notifier->ops->unbind)
> > -   notifier->ops->unbind(notifier, sd, sd->asd);
> > +   v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
> > 
> > mutex_unlock(_lock);
> >  }
> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > index 3c48f8b66d12..3bc8a7c0d83f 100644
> > --- a/include/media/v4l2-async.h
> > +++ b/include/media/v4l2-async.h
> > @@ -164,4 +164,5 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd);
> >   * @sd: pointer to  v4l2_subdev
> >   */
> >  void v4l2_async_unregister_subdev(struct v4l2_subdev *sd);
> > +
> >  #endif
> 
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH v13 11/25] v4l: async: Introduce helpers for calling async ops callbacks

2017-09-19 Thread Laurent Pinchart 
Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:10 EEST Sakari Ailus wrote:
> Add three helper functions to call async operations callbacks. Besides
> simplifying callbacks, this allows async notifiers to have no ops set,
> i.e. it can be left NULL.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Hans Verkuil 
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 49 
>  include/media/v4l2-async.h   |  1 +
>  2 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 7b2125b3d62f..c35d04b9122f
> 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -25,6 +25,34 @@
>  #include 
>  #include 
> 
> +static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier *n,
> +   struct v4l2_subdev *subdev,
> +   struct v4l2_async_subdev *asd)
> +{
> + if (!n->ops || !n->ops->bound)
> + return 0;
> +
> + return n->ops->bound(n, subdev, asd);
> +}
> +
> +static void v4l2_async_notifier_call_unbind(struct v4l2_async_notifier *n,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + if (!n->ops || !n->ops->unbind)
> + return;
> +
> + n->ops->unbind(n, subdev, asd);
> +}
> +
> +static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier *n)
> +{
> + if (!n->ops || !n->ops->complete)
> + return 0;
> +
> + return n->ops->complete(n);
> +}
> +

Wouldn't it be enough to add a single v4l2_async_notifier_call() macro ?

#define v4l2_async_notifier_call(n, op, args...) \
((n)->ops && (n)->ops->op ? (n)->ops->op(n, ##args) : 0)

Apart from that,

Reviewed-by: Laurent Pinchart 

>  static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd) {
>  #if IS_ENABLED(CONFIG_I2C)
> @@ -102,16 +130,13 @@ static int v4l2_async_match_notify(struct
> v4l2_async_notifier *notifier, {
>   int ret;
> 
> - if (notifier->ops->bound) {
> - ret = notifier->ops->bound(notifier, sd, asd);
> - if (ret < 0)
> - return ret;
> - }
> + ret = v4l2_async_notifier_call_bound(notifier, sd, asd);
> + if (ret < 0)
> + return ret;
> 
>   ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
>   if (ret < 0) {
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, asd);
> + v4l2_async_notifier_call_unbind(notifier, sd, asd);
>   return ret;
>   }
> 
> @@ -123,8 +148,8 @@ static int v4l2_async_match_notify(struct
> v4l2_async_notifier *notifier, /* Move from the global subdevice list to
> notifier's done */
>   list_move(>async_list, >done);
> 
> - if (list_empty(>waiting) && notifier->ops->complete)
> - return notifier->ops->complete(notifier);
> + if (list_empty(>waiting))
> + return v4l2_async_notifier_call_complete(notifier);
> 
>   return 0;
>  }
> @@ -210,8 +235,7 @@ void v4l2_async_notifier_unregister(struct
> v4l2_async_notifier *notifier) list_for_each_entry_safe(sd, tmp,
> >done, async_list) { v4l2_async_cleanup(sd);
> 
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, sd->asd);
> + v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
>   }
> 
>   mutex_unlock(_lock);
> @@ -300,8 +324,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev
> *sd)
> 
>   v4l2_async_cleanup(sd);
> 
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, sd->asd);
> + v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
> 
>   mutex_unlock(_lock);
>  }
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 3c48f8b66d12..3bc8a7c0d83f 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -164,4 +164,5 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd);
>   * @sd: pointer to  v4l2_subdev
>   */
>  void v4l2_async_unregister_subdev(struct v4l2_subdev *sd);
> +
>  #endif


-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 11/25] v4l: async: Introduce helpers for calling async ops callbacks

2017-09-16 Thread Pavel Machek 
On Fri 2017-09-15 17:17:10, Sakari Ailus wrote:
> Add three helper functions to call async operations callbacks. Besides
> simplifying callbacks, this allows async notifiers to have no ops set,
> i.e. it can be left NULL.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Hans Verkuil 

I'd remove "_call" from these names; they are long enough already and
do not add much. But either way is okay.

Acked-by: Pavel Machek 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

[PATCH v13 11/25] v4l: async: Introduce helpers for calling async ops callbacks

2017-09-15 Thread Sakari Ailus 
Add three helper functions to call async operations callbacks. Besides
simplifying callbacks, this allows async notifiers to have no ops set,
i.e. it can be left NULL.

Signed-off-by: Sakari Ailus 
Acked-by: Hans Verkuil 
---
 drivers/media/v4l2-core/v4l2-async.c | 49 ++--
 include/media/v4l2-async.h   |  1 +
 2 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c 
b/drivers/media/v4l2-core/v4l2-async.c
index 7b2125b3d62f..c35d04b9122f 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -25,6 +25,34 @@
 #include 
 #include 
 
+static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier *n,
+ struct v4l2_subdev *subdev,
+ struct v4l2_async_subdev *asd)
+{
+   if (!n->ops || !n->ops->bound)
+   return 0;
+
+   return n->ops->bound(n, subdev, asd);
+}
+
+static void v4l2_async_notifier_call_unbind(struct v4l2_async_notifier *n,
+   struct v4l2_subdev *subdev,
+   struct v4l2_async_subdev *asd)
+{
+   if (!n->ops || !n->ops->unbind)
+   return;
+
+   n->ops->unbind(n, subdev, asd);
+}
+
+static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier *n)
+{
+   if (!n->ops || !n->ops->complete)
+   return 0;
+
+   return n->ops->complete(n);
+}
+
 static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
 #if IS_ENABLED(CONFIG_I2C)
@@ -102,16 +130,13 @@ static int v4l2_async_match_notify(struct 
v4l2_async_notifier *notifier,
 {
int ret;
 
-   if (notifier->ops->bound) {
-   ret = notifier->ops->bound(notifier, sd, asd);
-   if (ret < 0)
-   return ret;
-   }
+   ret = v4l2_async_notifier_call_bound(notifier, sd, asd);
+   if (ret < 0)
+   return ret;
 
ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
if (ret < 0) {
-   if (notifier->ops->unbind)
-   notifier->ops->unbind(notifier, sd, asd);
+   v4l2_async_notifier_call_unbind(notifier, sd, asd);
return ret;
}
 
@@ -123,8 +148,8 @@ static int v4l2_async_match_notify(struct 
v4l2_async_notifier *notifier,
/* Move from the global subdevice list to notifier's done */
list_move(>async_list, >done);
 
-   if (list_empty(>waiting) && notifier->ops->complete)
-   return notifier->ops->complete(notifier);
+   if (list_empty(>waiting))
+   return v4l2_async_notifier_call_complete(notifier);
 
return 0;
 }
@@ -210,8 +235,7 @@ void v4l2_async_notifier_unregister(struct 
v4l2_async_notifier *notifier)
list_for_each_entry_safe(sd, tmp, >done, async_list) {
v4l2_async_cleanup(sd);
 
-   if (notifier->ops->unbind)
-   notifier->ops->unbind(notifier, sd, sd->asd);
+   v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
}
 
mutex_unlock(_lock);
@@ -300,8 +324,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
 
v4l2_async_cleanup(sd);
 
-   if (notifier->ops->unbind)
-   notifier->ops->unbind(notifier, sd, sd->asd);
+   v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
 
mutex_unlock(_lock);
 }
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 3c48f8b66d12..3bc8a7c0d83f 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -164,4 +164,5 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd);
  * @sd: pointer to  v4l2_subdev
  */
 void v4l2_async_unregister_subdev(struct v4l2_subdev *sd);
+
 #endif
-- 
2.11.0