Hi,
On 06/06/2013 02:11 AM, Sakari Ailus wrote:
Hi Sylwester,
Thanks for the patch!
And thanks for taking time to review!
On Fri, May 31, 2013 at 04:37:26PM +0200, Sylwester Nawrocki wrote:
...
@@ -547,25 +547,17 @@ int __media_entity_setup_link(struct media_link *link, u32 flags)
mdev = source->parent;
- if ((flags& MEDIA_LNK_FL_ENABLED)&& mdev->link_notify) {
- ret = mdev->link_notify(link->source, link->sink,
- MEDIA_LNK_FL_ENABLED);
+ if (mdev->link_notify) {
+ ret = mdev->link_notify(link, flags,
+ MEDIA_DEV_NOTIFY_PRE_LINK_CH);
if (ret< 0)
return ret;
}
ret = __media_entity_setup_link_notify(link, flags);
- if (ret< 0)
- goto err;
- if (!(flags& MEDIA_LNK_FL_ENABLED)&& mdev->link_notify)
- mdev->link_notify(link->source, link->sink, 0);
-
- return 0;
-
-err:
- if ((flags& MEDIA_LNK_FL_ENABLED)&& mdev->link_notify)
- mdev->link_notify(link->source, link->sink, 0);
+ if (mdev->link_notify)
+ mdev->link_notify(link, flags, MEDIA_DEV_NOTIFY_POST_LINK_CH);
This changes the behaviour of link_notify() so that the flags will be the
same independently of whether there was an error. I wonder if that's
intentional.
Yes, that's intentional. However I failed to update the omap3isp driver
link_notify handler properly to handle the link set up error case. I'll
correct that in next iteration.
I'd think that in the case of error the flags wouldn't change from what they
were, i.e. the flags argument would be "link->flags" instead of "flags".
The idea was to allow the link_notify handler to determine when link state
change succeeded, and when not. So the 'flags' link_notify() argument
indicates what was the intended state, while the actual state can be
determined by checking link->flags.
I considered not introducing the 'notification' argument and just comparing
'flags' and 'link->flags', but those look same before and after a call to
__media_entity_setup_link_notify() in error case.
return ret;
}
...
diff --git a/include/media/media-device.h b/include/media/media-device.h
index eaade98..353c4ee 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -45,6 +45,7 @@ struct device;
* @entities: List of registered entities
* @lock: Entities list lock
* @graph_mutex: Entities graph operation lock
+ * @link_notify: Link state change notification callback
*
* This structure represents an abstract high-level media device. It allows easy
* access to entities and provides basic media device-level support. The
@@ -75,10 +76,14 @@ struct media_device {
/* Serializes graph operations. */
struct mutex graph_mutex;
- int (*link_notify)(struct media_pad *source,
- struct media_pad *sink, u32 flags);
+ int (*link_notify)(struct media_link *link, unsigned int flags,
+ unsigned int notification);
media_link->flags is unsigned long. The patch doesn't break anything, but it
switches from u32/unsigned long to unsigned int/unsigned long for the field.
How about making media_link->flags unsigned int (or unsigned long) at the
same time, or not changing it? This could be fixed in a separate patch as
well (which I'm not necessarily expect from you now). There are probably a
number of places that would need to be changed.
Hmm, OK, I'll revert this 'flags' type change. I guess it's best to use
'unsigned int' all over, but I'll leave it out for a separate patch, for
someone to write. :)
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
