diff mbox

[1/4] media: Add pad flag MEDIA_PAD_FL_MUST_CONNECT

Message ID 1379541668-23085-2-git-send-email-sakari.ailus@iki.fi (mailing list archive)
State New, archived
Headers show

Commit Message

Sakari Ailus Sept. 18, 2013, 10:01 p.m. UTC 
Pads that set this flag must be connected by an active link for the entity
to stream.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Acked-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
---
 Documentation/DocBook/media/v4l/media-ioc-enum-links.xml |    8 ++++++++
 include/uapi/linux/media.h                               |    1 +
 2 files changed, 9 insertions(+)

Comments

Laurent Pinchart Sept. 20, 2013, 9:08 p.m. UTC | #1 
Hi Sakari,

Thank you for the patch.

On Thursday 19 September 2013 01:01:05 Sakari Ailus wrote:
> Pads that set this flag must be connected by an active link for the entity
> to stream.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> Acked-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> ---
>  Documentation/DocBook/media/v4l/media-ioc-enum-links.xml |    8 ++++++++
>  include/uapi/linux/media.h                               |    1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml index
> 355df43..59b212a 100644
> --- a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> +++ b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> @@ -134,6 +134,14 @@
>  	    <entry>Output pad, relative to the entity. Output pads source data
>  	    and are origins of links.</entry>
>  	  </row>
> +	  <row>
> +	    <entry><constant>MEDIA_PAD_FL_MUST_CONNECT</constant></entry>
> +	    <entry>A pad must be connected with an enabled link for the

s/A pad/The pad/ ?

> +	    entity to be able to stream. There could be temporary reasons
> +	    (e.g. device configuration dependent) for the pad to need
> +	    connecting; the absence of the flag won't say there
> +	    may not be any.</entry>

I believe the description doesn't make it very explicit that a MUST_CONNECT 
pad with no existing link is valid, as opposed to existing links with no 
enabled link, which would be invalid. Do you think we should fix that ?

