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

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:

quoted

On Tuesday, 19 September 2017 15:13:11 EEST Sakari Ailus wrote:

quoted

On Tue, Sep 19, 2017 at 03:01:14PM +0300, Laurent Pinchart wrote:

quoted

On Friday, 15 September 2017 17:17:10 EEST Sakari Ailus wrote:

quoted

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 <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <redacted>
---

 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 <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>

+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).

quoted

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