> +	  </row>
>  	</tbody>
>        </tgroup>
>      </table>
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index ed49574..d847c76 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -98,6 +98,7 @@ struct media_entity_desc {
> 
>  #define MEDIA_PAD_FL_SINK		(1 << 0)
>  #define MEDIA_PAD_FL_SOURCE		(1 << 1)
> +#define MEDIA_PAD_FL_MUST_CONNECT	(1 << 2)
> 
>  struct media_pad_desc {
>  	__u32 entity;		/* entity ID */
Sakari Ailus Sept. 30, 2013, 11:08 p.m. UTC | #2 
Hi Laurent,

On Fri, Sep 20, 2013 at 11:08:47PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.

Thanks for the review! :)

> On Thursday 19 September 2013 01:01:05 Sakari Ailus wrote:
> > Pads that set this flag must be connected by an active link for the entity
> > to stream.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > Acked-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> > ---
> >  Documentation/DocBook/media/v4l/media-ioc-enum-links.xml |    8 ++++++++
> >  include/uapi/linux/media.h                               |    1 +
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml index
> > 355df43..59b212a 100644
> > --- a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > +++ b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > @@ -134,6 +134,14 @@
> >  	    <entry>Output pad, relative to the entity. Output pads source data
> >  	    and are origins of links.</entry>
> >  	  </row>
> > +	  <row>
> > +	    <entry><constant>MEDIA_PAD_FL_MUST_CONNECT</constant></entry>
> > +	    <entry>A pad must be connected with an enabled link for the
> 
> s/A pad/The pad/ ?

Fixed.

> > +	    entity to be able to stream. There could be temporary reasons
> > +	    (e.g. device configuration dependent) for the pad to need
> > +	    connecting; the absence of the flag won't say there
> > +	    may not be any.</entry>
> 
> I believe the description doesn't make it very explicit that a MUST_CONNECT 
> pad with no existing link is valid, as opposed to existing links with no 
> enabled link, which would be invalid. Do you think we should fix that ?

Yes. I propose to add this: "The flag has no effect on pads without
connected links."

> > +	  </row>
> >  	</tbody>
> >        </tgroup>
> >      </table>
> > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > index ed49574..d847c76 100644
> > --- a/include/uapi/linux/media.h
> > +++ b/include/uapi/linux/media.h
> > @@ -98,6 +98,7 @@ struct media_entity_desc {
> > 
> >  #define MEDIA_PAD_FL_SINK		(1 << 0)
> >  #define MEDIA_PAD_FL_SOURCE		(1 << 1)
> > +#define MEDIA_PAD_FL_MUST_CONNECT	(1 << 2)
> > 
> >  struct media_pad_desc {
> >  	__u32 entity;		/* entity ID */
Laurent Pinchart Sept. 30, 2013, 11:21 p.m. UTC | #3 
Hi Sakari,

On Tuesday 01 October 2013 02:08:47 Sakari Ailus wrote:
> On Fri, Sep 20, 2013 at 11:08:47PM +0200, Laurent Pinchart wrote:
> > On Thursday 19 September 2013 01:01:05 Sakari Ailus wrote:
> > > Pads that set this flag must be connected by an active link for the
> > > entity
> > > to stream.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > Acked-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> > > ---
> > > 
> > >  Documentation/DocBook/media/v4l/media-ioc-enum-links.xml |    8 +++++++
> > >  include/uapi/linux/media.h                               |    1 +
> > >  2 files changed, 9 insertions(+)
> > > 
> > > diff --git a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > > b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml index
> > > 355df43..59b212a 100644
> > > --- a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > > +++ b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > > @@ -134,6 +134,14 @@
> > >  	    <entry>Output pad, relative to the entity. Output pads source
> > >  	    data and are origins of links.</entry>
> > >  	  </row>
> > > +	  <row>
> > > +	    <entry><constant>MEDIA_PAD_FL_MUST_CONNECT</constant></entry>
> > > +	    <entry>A pad must be connected with an enabled link for the
> > 
> > s/A pad/The pad/ ?
> 
> Fixed.
> 
> > > +	    entity to be able to stream. There could be temporary reasons
> > > +	    (e.g. device configuration dependent) for the pad to need
> > > +	    connecting; the absence of the flag won't say there
> > > +	    may not be any.</entry>
> > 
> > I believe the description doesn't make it very explicit that a
> > MUST_CONNECT pad with no existing link is valid, as opposed to existing
> > links with no enabled link, which would be invalid. Do you think we should
> > fix that ?
> 
> Yes. I propose to add this: "The flag has no effect on pads without
> connected links."

What about

If the pad is linked to any other pad, at least one of the links must be
enabled for the entity to be able to stream. There could be temporary reasons
(e.g. device configuration dependent) for the pad to need enabled links; the 
absence of the flag doesn't imply there is none. The flag has no effect on
pads without connected links.

> > > +	  </row>
> > >  	</tbody>
> > >        </tgroup>
> > >      </table>
Sakari Ailus Sept. 30, 2013, 11:28 p.m. UTC | #4 
Hi Laurent,

Thnanks for the comments. A few more below.

On Tue, Oct 01, 2013 at 01:21:58AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday 01 October 2013 02:08:47 Sakari Ailus wrote:
> > On Fri, Sep 20, 2013 at 11:08:47PM +0200, Laurent Pinchart wrote:
> > > On Thursday 19 September 2013 01:01:05 Sakari Ailus wrote:
> > > > Pads that set this flag must be connected by an active link for the
> > > > entity
> > > > to stream.
> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > Acked-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> > > > ---
> > > > 
> > > >  Documentation/DocBook/media/v4l/media-ioc-enum-links.xml |    8 +++++++
> > > >  include/uapi/linux/media.h                               |    1 +
> > > >  2 files changed, 9 insertions(+)
> > > > 
> > > > diff --git a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > > > b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml index
> > > > 355df43..59b212a 100644
> > > > --- a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > > > +++ b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > > > @@ -134,6 +134,14 @@
> > > >  	    <entry>Output pad, relative to the entity. Output pads source
> > > >  	    data and are origins of links.</entry>
> > > >  	  </row>
> > > > +	  <row>
> > > > +	    <entry><constant>MEDIA_PAD_FL_MUST_CONNECT</constant></entry>
> > > > +	    <entry>A pad must be connected with an enabled link for the
> > > 
> > > s/A pad/The pad/ ?
> > 
> > Fixed.
> > 
> > > > +	    entity to be able to stream. There could be temporary reasons
> > > > +	    (e.g. device configuration dependent) for the pad to need
> > > > +	    connecting; the absence of the flag won't say there
> > > > +	    may not be any.</entry>
> > > 
> > > I believe the description doesn't make it very explicit that a
> > > MUST_CONNECT pad with no existing link is valid, as opposed to existing
> > > links with no enabled link, which would be invalid. Do you think we should
> > > fix that ?
> > 
> > Yes. I propose to add this: "The flag has no effect on pads without
> > connected links."
> 
> What about
> 
> If the pad is linked to any other pad, at least one of the links must be
> enabled for the entity to be able to stream. There could be temporary reasons
> (e.g. device configuration dependent) for the pad to need enabled links; the 
> absence of the flag doesn't imply there is none. The flag has no effect on
> pads without connected links.

Thinking about this again, I'd add before the comma: "and this flag is set".

And if you put it like that then the last sentence is redundat --- I'd drop
it.

What do you think?
Laurent Pinchart Oct. 1, 2013, 8:55 a.m. UTC | #5 
Hi Sakari,

On Tuesday 01 October 2013 02:28:23 Sakari Ailus wrote:
> On Tue, Oct 01, 2013 at 01:21:58AM +0200, Laurent Pinchart wrote:
> > On Tuesday 01 October 2013 02:08:47 Sakari Ailus wrote:
> > > On Fri, Sep 20, 2013 at 11:08:47PM +0200, Laurent Pinchart wrote:
> > > > On Thursday 19 September 2013 01:01:05 Sakari Ailus wrote:
> > > > > Pads that set this flag must be connected by an active link for the
> > > > > entity to stream.
> > > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > Acked-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>

[snip]

> > What about
> > 
> > If the pad is linked to any other pad, at least one of the links must be
> > enabled for the entity to be able to stream. There could be temporary
> > reasons (e.g. device configuration dependent) for the pad to need enabled
> > links; the absence of the flag doesn't imply there is none. The flag has
> > no effect on pads without connected links.
> 
> Thinking about this again, I'd add before the comma: "and this flag is set".
> 
> And if you put it like that then the last sentence is redundat --- I'd drop
> it.
> 
> What do you think?

What about

"When this flag is set, if the pad is linked to any other pad then at least 
one of those links must be enabled for the entity to be able to stream. There 
could be temporary reasons (e.g. device configuration dependent) for the pad 
to need enabled links even when this flag isn't set; the absence of the flag 
doesn't imply there is none. The flag has no effect on pads without connected 
links."

Feel free to drop the last sentence.
Sakari Ailus Oct. 1, 2013, 9:17 a.m. UTC | #6 
Hi Laurent,

On Tue, Oct 01, 2013 at 10:55:04AM +0200, Laurent Pinchart wrote:
> On Tuesday 01 October 2013 02:28:23 Sakari Ailus wrote:
> > On Tue, Oct 01, 2013 at 01:21:58AM +0200, Laurent Pinchart wrote:
> > > On Tuesday 01 October 2013 02:08:47 Sakari Ailus wrote:
> > > > On Fri, Sep 20, 2013 at 11:08:47PM +0200, Laurent Pinchart wrote:
> > > > > On Thursday 19 September 2013 01:01:05 Sakari Ailus wrote:
> > > > > > Pads that set this flag must be connected by an active link for the
> > > > > > entity to stream.
> > > > > > 
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > > Acked-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> 
> [snip]
> 
> > > What about
> > > 
> > > If the pad is linked to any other pad, at least one of the links must be
> > > enabled for the entity to be able to stream. There could be temporary
> > > reasons (e.g. device configuration dependent) for the pad to need enabled
> > > links; the absence of the flag doesn't imply there is none. The flag has
> > > no effect on pads without connected links.
> > 
> > Thinking about this again, I'd add before the comma: "and this flag is set".
> > 
> > And if you put it like that then the last sentence is redundat --- I'd drop
> > it.
> > 
> > What do you think?
> 
> What about
> 
> "When this flag is set, if the pad is linked to any other pad then at least 

How about:

"If this flag is set and the pad is linked to any other pad, then"...

I think it's cleaner like that.

> one of those links must be enabled for the entity to be able to stream. There 
> could be temporary reasons (e.g. device configuration dependent) for the pad 
> to need enabled links even when this flag isn't set; the absence of the flag 
> doesn't imply there is none. The flag has no effect on pads without connected 
> links."
> 
> Feel free to drop the last sentence.

Thinking about it again, I'm fine keeping it. :-)
Laurent Pinchart Oct. 1, 2013, 9:22 a.m. UTC | #7 
Hi Sakari,

On Tuesday 01 October 2013 12:17:21 Sakari Ailus wrote:
> On Tue, Oct 01, 2013 at 10:55:04AM +0200, Laurent Pinchart wrote:
> > On Tuesday 01 October 2013 02:28:23 Sakari Ailus wrote:
> > > On Tue, Oct 01, 2013 at 01:21:58AM +0200, Laurent Pinchart wrote:
> > > > On Tuesday 01 October 2013 02:08:47 Sakari Ailus wrote:
> > > > > On Fri, Sep 20, 2013 at 11:08:47PM +0200, Laurent Pinchart wrote:
> > > > > > On Thursday 19 September 2013 01:01:05 Sakari Ailus wrote:
> > > > > > > Pads that set this flag must be connected by an active link for
> > > > > > > the  entity to stream.
> > > > > > > 
> > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > > > Acked-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> > 
> > [snip]
> > 
> > > > What about
> > > > 
> > > > If the pad is linked to any other pad, at least one of the links must
> > > > be enabled for the entity to be able to stream. There could be
> > > > temporary reasons (e.g. device configuration dependent) for the pad to
> > > > need enabled links; the absence of the flag doesn't imply there is
> > > > none. The flag has no effect on pads without connected links.
> > > 
> > > Thinking about this again, I'd add before the comma: "and this flag is
> > > set".
> > > 
> > > And if you put it like that then the last sentence is redundat --- I'd
> > > drop it.
> > > 
> > > What do you think?
> > 
> > What about
> > 
> > "When this flag is set, if the pad is linked to any other pad then at
> > least
> 
> How about:
> 
> "If this flag is set and the pad is linked to any other pad, then"...
> 
> I think it's cleaner like that.

Fine with me.

> > one of those links must be enabled for the entity to be able to stream.
> > There could be temporary reasons (e.g. device configuration dependent)
> > for the pad to need enabled links even when this flag isn't set; the
> > absence of the flag doesn't imply there is none. The flag has no effect
> > on pads without connected links."
> > 
> > Feel free to drop the last sentence.
> 
> Thinking about it again, I'm fine keeping it. :-)
diff mbox

Patch

diff --git a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
index 355df43..59b212a 100644
--- a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
+++ b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
@@ -134,6 +134,14 @@ 
 	    <entry>Output pad, relative to the entity. Output pads source data
 	    and are origins of links.</entry>
 	  </row>
+	  <row>
+	    <entry><constant>MEDIA_PAD_FL_MUST_CONNECT</constant></entry>
+	    <entry>A pad must be connected with an enabled link for the
+	    entity to be able to stream. There could be temporary reasons
+	    (e.g. device configuration dependent) for the pad to need
+	    connecting; the absence of the flag won't say there
+	    may not be any.</entry>
+	  </row>
 	</tbody>
       </tgroup>
     </table>
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index ed49574..d847c76 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -98,6 +98,7 @@  struct media_entity_desc {
 
 #define MEDIA_PAD_FL_SINK		(1 << 0)
 #define MEDIA_PAD_FL_SOURCE		(1 << 1)
+#define MEDIA_PAD_FL_MUST_CONNECT	(1 << 2)
 
 struct media_pad_desc {
 	__u32 entity;		/* entity ID */