[PATCHv5 00/12] media/mc: fix inconsistencies

[PATCHv5 00/12] media/mc: fix inconsistencies

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This series is v5 of my previous attempt:

https://www.mail-archive.com/linux-media@vger.kernel.org/msg133111.html

The goal is to fix the inconsistencies between the 'old' and 'new' 
MC API. I hate the terms 'old' and 'new', there is nothing wrong IMHO with 
using an 'old' API if it meets the needs of the application.

Making G_TOPOLOGY useful is urgently needed since I think that will be
very helpful for the work we want to do on library support for complex
cameras.

Changes since v4:

- Improve documentation of the new index field in patch 2.
- Added patch 11 to sync the index field documentation in media-ioc-enum-links.rst
  with the index field documentation from media-ioc-g-topology.rst.
- Added patch 12 that clarifies that you should not hardcode ID values
  in applications since they can change between instances of the device.
  Also document that the entity name is unique within the media topology.

Changes since v3:

- Renamed MEDIA_ENT_F_DTV_ENCODER to MEDIA_ENT_F_DV_ENCODER
- Added a new patch renaming MEDIA_ENT_F_DTV_DECODER to MEDIA_ENT_F_DV_DECODER.
- Added a new patch that reorders the function defines in media.h so that they
  are in increasing numerical order (the en/decoder functions where not in
  order).
- Added Sakari's Acks (except for the two new patches).

Regards,

        Hans

Hans Verkuil (12):
  media: add 'index' to struct media_v2_pad
  media-ioc-g-topology.rst: document new 'index' field
  media: add flags field to struct media_v2_entity
  media-ioc-g-topology.rst: document new 'flags' field
  media: rename MEDIA_ENT_F_DTV_DECODER to MEDIA_ENT_F_DV_DECODER
  media.h: add MEDIA_ENT_F_DV_ENCODER
  media.h: reorder video en/decoder functions
  ad9389b/adv7511: set proper media entity function
  adv7180/tvp514x/tvp7002: fix entity function
  media/i2c: add missing entity functions
  media-ioc-enum-links.rst: improve pad index description
  media-ioc-enum-entities.rst/-g-topology.rst: clarify ID/name usage

 .../uapi/mediactl/media-ioc-enum-entities.rst |  9 ++--
 .../uapi/mediactl/media-ioc-enum-links.rst    |  4 +-
 .../uapi/mediactl/media-ioc-g-topology.rst    | 42 +++++++++++++++----
 .../media/uapi/mediactl/media-types.rst       |  9 +++-
 drivers/media/i2c/ad9389b.c                   |  1 +
 drivers/media/i2c/adv7180.c                   |  2 +-
 drivers/media/i2c/adv7511.c                   |  1 +
 drivers/media/i2c/adv7604.c                   |  1 +
 drivers/media/i2c/adv7842.c                   |  1 +
 drivers/media/i2c/et8ek8/et8ek8_driver.c      |  1 +
 drivers/media/i2c/mt9m032.c                   |  1 +
 drivers/media/i2c/mt9p031.c                   |  1 +
 drivers/media/i2c/mt9t001.c                   |  1 +
 drivers/media/i2c/mt9v032.c                   |  1 +
 drivers/media/i2c/tda1997x.c                  |  2 +-
 drivers/media/i2c/tvp514x.c                   |  2 +-
 drivers/media/i2c/tvp7002.c                   |  2 +-
 drivers/media/media-device.c                  |  2 +
 include/uapi/linux/media.h                    | 39 +++++++++++++----
 19 files changed, 97 insertions(+), 25 deletions(-)

-- 
2.17.0

[PATCHv5 01/12] media: add 'index' to struct media_v2_pad

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

The v2 pad structure never exposed the pad index, which made it impossible
to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.

It is really trivial to just expose this information, so implement this.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c |  1 +
 include/uapi/linux/media.h   | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 47bb2254fbfd..047d38372a27 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -331,6 +331,7 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
 		kpad.id = pad->graph_obj.id;
 		kpad.entity_id = pad->entity->graph_obj.id;
 		kpad.flags = pad->flags;
+		kpad.index = pad->index;
 
 		if (copy_to_user(upad, &kpad, sizeof(kpad)))
 			ret = -EFAULT;
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 86c7dcc9cba3..f6338bd57929 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -305,11 +305,21 @@ struct media_v2_interface {
 	};
 } __attribute__ ((packed));
 
+/*
+ * Appeared in 4.19.0.
+ *
+ * The media_version argument comes from the media_version field in
+ * struct media_device_info.
+ */
+#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
+	((media_version) >= ((4 << 16) | (19 << 8) | 0))
+
 struct media_v2_pad {
 	__u32 id;
 	__u32 entity_id;
 	__u32 flags;
-	__u32 reserved[5];
+	__u32 index;
+	__u32 reserved[4];
 } __attribute__ ((packed));
 
 struct media_v2_link {
-- 
2.17.0

Re: [PATCHv5 01/12] media: add 'index' to struct media_v2_pad

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday, 29 June 2018 14:43:20 EEST Hans Verkuil wrote:
> From: Hans Verkuil <hansverk@cisco.com>
> 
> The v2 pad structure never exposed the pad index, which made it impossible
> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
> 
> It is really trivial to just expose this information, so implement this.
> 
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-device.c |  1 +
>  include/uapi/linux/media.h   | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 47bb2254fbfd..047d38372a27 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct
> media_device *mdev, void *arg) kpad.id = pad->graph_obj.id;
>  		kpad.entity_id = pad->entity->graph_obj.id;
>  		kpad.flags = pad->flags;
> +		kpad.index = pad->index;
> 
>  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
>  			ret = -EFAULT;
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 86c7dcc9cba3..f6338bd57929 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -305,11 +305,21 @@ struct media_v2_interface {
>  	};
>  } __attribute__ ((packed));
> 
> +/*
> + * Appeared in 4.19.0.
> + *
> + * The media_version argument comes from the media_version field in
> + * struct media_device_info.
> + */
> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
> +	((media_version) >= ((4 << 16) | (19 << 8) | 0))

I agree that we need tn index field, but I don't think we need to care about 
backward compatibility. The lack of an index field makes it clear that the API 
has never been properly used, as it was impossible to do so.

>  struct media_v2_pad {
>  	__u32 id;
>  	__u32 entity_id;
>  	__u32 flags;
> -	__u32 reserved[5];
> +	__u32 index;
> +	__u32 reserved[4];
>  } __attribute__ ((packed));
> 
>  struct media_v2_link {

-- 
Regards,

Laurent Pinchart

Re: [PATCHv5 01/12] media: add 'index' to struct media_v2_pad

[Hans Verkuil]

On 09/07/18 14:55, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Friday, 29 June 2018 14:43:20 EEST Hans Verkuil wrote:
>> From: Hans Verkuil <hansverk@cisco.com>
>>
>> The v2 pad structure never exposed the pad index, which made it impossible
>> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
>>
>> It is really trivial to just expose this information, so implement this.
>>
>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>  drivers/media/media-device.c |  1 +
>>  include/uapi/linux/media.h   | 12 +++++++++++-
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 47bb2254fbfd..047d38372a27 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct
>> media_device *mdev, void *arg) kpad.id = pad->graph_obj.id;
>>  		kpad.entity_id = pad->entity->graph_obj.id;
>>  		kpad.flags = pad->flags;
>> +		kpad.index = pad->index;
>>
>>  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
>>  			ret = -EFAULT;
>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>> index 86c7dcc9cba3..f6338bd57929 100644
>> --- a/include/uapi/linux/media.h
>> +++ b/include/uapi/linux/media.h
>> @@ -305,11 +305,21 @@ struct media_v2_interface {
>>  	};
>>  } __attribute__ ((packed));
>>
>> +/*
>> + * Appeared in 4.19.0.
>> + *
>> + * The media_version argument comes from the media_version field in
>> + * struct media_device_info.
>> + */
>> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
>> +	((media_version) >= ((4 << 16) | (19 << 8) | 0))
> 
> I agree that we need tn index field, but I don't think we need to care about 
> backward compatibility. The lack of an index field makes it clear that the API 
> has never been properly used, as it was impossible to do so.

We do need to care: there is no reason why a v4l2 application can't be used on
an older kernel. Most v4l2 applications copy the V4L2 headers to the application
(in fact, that's what v4l-utils does) and so they need to know if a field is
actually filled in by whatever kernel is used. In most cases they can just check
against 0, but that happens to be a valid index :-(

So this is really needed. Same for the flags field.

Regards,

	Hans

> 
>>  struct media_v2_pad {
>>  	__u32 id;
>>  	__u32 entity_id;
>>  	__u32 flags;
>> -	__u32 reserved[5];
>> +	__u32 index;
>> +	__u32 reserved[4];
>>  } __attribute__ ((packed));
>>
>>  struct media_v2_link {
> 

Re: [PATCHv5 01/12] media: add 'index' to struct media_v2_pad

[Laurent Pinchart]

Hi Hans,

On Monday, 9 July 2018 16:40:51 EEST Hans Verkuil wrote:
> On 09/07/18 14:55, Laurent Pinchart wrote:
> > On Friday, 29 June 2018 14:43:20 EEST Hans Verkuil wrote:
> >> From: Hans Verkuil <hansverk@cisco.com>
> >> 
> >> The v2 pad structure never exposed the pad index, which made it
> >> impossible
> >> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
> >> 
> >> It is really trivial to just expose this information, so implement this.
> >> 
> >> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> >> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> ---
> >> 
> >>  drivers/media/media-device.c |  1 +
> >>  include/uapi/linux/media.h   | 12 +++++++++++-
> >>  2 files changed, 12 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index 47bb2254fbfd..047d38372a27 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c
> >> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct
> >> media_device *mdev, void *arg)
> >>  		kpad.id = pad->graph_obj.id;
> >>  		kpad.entity_id = pad->entity->graph_obj.id;
> >>  		kpad.flags = pad->flags;
> >> +		kpad.index = pad->index;
> >> 
> >>  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
> >>  			ret = -EFAULT;
> >> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> >> index 86c7dcc9cba3..f6338bd57929 100644
> >> --- a/include/uapi/linux/media.h
> >> +++ b/include/uapi/linux/media.h
> >> @@ -305,11 +305,21 @@ struct media_v2_interface {
> >>  	};
> >>  } __attribute__ ((packed));
> >> 
> >> +/*
> >> + * Appeared in 4.19.0.
> >> + *
> >> + * The media_version argument comes from the media_version field in
> >> + * struct media_device_info.
> >> + */
> >> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
> >> +	((media_version) >= ((4 << 16) | (19 << 8) | 0))
> > 
> > I agree that we need tn index field, but I don't think we need to care
> > about backward compatibility. The lack of an index field makes it clear
> > that the API has never been properly used, as it was impossible to do so.
> 
> We do need to care: there is no reason why a v4l2 application can't be used
> on an older kernel. Most v4l2 applications copy the V4L2 headers to the
> application (in fact, that's what v4l-utils does) and so they need to know
> if a field is actually filled in by whatever kernel is used. In most cases
> they can just check against 0, but that happens to be a valid index :-(
> 
> So this is really needed. Same for the flags field.

You're right. I was thinking we could detect this on the kernel side by 
checking the ioctl argument size if we added the index field to the 
media_v2_pad structure instead of replacing one of the reserved fields, but 
media_v2_pad is not passed directly to the G_TOPOLOGY ioctl, so that won't 
help.

I wonder whether we shouldn't just define

#define MEDIA_V2_IS_BROKEN(media_version) \
	((media_version) < ((4 << 16) | (19 << 8) | 0))

as in practice applications should really avoid the G_TOPOLOGY ioctl without 
this patch series. Having multiple version-based macros to check for features 
won't be very helpful, and could be counter-productive as applications might 
incorrectly decide to still use the API to retrieve some information when they 
should really avoid it.

And, while at it, should we use KERNEL_VERSION() instead of hardcoding it ?

#define MEDIA_V2_IS_BROKEN(media_version) \
	((media_version) < KERNEL_VERSION(4, 19, 0))

Still thinking out loud, the fact that we can't change the size of the 
structures pointed to by media_v2_topology bothers me. We could add a version 
field to media_v2_topology that would be set by applications to tell the 
kernel which version of the API they expect. On the other hand, maybe we'll 
just do a media_v3_topology when the need arises...

(And I still really don't like the use of media_v2_link to describe the 
association between an entity and an interface, I think a media_v2_association 
structure would have been cleaner :-().

> >>  struct media_v2_pad {
> >>  	__u32 id;
> >>  	__u32 entity_id;
> >>  	__u32 flags;
> >> -	__u32 reserved[5];
> >> +	__u32 index;
> >> +	__u32 reserved[4];
> >>  } __attribute__ ((packed));
> >>  
> >>  struct media_v2_link {

-- 
Regards,

Laurent Pinchart

Re: [PATCHv5 01/12] media: add 'index' to struct media_v2_pad

[Hans Verkuil]

On 11/07/18 13:33, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday, 9 July 2018 16:40:51 EEST Hans Verkuil wrote:
>> On 09/07/18 14:55, Laurent Pinchart wrote:
>>> On Friday, 29 June 2018 14:43:20 EEST Hans Verkuil wrote:
>>>> From: Hans Verkuil <hansverk@cisco.com>
>>>>
>>>> The v2 pad structure never exposed the pad index, which made it
>>>> impossible
>>>> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
>>>>
>>>> It is really trivial to just expose this information, so implement this.
>>>>
>>>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>>>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> ---
>>>>
>>>>  drivers/media/media-device.c |  1 +
>>>>  include/uapi/linux/media.h   | 12 +++++++++++-
>>>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>>> index 47bb2254fbfd..047d38372a27 100644
>>>> --- a/drivers/media/media-device.c
>>>> +++ b/drivers/media/media-device.c
>>>> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct
>>>> media_device *mdev, void *arg)
>>>>  		kpad.id = pad->graph_obj.id;
>>>>  		kpad.entity_id = pad->entity->graph_obj.id;
>>>>  		kpad.flags = pad->flags;
>>>> +		kpad.index = pad->index;
>>>>
>>>>  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
>>>>  			ret = -EFAULT;
>>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>>>> index 86c7dcc9cba3..f6338bd57929 100644
>>>> --- a/include/uapi/linux/media.h
>>>> +++ b/include/uapi/linux/media.h
>>>> @@ -305,11 +305,21 @@ struct media_v2_interface {
>>>>  	};
>>>>  } __attribute__ ((packed));
>>>>
>>>> +/*
>>>> + * Appeared in 4.19.0.
>>>> + *
>>>> + * The media_version argument comes from the media_version field in
>>>> + * struct media_device_info.
>>>> + */
>>>> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
>>>> +	((media_version) >= ((4 << 16) | (19 << 8) | 0))
>>>
>>> I agree that we need tn index field, but I don't think we need to care
>>> about backward compatibility. The lack of an index field makes it clear
>>> that the API has never been properly used, as it was impossible to do so.
>>
>> We do need to care: there is no reason why a v4l2 application can't be used
>> on an older kernel. Most v4l2 applications copy the V4L2 headers to the
>> application (in fact, that's what v4l-utils does) and so they need to know
>> if a field is actually filled in by whatever kernel is used. In most cases
>> they can just check against 0, but that happens to be a valid index :-(
>>
>> So this is really needed. Same for the flags field.
> 
> You're right. I was thinking we could detect this on the kernel side by 
> checking the ioctl argument size if we added the index field to the 
> media_v2_pad structure instead of replacing one of the reserved fields, but 
> media_v2_pad is not passed directly to the G_TOPOLOGY ioctl, so that won't 
> help.
> 
> I wonder whether we shouldn't just define
> 
> #define MEDIA_V2_IS_BROKEN(media_version) \
> 	((media_version) < ((4 << 16) | (19 << 8) | 0))
> 
> as in practice applications should really avoid the G_TOPOLOGY ioctl without 
> this patch series. Having multiple version-based macros to check for features 
> won't be very helpful, and could be counter-productive as applications might 
> incorrectly decide to still use the API to retrieve some information when they 
> should really avoid it.

G_TOPOLOGY is still useful if all you want is to retrieve the topology.
Only if you need to modify routing is it indeed useless.

I prefer to keep the current defines: it is clear what they do whereas
'IS_BROKEN' just makes people wonder why it is broken.

> And, while at it, should we use KERNEL_VERSION() instead of hardcoding it ?
> 
> #define MEDIA_V2_IS_BROKEN(media_version) \
> 	((media_version) < KERNEL_VERSION(4, 19, 0))

It's not clear if you are actually allowed to use KERNEL_VERSION in userspace
headers. The only header in the kernel where this is used is actually media.h:

#define MEDIA_API_VERSION                    KERNEL_VERSION(0, 1, 0)

And that define is a legacy define that probably nobody uses.

There is no other header that does this. So I prefer not to depend on this,
and in fact I think the MEDIA_API_VERSION should also be rewritten so it
doesn't depend on KERNEL_VERSION anymore.

Regards,

	Hans

> 
> Still thinking out loud, the fact that we can't change the size of the 
> structures pointed to by media_v2_topology bothers me. We could add a version 
> field to media_v2_topology that would be set by applications to tell the 
> kernel which version of the API they expect. On the other hand, maybe we'll 
> just do a media_v3_topology when the need arises...
> 
> (And I still really don't like the use of media_v2_link to describe the 
> association between an entity and an interface, I think a media_v2_association 
> structure would have been cleaner :-().
> 
>>>>  struct media_v2_pad {
>>>>  	__u32 id;
>>>>  	__u32 entity_id;
>>>>  	__u32 flags;
>>>> -	__u32 reserved[5];
>>>> +	__u32 index;
>>>> +	__u32 reserved[4];
>>>>  } __attribute__ ((packed));
>>>>  
>>>>  struct media_v2_link {
> 

Re: [PATCHv5 01/12] media: add 'index' to struct media_v2_pad

[Sakari Ailus]

Hi Laurent,

On Wed, Jul 11, 2018 at 02:33:47PM +0300, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday, 9 July 2018 16:40:51 EEST Hans Verkuil wrote:
> > On 09/07/18 14:55, Laurent Pinchart wrote:
> > > On Friday, 29 June 2018 14:43:20 EEST Hans Verkuil wrote:
> > >> From: Hans Verkuil <hansverk@cisco.com>
> > >> 
> > >> The v2 pad structure never exposed the pad index, which made it
> > >> impossible
> > >> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
> > >> 
> > >> It is really trivial to just expose this information, so implement this.
> > >> 
> > >> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> > >> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >> ---
> > >> 
> > >>  drivers/media/media-device.c |  1 +
> > >>  include/uapi/linux/media.h   | 12 +++++++++++-
> > >>  2 files changed, 12 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > >> index 47bb2254fbfd..047d38372a27 100644
> > >> --- a/drivers/media/media-device.c
> > >> +++ b/drivers/media/media-device.c
> > >> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct
> > >> media_device *mdev, void *arg)
> > >>  		kpad.id = pad->graph_obj.id;
> > >>  		kpad.entity_id = pad->entity->graph_obj.id;
> > >>  		kpad.flags = pad->flags;
> > >> +		kpad.index = pad->index;
> > >> 
> > >>  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
> > >>  			ret = -EFAULT;
> > >> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > >> index 86c7dcc9cba3..f6338bd57929 100644
> > >> --- a/include/uapi/linux/media.h
> > >> +++ b/include/uapi/linux/media.h
> > >> @@ -305,11 +305,21 @@ struct media_v2_interface {
> > >>  	};
> > >>  } __attribute__ ((packed));
> > >> 
> > >> +/*
> > >> + * Appeared in 4.19.0.
> > >> + *
> > >> + * The media_version argument comes from the media_version field in
> > >> + * struct media_device_info.
> > >> + */
> > >> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
> > >> +	((media_version) >= ((4 << 16) | (19 << 8) | 0))
> > > 
> > > I agree that we need tn index field, but I don't think we need to care
> > > about backward compatibility. The lack of an index field makes it clear
> > > that the API has never been properly used, as it was impossible to do so.
> > 
> > We do need to care: there is no reason why a v4l2 application can't be used
> > on an older kernel. Most v4l2 applications copy the V4L2 headers to the
> > application (in fact, that's what v4l-utils does) and so they need to know
> > if a field is actually filled in by whatever kernel is used. In most cases
> > they can just check against 0, but that happens to be a valid index :-(
> > 
> > So this is really needed. Same for the flags field.
> 
> You're right. I was thinking we could detect this on the kernel side by 
> checking the ioctl argument size if we added the index field to the 
> media_v2_pad structure instead of replacing one of the reserved fields, but 
> media_v2_pad is not passed directly to the G_TOPOLOGY ioctl, so that won't 
> help.
> 
> I wonder whether we shouldn't just define
> 
> #define MEDIA_V2_IS_BROKEN(media_version) \
> 	((media_version) < ((4 << 16) | (19 << 8) | 0))
> 
> as in practice applications should really avoid the G_TOPOLOGY ioctl without 
> this patch series. Having multiple version-based macros to check for features 
> won't be very helpful, and could be counter-productive as applications might 
> incorrectly decide to still use the API to retrieve some information when they 
> should really avoid it.
> 
> And, while at it, should we use KERNEL_VERSION() instead of hardcoding it ?
> 
> #define MEDIA_V2_IS_BROKEN(media_version) \
> 	((media_version) < KERNEL_VERSION(4, 19, 0))
> 
> Still thinking out loud, the fact that we can't change the size of the 
> structures pointed to by media_v2_topology bothers me. We could add a version 
> field to media_v2_topology that would be set by applications to tell the 
> kernel which version of the API they expect. On the other hand, maybe we'll 
> just do a media_v3_topology when the need arises...

What you could to is to allocate another field for the new struct; we're
entirely free to put whatever we want there, and the kernel would simply
need to convert between the two. Not ideal though. Another option would be to
explicitly convey the struct size as for the IOCTL argument itself. Both
should probably be used sparingly.

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

Re: [PATCHv5 01/12] media: add 'index' to struct media_v2_pad

[Mauro Carvalho Chehab]

Em Fri, 3 Aug 2018 15:34:26 +0300
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> > Still thinking out loud, the fact that we can't change the size of the 
> > structures pointed to by media_v2_topology bothers me. We could add a version 
> > field to media_v2_topology that would be set by applications to tell the 
> > kernel which version of the API they expect. On the other hand, maybe we'll 
> > just do a media_v3_topology when the need arises...  
> 
> What you could to is to allocate another field for the new struct; we're
> entirely free to put whatever we want there, and the kernel would simply
> need to convert between the two. Not ideal though. Another option would be to
> explicitly convey the struct size as for the IOCTL argument itself. Both
> should probably be used sparingly.

The idea we had at the MC workshop is that anything else would be mapped
via the properties API.

So, the per-type structs will be kept on a minimal format.


Thanks,
Mauro

[PATCHv5 02/12] media-ioc-g-topology.rst: document new 'index' field

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Document the new struct media_v2_pad 'index' field.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 .../media/uapi/mediactl/media-ioc-g-topology.rst     | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
index a3f259f83b25..bae2b4db89cc 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
@@ -176,7 +176,7 @@ desired arrays with the media graph elements.
     *  -  struct media_v2_intf_devnode
        -  ``devnode``
        -  Used only for device node interfaces. See
-	  :c:type:`media_v2_intf_devnode` for details..
+	  :c:type:`media_v2_intf_devnode` for details.
 
 
 .. tabularcolumns:: |p{1.6cm}|p{3.2cm}|p{12.7cm}|
@@ -218,7 +218,15 @@ desired arrays with the media graph elements.
        -  Pad flags, see :ref:`media-pad-flag` for more details.
 
     *  -  __u32
-       -  ``reserved``\ [5]
+       -  ``index``
+       -  Pad index, starts at 0. Only valid if ``MEDIA_V2_PAD_HAS_INDEX(media_version)``
+	  returns true. The ``media_version`` is defined in struct
+	  :c:type:`media_device_info` and can be retrieved using
+	  :ref:`MEDIA_IOC_DEVICE_INFO`. Pad indices are stable. If new pads are added
+	  for an entity in the future, then those will be added at the end.
+
+    *  -  __u32
+       -  ``reserved``\ [4]
        -  Reserved for future extensions. Drivers and applications must set
 	  this array to zero.
 
-- 
2.17.0

Re: [PATCHv5 02/12] media-ioc-g-topology.rst: document new 'index' field

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday, 29 June 2018 14:43:21 EEST Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Document the new struct media_v2_pad 'index' field.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  .../media/uapi/mediactl/media-ioc-g-topology.rst     | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst index
> a3f259f83b25..bae2b4db89cc 100644
> --- a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> +++ b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> @@ -176,7 +176,7 @@ desired arrays with the media graph elements.
>      *  -  struct media_v2_intf_devnode
>         -  ``devnode``
>         -  Used only for device node interfaces. See
> -	  :c:type:`media_v2_intf_devnode` for details..
> +	  :c:type:`media_v2_intf_devnode` for details.
> 
> 
>  .. tabularcolumns:: |p{1.6cm}|p{3.2cm}|p{12.7cm}|
> @@ -218,7 +218,15 @@ desired arrays with the media graph elements.
>         -  Pad flags, see :ref:`media-pad-flag` for more details.
> 
>      *  -  __u32
> -       -  ``reserved``\ [5]
> +       -  ``index``
> +       -  Pad index, starts at 0. Only valid if
> ``MEDIA_V2_PAD_HAS_INDEX(media_version)`` +	  returns true. The
> ``media_version`` is defined in struct
> +	  :c:type:`media_device_info` and can be retrieved using
> +	  :ref:`MEDIA_IOC_DEVICE_INFO`. Pad indices are stable. If new pads are
> added
> +	  for an entity in the future, then those will be added at the end.

Same comment as for 01/12, I don't think we need to care about backward 
compatibility as this API clearly could not have been used by application. 
Apart from that the documentation update looks good to me.

> +    *  -  __u32
> +       -  ``reserved``\ [4]
>         -  Reserved for future extensions. Drivers and applications must set
> this array to zero.

-- 
Regards,

Laurent Pinchart

[PATCHv5 03/12] media: add flags field to struct media_v2_entity

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

The v2 entity structure never exposed the entity flags, which made it
impossible to detect connector or default entities.

It is really trivial to just expose this information, so implement this.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c |  1 +
 include/uapi/linux/media.h   | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 047d38372a27..14959b19a342 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -266,6 +266,7 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
 		memset(&kentity, 0, sizeof(kentity));
 		kentity.id = entity->graph_obj.id;
 		kentity.function = entity->function;
+		kentity.flags = entity->flags;
 		strlcpy(kentity.name, entity->name,
 			sizeof(kentity.name));
 
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index f6338bd57929..ebd2cda67833 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -280,11 +280,21 @@ struct media_links_enum {
  * MC next gen API definitions
  */
 
+/*
+ * Appeared in 4.19.0.
+ *
+ * The media_version argument comes from the media_version field in
+ * struct media_device_info.
+ */
+#define MEDIA_V2_ENTITY_HAS_FLAGS(media_version) \
+	((media_version) >= ((4 << 16) | (19 << 8) | 0))
+
 struct media_v2_entity {
 	__u32 id;
 	char name[64];
 	__u32 function;		/* Main function of the entity */
-	__u32 reserved[6];
+	__u32 flags;
+	__u32 reserved[5];
 } __attribute__ ((packed));
 
 /* Should match the specific fields at media_intf_devnode */
-- 
2.17.0

Re: [PATCHv5 03/12] media: add flags field to struct media_v2_entity

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday, 29 June 2018 14:43:22 EEST Hans Verkuil wrote:
> From: Hans Verkuil <hansverk@cisco.com>
> 
> The v2 entity structure never exposed the entity flags, which made it
> impossible to detect connector or default entities.
> 
> It is really trivial to just expose this information, so implement this.
> 
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-device.c |  1 +
>  include/uapi/linux/media.h   | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 047d38372a27..14959b19a342 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -266,6 +266,7 @@ static long media_device_get_topology(struct
> media_device *mdev, void *arg) memset(&kentity, 0, sizeof(kentity));
>  		kentity.id = entity->graph_obj.id;
>  		kentity.function = entity->function;
> +		kentity.flags = entity->flags;
>  		strlcpy(kentity.name, entity->name,
>  			sizeof(kentity.name));
> 
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index f6338bd57929..ebd2cda67833 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -280,11 +280,21 @@ struct media_links_enum {
>   * MC next gen API definitions
>   */
> 
> +/*
> + * Appeared in 4.19.0.
> + *
> + * The media_version argument comes from the media_version field in
> + * struct media_device_info.
> + */
> +#define MEDIA_V2_ENTITY_HAS_FLAGS(media_version) \
> +	((media_version) >= ((4 << 16) | (19 << 8) | 0))

Same comment here as for patch 01/12. It also applies to patch 04/12.

>  struct media_v2_entity {
>  	__u32 id;
>  	char name[64];
>  	__u32 function;		/* Main function of the entity */
> -	__u32 reserved[6];
> +	__u32 flags;
> +	__u32 reserved[5];
>  } __attribute__ ((packed));
> 
>  /* Should match the specific fields at media_intf_devnode */

-- 
Regards,

Laurent Pinchart

[PATCHv5 04/12] media-ioc-g-topology.rst: document new 'flags' field

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Document the new struct media_v2_entity 'flags' field.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 .../media/uapi/mediactl/media-ioc-g-topology.rst       | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
index bae2b4db89cc..e572dd0d806d 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
@@ -142,7 +142,15 @@ desired arrays with the media graph elements.
        -  Entity main function, see :ref:`media-entity-functions` for details.
 
     *  -  __u32
-       -  ``reserved``\ [6]
+       -  ``flags``
+       -  Entity flags, see :ref:`media-entity-flag` for details.
+	  Only valid if ``MEDIA_V2_ENTITY_HAS_FLAGS(media_version)``
+	  returns true. The ``media_version`` is defined in struct
+	  :c:type:`media_device_info` and can be retrieved using
+	  :ref:`MEDIA_IOC_DEVICE_INFO`.
+
+    *  -  __u32
+       -  ``reserved``\ [5]
        -  Reserved for future extensions. Drivers and applications must set
 	  this array to zero.
 
-- 
2.17.0

[PATCHv5 05/12] media: rename MEDIA_ENT_F_DTV_DECODER to MEDIA_ENT_F_DV_DECODER

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

The use of 'DTV' is very confusing since it normally refers to Digital
TV e.g. DVB etc.

Instead use 'DV' (Digital Video), which nicely corresponds to the
DV Timings API used to configure such receivers and transmitters.

We keep an alias to avoid breaking userspace applications.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 Documentation/media/uapi/mediactl/media-types.rst | 2 +-
 drivers/media/i2c/adv7604.c                       | 1 +
 drivers/media/i2c/adv7842.c                       | 1 +
 drivers/media/i2c/tda1997x.c                      | 2 +-
 include/uapi/linux/media.h                        | 4 +++-
 5 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/media/uapi/mediactl/media-types.rst b/Documentation/media/uapi/mediactl/media-types.rst
index 96910cf2eaaa..c11b0c7e890b 100644
--- a/Documentation/media/uapi/mediactl/media-types.rst
+++ b/Documentation/media/uapi/mediactl/media-types.rst
@@ -200,7 +200,7 @@ Types and flags used to represent the media graph elements
          MIPI CSI-2, etc.), and outputs them on its source pad to an output
          video bus of another type (eDP, MIPI CSI-2, parallel, etc.).
 
-    *  -  ``MEDIA_ENT_F_DTV_DECODER``
+    *  -  ``MEDIA_ENT_F_DV_DECODER``
        -  Digital video decoder. The basic function of the video decoder is
 	  to accept digital video from a wide variety of sources
 	  and output it in some digital video standard, with appropriate
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 1a3b2c04d9f9..668be2bca57a 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -3499,6 +3499,7 @@ static int adv76xx_probe(struct i2c_client *client,
 	for (i = 0; i < state->source_pad; ++i)
 		state->pads[i].flags = MEDIA_PAD_FL_SINK;
 	state->pads[state->source_pad].flags = MEDIA_PAD_FL_SOURCE;
+	sd->entity.function = MEDIA_ENT_F_DV_DECODER;
 
 	err = media_entity_pads_init(&sd->entity, state->source_pad + 1,
 				state->pads);
diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
index fddac32e5051..99d781343fb1 100644
--- a/drivers/media/i2c/adv7842.c
+++ b/drivers/media/i2c/adv7842.c
@@ -3541,6 +3541,7 @@ static int adv7842_probe(struct i2c_client *client,
 	INIT_DELAYED_WORK(&state->delayed_work_enable_hotplug,
 			adv7842_delayed_work_enable_hotplug);
 
+	sd->entity.function = MEDIA_ENT_F_DV_DECODER;
 	state->pad.flags = MEDIA_PAD_FL_SOURCE;
 	err = media_entity_pads_init(&sd->entity, 1, &state->pad);
 	if (err)
diff --git a/drivers/media/i2c/tda1997x.c b/drivers/media/i2c/tda1997x.c
index 039a92c3294a..d114ac5243ec 100644
--- a/drivers/media/i2c/tda1997x.c
+++ b/drivers/media/i2c/tda1997x.c
@@ -2570,7 +2570,7 @@ static int tda1997x_probe(struct i2c_client *client,
 		 id->name, i2c_adapter_id(client->adapter),
 		 client->addr);
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
-	sd->entity.function = MEDIA_ENT_F_DTV_DECODER;
+	sd->entity.function = MEDIA_ENT_F_DV_DECODER;
 	sd->entity.ops = &tda1997x_media_ops;
 
 	/* set allowed mbus modes based on chip, bus-type, and bus-width */
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index ebd2cda67833..99f5e0978ebb 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -93,7 +93,7 @@ struct media_device_info {
  * Video decoder functions
  */
 #define MEDIA_ENT_F_ATV_DECODER			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 4)
-#define MEDIA_ENT_F_DTV_DECODER			(MEDIA_ENT_F_BASE + 0x6001)
+#define MEDIA_ENT_F_DV_DECODER			(MEDIA_ENT_F_BASE + 0x6001)
 
 /*
  * Digital TV, analog TV, radio and/or software defined radio tuner functions.
@@ -400,6 +400,8 @@ struct media_v2_topology {
 #define MEDIA_ENT_T_V4L2_SUBDEV_DECODER		MEDIA_ENT_F_ATV_DECODER
 #define MEDIA_ENT_T_V4L2_SUBDEV_TUNER		MEDIA_ENT_F_TUNER
 
+#define MEDIA_ENT_F_DTV_DECODER			MEDIA_ENT_F_DV_DECODER
+
 /*
  * There is still no ALSA support in the media controller. These
  * defines should not have been added and we leave them here only
-- 
2.17.0

Re: [PATCHv5 05/12] media: rename MEDIA_ENT_F_DTV_DECODER to MEDIA_ENT_F_DV_DECODER

[Ezequiel Garcia]

On 29 June 2018 at 08:43, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> From: Hans Verkuil <hansverk@cisco.com>
>
> The use of 'DTV' is very confusing since it normally refers to Digital
> TV e.g. DVB etc.
>
> Instead use 'DV' (Digital Video), which nicely corresponds to the
> DV Timings API used to configure such receivers and transmitters.
>
> We keep an alias to avoid breaking userspace applications.
>
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> ---
>  Documentation/media/uapi/mediactl/media-types.rst | 2 +-
>  drivers/media/i2c/adv7604.c                       | 1 +
>  drivers/media/i2c/adv7842.c                       | 1 +

It would be nice to mention in the commit log
that this patch also sets the function for these drivers.

Regards,
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCHv5 05/12] media: rename MEDIA_ENT_F_DTV_DECODER to MEDIA_ENT_F_DV_DECODER

[Laurent Pinchart]

Hello,

On Friday, 29 June 2018 20:40:49 EEST Ezequiel Garcia wrote:
> On 29 June 2018 at 08:43, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > From: Hans Verkuil <hansverk@cisco.com>
> > 
> > The use of 'DTV' is very confusing since it normally refers to Digital
> > TV e.g. DVB etc.
> > 
> > Instead use 'DV' (Digital Video), which nicely corresponds to the
> > DV Timings API used to configure such receivers and transmitters.
> > 
> > We keep an alias to avoid breaking userspace applications.
> > 
> > Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> > ---
> > 
> >  Documentation/media/uapi/mediactl/media-types.rst | 2 +-
> >  drivers/media/i2c/adv7604.c                       | 1 +
> >  drivers/media/i2c/adv7842.c                       | 1 +
> 
> It would be nice to mention in the commit log
> that this patch also sets the function for these drivers.

That's also my only concern with this patch (alternatively that change could 
be split to a separate patch).

-- 
Regards,

Laurent Pinchart

Re: [PATCHv5 05/12] media: rename MEDIA_ENT_F_DTV_DECODER to MEDIA_ENT_F_DV_DECODER

[Hans Verkuil]

On 09/07/18 15:00, Laurent Pinchart wrote:
> Hello,
> 
> On Friday, 29 June 2018 20:40:49 EEST Ezequiel Garcia wrote:
>> On 29 June 2018 at 08:43, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> From: Hans Verkuil <hansverk@cisco.com>
>>>
>>> The use of 'DTV' is very confusing since it normally refers to Digital
>>> TV e.g. DVB etc.
>>>
>>> Instead use 'DV' (Digital Video), which nicely corresponds to the
>>> DV Timings API used to configure such receivers and transmitters.
>>>
>>> We keep an alias to avoid breaking userspace applications.
>>>
>>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>>> ---
>>>
>>>  Documentation/media/uapi/mediactl/media-types.rst | 2 +-
>>>  drivers/media/i2c/adv7604.c                       | 1 +
>>>  drivers/media/i2c/adv7842.c                       | 1 +
>>
>> It would be nice to mention in the commit log
>> that this patch also sets the function for these drivers.
> 
> That's also my only concern with this patch (alternatively that change could 
> be split to a separate patch).
> 

I'll clarify the commit log. I can't split up this patch since the old define
is only available under #ifndef __KERNEL__, to prevent drivers from accidentally
using it in the kernel in the future.

Regards,

	Hans

Re: [PATCHv5 05/12] media: rename MEDIA_ENT_F_DTV_DECODER to MEDIA_ENT_F_DV_DECODER

[Laurent Pinchart]

Hi Hans,

On Monday, 9 July 2018 16:42:09 EEST Hans Verkuil wrote:
> On 09/07/18 15:00, Laurent Pinchart wrote:
> > On Friday, 29 June 2018 20:40:49 EEST Ezequiel Garcia wrote:
> >> On 29 June 2018 at 08:43, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>> From: Hans Verkuil <hansverk@cisco.com>
> >>> 
> >>> The use of 'DTV' is very confusing since it normally refers to Digital
> >>> TV e.g. DVB etc.
> >>> 
> >>> Instead use 'DV' (Digital Video), which nicely corresponds to the
> >>> DV Timings API used to configure such receivers and transmitters.
> >>> 
> >>> We keep an alias to avoid breaking userspace applications.
> >>> 
> >>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> >>> ---
> >>> 
> >>>  Documentation/media/uapi/mediactl/media-types.rst | 2 +-
> >>>  drivers/media/i2c/adv7604.c                       | 1 +
> >>>  drivers/media/i2c/adv7842.c                       | 1 +
> >> 
> >> It would be nice to mention in the commit log
> >> that this patch also sets the function for these drivers.
> > 
> > That's also my only concern with this patch (alternatively that change
> > could be split to a separate patch).
> 
> I'll clarify the commit log. I can't split up this patch since the old
> define is only available under #ifndef __KERNEL__, to prevent drivers from
> accidentally using it in the kernel in the future.

But the two drivers above don't use MEDIA_ENT_F_DTV_DECODER, do they ? Only 
drivers/media/i2c/tda1997x.c does.

-- 
Regards,

Laurent Pinchart

[PATCHv5 06/12] media.h: add MEDIA_ENT_F_DV_ENCODER

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add a new function for digital video encoders such as HDMI transmitters.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/media/uapi/mediactl/media-types.rst | 7 +++++++
 include/uapi/linux/media.h                        | 3 ++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/media/uapi/mediactl/media-types.rst b/Documentation/media/uapi/mediactl/media-types.rst
index c11b0c7e890b..e90d4d0a7f8b 100644
--- a/Documentation/media/uapi/mediactl/media-types.rst
+++ b/Documentation/media/uapi/mediactl/media-types.rst
@@ -206,6 +206,13 @@ Types and flags used to represent the media graph elements
 	  and output it in some digital video standard, with appropriate
 	  timing signals.
 
+    *  -  ``MEDIA_ENT_F_DV_ENCODER``
+       -  Digital video encoder. The basic function of the video encoder is
+	  to accept digital video from some digital video standard with
+	  appropriate timing signals (usually a parallel video bus with sync
+	  signals) and output this to a digital video output connector such
+	  as HDMI or DisplayPort.
+
 ..  tabularcolumns:: |p{5.5cm}|p{12.0cm}|
 
 .. _media-entity-flag:
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 99f5e0978ebb..6f594fa238c2 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -90,10 +90,11 @@ struct media_device_info {
 #define MEDIA_ENT_F_LENS			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 3)
 
 /*
- * Video decoder functions
+ * Video decoder/encoder functions
  */
 #define MEDIA_ENT_F_ATV_DECODER			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 4)
 #define MEDIA_ENT_F_DV_DECODER			(MEDIA_ENT_F_BASE + 0x6001)
+#define MEDIA_ENT_F_DV_ENCODER			(MEDIA_ENT_F_BASE + 0x6002)
 
 /*
  * Digital TV, analog TV, radio and/or software defined radio tuner functions.
-- 
2.17.0

Re: [PATCHv5 06/12] media.h: add MEDIA_ENT_F_DV_ENCODER

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday, 29 June 2018 14:43:25 EEST Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Add a new function for digital video encoders such as HDMI transmitters.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/media/uapi/mediactl/media-types.rst | 7 +++++++
>  include/uapi/linux/media.h                        | 3 ++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/media/uapi/mediactl/media-types.rst
> b/Documentation/media/uapi/mediactl/media-types.rst index
> c11b0c7e890b..e90d4d0a7f8b 100644
> --- a/Documentation/media/uapi/mediactl/media-types.rst
> +++ b/Documentation/media/uapi/mediactl/media-types.rst
> @@ -206,6 +206,13 @@ Types and flags used to represent the media graph
> elements and output it in some digital video standard, with appropriate
> timing signals.
> 
> +    *  -  ``MEDIA_ENT_F_DV_ENCODER``
> +       -  Digital video encoder. The basic function of the video encoder is
> +	  to accept digital video from some digital video standard with
> +	  appropriate timing signals (usually a parallel video bus with sync
> +	  signals) and output this to a digital video output connector such
> +	  as HDMI or DisplayPort.

This is slightly vague in my opinion, but not worse than the definition of 
MEDIA_ENT_F_DV_DECODER, so I'm fine with it.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  ..  tabularcolumns:: |p{5.5cm}|p{12.0cm}|
> 
>  .. _media-entity-flag:
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 99f5e0978ebb..6f594fa238c2 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -90,10 +90,11 @@ struct media_device_info {
>  #define MEDIA_ENT_F_LENS			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 3)
> 
>  /*
> - * Video decoder functions
> + * Video decoder/encoder functions
>   */
>  #define MEDIA_ENT_F_ATV_DECODER			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 4)
>  #define MEDIA_ENT_F_DV_DECODER			(MEDIA_ENT_F_BASE + 0x6001)
> +#define MEDIA_ENT_F_DV_ENCODER			(MEDIA_ENT_F_BASE + 0x6002)
> 
>  /*
>   * Digital TV, analog TV, radio and/or software defined radio tuner
> functions.

-- 
Regards,

Laurent Pinchart

[PATCHv5 07/12] media.h: reorder video en/decoder functions

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

Keep the function defines in numerical order: 0x6000 comes after
0x2000, so move it back.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 include/uapi/linux/media.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 6f594fa238c2..76d9bd64c116 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -89,13 +89,6 @@ struct media_device_info {
 #define MEDIA_ENT_F_FLASH			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 2)
 #define MEDIA_ENT_F_LENS			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 3)
 
-/*
- * Video decoder/encoder functions
- */
-#define MEDIA_ENT_F_ATV_DECODER			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 4)
-#define MEDIA_ENT_F_DV_DECODER			(MEDIA_ENT_F_BASE + 0x6001)
-#define MEDIA_ENT_F_DV_ENCODER			(MEDIA_ENT_F_BASE + 0x6002)
-
 /*
  * Digital TV, analog TV, radio and/or software defined radio tuner functions.
  *
@@ -140,6 +133,13 @@ struct media_device_info {
 #define MEDIA_ENT_F_VID_MUX			(MEDIA_ENT_F_BASE + 0x5001)
 #define MEDIA_ENT_F_VID_IF_BRIDGE		(MEDIA_ENT_F_BASE + 0x5002)
 
+/*
+ * Video decoder/encoder functions
+ */
+#define MEDIA_ENT_F_ATV_DECODER			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 4)
+#define MEDIA_ENT_F_DV_DECODER			(MEDIA_ENT_F_BASE + 0x6001)
+#define MEDIA_ENT_F_DV_ENCODER			(MEDIA_ENT_F_BASE + 0x6002)
+
 /* Entity flags */
 #define MEDIA_ENT_FL_DEFAULT			(1 << 0)
 #define MEDIA_ENT_FL_CONNECTOR			(1 << 1)
-- 
2.17.0

Re: [PATCHv5 07/12] media.h: reorder video en/decoder functions

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday, 29 June 2018 14:43:26 EEST Hans Verkuil wrote:
> From: Hans Verkuil <hansverk@cisco.com>
> 
> Keep the function defines in numerical order: 0x6000 comes after
> 0x2000, so move it back.
> 
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/uapi/linux/media.h | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 6f594fa238c2..76d9bd64c116 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -89,13 +89,6 @@ struct media_device_info {
>  #define MEDIA_ENT_F_FLASH			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 2)
>  #define MEDIA_ENT_F_LENS			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 3)
> 
> -/*
> - * Video decoder/encoder functions
> - */
> -#define MEDIA_ENT_F_ATV_DECODER			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 4)
> -#define MEDIA_ENT_F_DV_DECODER			(MEDIA_ENT_F_BASE + 0x6001)
> -#define MEDIA_ENT_F_DV_ENCODER			(MEDIA_ENT_F_BASE + 0x6002)
> -
>  /*
>   * Digital TV, analog TV, radio and/or software defined radio tuner
> functions. *
> @@ -140,6 +133,13 @@ struct media_device_info {
>  #define MEDIA_ENT_F_VID_MUX			(MEDIA_ENT_F_BASE + 0x5001)
>  #define MEDIA_ENT_F_VID_IF_BRIDGE		(MEDIA_ENT_F_BASE + 0x5002)
> 
> +/*
> + * Video decoder/encoder functions
> + */
> +#define MEDIA_ENT_F_ATV_DECODER			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 4)
> +#define MEDIA_ENT_F_DV_DECODER			(MEDIA_ENT_F_BASE + 0x6001)
> +#define MEDIA_ENT_F_DV_ENCODER			(MEDIA_ENT_F_BASE + 0x6002)
> +
>  /* Entity flags */
>  #define MEDIA_ENT_FL_DEFAULT			(1 << 0)
>  #define MEDIA_ENT_FL_CONNECTOR			(1 << 1)


-- 
Regards,

Laurent Pinchart

[PATCHv5 08/12] ad9389b/adv7511: set proper media entity function

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

These two drivers both have function MEDIA_ENT_F_DV_ENCODER.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ad9389b.c | 1 +
 drivers/media/i2c/adv7511.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/media/i2c/ad9389b.c b/drivers/media/i2c/ad9389b.c
index 91ff06088572..5b008b0002c0 100644
--- a/drivers/media/i2c/ad9389b.c
+++ b/drivers/media/i2c/ad9389b.c
@@ -1134,6 +1134,7 @@ static int ad9389b_probe(struct i2c_client *client, const struct i2c_device_id *
 		goto err_hdl;
 	}
 	state->pad.flags = MEDIA_PAD_FL_SINK;
+	sd->entity.function = MEDIA_ENT_F_DV_ENCODER;
 	err = media_entity_pads_init(&sd->entity, 1, &state->pad);
 	if (err)
 		goto err_hdl;
diff --git a/drivers/media/i2c/adv7511.c b/drivers/media/i2c/adv7511.c
index 5731751d3f2a..55c2ea0720d9 100644
--- a/drivers/media/i2c/adv7511.c
+++ b/drivers/media/i2c/adv7511.c
@@ -1847,6 +1847,7 @@ static int adv7511_probe(struct i2c_client *client, const struct i2c_device_id *
 		goto err_hdl;
 	}
 	state->pad.flags = MEDIA_PAD_FL_SINK;
+	sd->entity.function = MEDIA_ENT_F_DV_ENCODER;
 	err = media_entity_pads_init(&sd->entity, 1, &state->pad);
 	if (err)
 		goto err_hdl;
-- 
2.17.0

Re: [PATCHv5 08/12] ad9389b/adv7511: set proper media entity function

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday, 29 June 2018 14:43:27 EEST Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> These two drivers both have function MEDIA_ENT_F_DV_ENCODER.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

As this patch is separate from 06/12, I think it would make sense to split the 
driver changes from 05/12 to a separate patch.

> ---
>  drivers/media/i2c/ad9389b.c | 1 +
>  drivers/media/i2c/adv7511.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/media/i2c/ad9389b.c b/drivers/media/i2c/ad9389b.c
> index 91ff06088572..5b008b0002c0 100644
> --- a/drivers/media/i2c/ad9389b.c
> +++ b/drivers/media/i2c/ad9389b.c
> @@ -1134,6 +1134,7 @@ static int ad9389b_probe(struct i2c_client *client,
> const struct i2c_device_id * goto err_hdl;
>  	}
>  	state->pad.flags = MEDIA_PAD_FL_SINK;
> +	sd->entity.function = MEDIA_ENT_F_DV_ENCODER;
>  	err = media_entity_pads_init(&sd->entity, 1, &state->pad);
>  	if (err)
>  		goto err_hdl;
> diff --git a/drivers/media/i2c/adv7511.c b/drivers/media/i2c/adv7511.c
> index 5731751d3f2a..55c2ea0720d9 100644
> --- a/drivers/media/i2c/adv7511.c
> +++ b/drivers/media/i2c/adv7511.c
> @@ -1847,6 +1847,7 @@ static int adv7511_probe(struct i2c_client *client,
> const struct i2c_device_id * goto err_hdl;
>  	}
>  	state->pad.flags = MEDIA_PAD_FL_SINK;
> +	sd->entity.function = MEDIA_ENT_F_DV_ENCODER;
>  	err = media_entity_pads_init(&sd->entity, 1, &state->pad);
>  	if (err)
>  		goto err_hdl;


-- 
Regards,

Laurent Pinchart

Re: [PATCHv5 08/12] ad9389b/adv7511: set proper media entity function

[Hans Verkuil]

On 09/07/18 15:04, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Friday, 29 June 2018 14:43:27 EEST Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> These two drivers both have function MEDIA_ENT_F_DV_ENCODER.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> As this patch is separate from 06/12, I think it would make sense to split the 
> driver changes from 05/12 to a separate patch.

MEDIA_ENT_F_DV_ENCODER is new and hasn't been used before, so I can split
it in two patches.

MEDIA_ENT_F_DTV_DECODER however was already in use, and since this old define
disappeared under #ifndef __KERNEL__ drivers had to be changed in the same
patch to prevent bisect fails.

Regards,

	Hans

> 
>> ---
>>  drivers/media/i2c/ad9389b.c | 1 +
>>  drivers/media/i2c/adv7511.c | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ad9389b.c b/drivers/media/i2c/ad9389b.c
>> index 91ff06088572..5b008b0002c0 100644
>> --- a/drivers/media/i2c/ad9389b.c
>> +++ b/drivers/media/i2c/ad9389b.c
>> @@ -1134,6 +1134,7 @@ static int ad9389b_probe(struct i2c_client *client,
>> const struct i2c_device_id * goto err_hdl;
>>  	}
>>  	state->pad.flags = MEDIA_PAD_FL_SINK;
>> +	sd->entity.function = MEDIA_ENT_F_DV_ENCODER;
>>  	err = media_entity_pads_init(&sd->entity, 1, &state->pad);
>>  	if (err)
>>  		goto err_hdl;
>> diff --git a/drivers/media/i2c/adv7511.c b/drivers/media/i2c/adv7511.c
>> index 5731751d3f2a..55c2ea0720d9 100644
>> --- a/drivers/media/i2c/adv7511.c
>> +++ b/drivers/media/i2c/adv7511.c
>> @@ -1847,6 +1847,7 @@ static int adv7511_probe(struct i2c_client *client,
>> const struct i2c_device_id * goto err_hdl;
>>  	}
>>  	state->pad.flags = MEDIA_PAD_FL_SINK;
>> +	sd->entity.function = MEDIA_ENT_F_DV_ENCODER;
>>  	err = media_entity_pads_init(&sd->entity, 1, &state->pad);
>>  	if (err)
>>  		goto err_hdl;
> 
> 

[PATCHv5 09/12] adv7180/tvp514x/tvp7002: fix entity function

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The entity function was ORed with the flags field instead of
assigned to the function field. Correct this.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/adv7180.c | 2 +-
 drivers/media/i2c/tvp514x.c | 2 +-
 drivers/media/i2c/tvp7002.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 25d24a3f10a7..a727d7f806a1 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -1335,7 +1335,7 @@ static int adv7180_probe(struct i2c_client *client,
 		goto err_unregister_vpp_client;
 
 	state->pad.flags = MEDIA_PAD_FL_SOURCE;
-	sd->entity.flags |= MEDIA_ENT_F_ATV_DECODER;
+	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
 	ret = media_entity_pads_init(&sd->entity, 1, &state->pad);
 	if (ret)
 		goto err_free_ctrl;
diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
index 6a9890531d01..675b9ae212ab 100644
--- a/drivers/media/i2c/tvp514x.c
+++ b/drivers/media/i2c/tvp514x.c
@@ -1084,7 +1084,7 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	decoder->pad.flags = MEDIA_PAD_FL_SOURCE;
 	decoder->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-	decoder->sd.entity.flags |= MEDIA_ENT_F_ATV_DECODER;
+	decoder->sd.entity.function = MEDIA_ENT_F_ATV_DECODER;
 
 	ret = media_entity_pads_init(&decoder->sd.entity, 1, &decoder->pad);
 	if (ret < 0) {
diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
index 4599b7e28a8d..4f5c627579c7 100644
--- a/drivers/media/i2c/tvp7002.c
+++ b/drivers/media/i2c/tvp7002.c
@@ -1010,7 +1010,7 @@ static int tvp7002_probe(struct i2c_client *c, const struct i2c_device_id *id)
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	device->pad.flags = MEDIA_PAD_FL_SOURCE;
 	device->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-	device->sd.entity.flags |= MEDIA_ENT_F_ATV_DECODER;
+	device->sd.entity.function = MEDIA_ENT_F_ATV_DECODER;
 
 	error = media_entity_pads_init(&device->sd.entity, 1, &device->pad);
 	if (error < 0)
-- 
2.17.0

Re: [PATCHv5 09/12] adv7180/tvp514x/tvp7002: fix entity function

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday, 29 June 2018 14:43:28 EEST Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The entity function was ORed with the flags field instead of
> assigned to the function field. Correct this.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/adv7180.c | 2 +-
>  drivers/media/i2c/tvp514x.c | 2 +-
>  drivers/media/i2c/tvp7002.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 25d24a3f10a7..a727d7f806a1 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -1335,7 +1335,7 @@ static int adv7180_probe(struct i2c_client *client,
>  		goto err_unregister_vpp_client;
> 
>  	state->pad.flags = MEDIA_PAD_FL_SOURCE;
> -	sd->entity.flags |= MEDIA_ENT_F_ATV_DECODER;
> +	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
>  	ret = media_entity_pads_init(&sd->entity, 1, &state->pad);
>  	if (ret)
>  		goto err_free_ctrl;
> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
> index 6a9890531d01..675b9ae212ab 100644
> --- a/drivers/media/i2c/tvp514x.c
> +++ b/drivers/media/i2c/tvp514x.c
> @@ -1084,7 +1084,7 @@ tvp514x_probe(struct i2c_client *client, const struct
> i2c_device_id *id) #if defined(CONFIG_MEDIA_CONTROLLER)
>  	decoder->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	decoder->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> -	decoder->sd.entity.flags |= MEDIA_ENT_F_ATV_DECODER;
> +	decoder->sd.entity.function = MEDIA_ENT_F_ATV_DECODER;
> 
>  	ret = media_entity_pads_init(&decoder->sd.entity, 1, &decoder->pad);
>  	if (ret < 0) {
> diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
> index 4599b7e28a8d..4f5c627579c7 100644
> --- a/drivers/media/i2c/tvp7002.c
> +++ b/drivers/media/i2c/tvp7002.c
> @@ -1010,7 +1010,7 @@ static int tvp7002_probe(struct i2c_client *c, const
> struct i2c_device_id *id) #if defined(CONFIG_MEDIA_CONTROLLER)
>  	device->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	device->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> -	device->sd.entity.flags |= MEDIA_ENT_F_ATV_DECODER;
> +	device->sd.entity.function = MEDIA_ENT_F_ATV_DECODER;
> 
>  	error = media_entity_pads_init(&device->sd.entity, 1, &device->pad);
>  	if (error < 0)


-- 
Regards,

Laurent Pinchart

[PATCHv5 10/12] media/i2c: add missing entity functions

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Several drivers in media/i2c do not set the entity function.
Correct this.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/et8ek8/et8ek8_driver.c | 1 +
 drivers/media/i2c/mt9m032.c              | 1 +
 drivers/media/i2c/mt9p031.c              | 1 +
 drivers/media/i2c/mt9t001.c              | 1 +
 drivers/media/i2c/mt9v032.c              | 1 +
 5 files changed, 5 insertions(+)

diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
index e9eff9039ef5..37ef38947e01 100644
--- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
+++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
@@ -1446,6 +1446,7 @@ static int et8ek8_probe(struct i2c_client *client,
 	sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	sensor->subdev.internal_ops = &et8ek8_internal_ops;
 
+	sensor->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&sensor->subdev.entity, 1, &sensor->pad);
 	if (ret < 0) {
diff --git a/drivers/media/i2c/mt9m032.c b/drivers/media/i2c/mt9m032.c
index 6a9e068462fd..b385f2b632ad 100644
--- a/drivers/media/i2c/mt9m032.c
+++ b/drivers/media/i2c/mt9m032.c
@@ -793,6 +793,7 @@ static int mt9m032_probe(struct i2c_client *client,
 	v4l2_ctrl_cluster(2, &sensor->hflip);
 
 	sensor->subdev.ctrl_handler = &sensor->ctrls;
+	sensor->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&sensor->subdev.entity, 1, &sensor->pad);
 	if (ret < 0)
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 91d822fc4443..715be3632b01 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -1111,6 +1111,7 @@ static int mt9p031_probe(struct i2c_client *client,
 	v4l2_i2c_subdev_init(&mt9p031->subdev, client, &mt9p031_subdev_ops);
 	mt9p031->subdev.internal_ops = &mt9p031_subdev_internal_ops;
 
+	mt9p031->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 	mt9p031->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&mt9p031->subdev.entity, 1, &mt9p031->pad);
 	if (ret < 0)
diff --git a/drivers/media/i2c/mt9t001.c b/drivers/media/i2c/mt9t001.c
index 9d981d9f5686..f683d2cb0486 100644
--- a/drivers/media/i2c/mt9t001.c
+++ b/drivers/media/i2c/mt9t001.c
@@ -943,6 +943,7 @@ static int mt9t001_probe(struct i2c_client *client,
 	mt9t001->subdev.internal_ops = &mt9t001_subdev_internal_ops;
 	mt9t001->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
+	mt9t001->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 	mt9t001->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&mt9t001->subdev.entity, 1, &mt9t001->pad);
 
diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
index 4de63b2df334..f74730d24d8f 100644
--- a/drivers/media/i2c/mt9v032.c
+++ b/drivers/media/i2c/mt9v032.c
@@ -1162,6 +1162,7 @@ static int mt9v032_probe(struct i2c_client *client,
 	mt9v032->subdev.internal_ops = &mt9v032_subdev_internal_ops;
 	mt9v032->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
+	mt9v032->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 	mt9v032->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&mt9v032->subdev.entity, 1, &mt9v032->pad);
 	if (ret < 0)
-- 
2.17.0

Re: [PATCHv5 10/12] media/i2c: add missing entity functions

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday, 29 June 2018 14:43:29 EEST Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Several drivers in media/i2c do not set the entity function.
> Correct this.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/et8ek8/et8ek8_driver.c | 1 +
>  drivers/media/i2c/mt9m032.c              | 1 +
>  drivers/media/i2c/mt9p031.c              | 1 +
>  drivers/media/i2c/mt9t001.c              | 1 +
>  drivers/media/i2c/mt9v032.c              | 1 +
>  5 files changed, 5 insertions(+)
> 
> diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c
> b/drivers/media/i2c/et8ek8/et8ek8_driver.c index e9eff9039ef5..37ef38947e01
> 100644
> --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
> +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> @@ -1446,6 +1446,7 @@ static int et8ek8_probe(struct i2c_client *client,
>  	sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  	sensor->subdev.internal_ops = &et8ek8_internal_ops;
> 
> +	sensor->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ret = media_entity_pads_init(&sensor->subdev.entity, 1, &sensor->pad);
>  	if (ret < 0) {
> diff --git a/drivers/media/i2c/mt9m032.c b/drivers/media/i2c/mt9m032.c
> index 6a9e068462fd..b385f2b632ad 100644
> --- a/drivers/media/i2c/mt9m032.c
> +++ b/drivers/media/i2c/mt9m032.c
> @@ -793,6 +793,7 @@ static int mt9m032_probe(struct i2c_client *client,
>  	v4l2_ctrl_cluster(2, &sensor->hflip);
> 
>  	sensor->subdev.ctrl_handler = &sensor->ctrls;
> +	sensor->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ret = media_entity_pads_init(&sensor->subdev.entity, 1, &sensor->pad);
>  	if (ret < 0)
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 91d822fc4443..715be3632b01 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -1111,6 +1111,7 @@ static int mt9p031_probe(struct i2c_client *client,
>  	v4l2_i2c_subdev_init(&mt9p031->subdev, client, &mt9p031_subdev_ops);
>  	mt9p031->subdev.internal_ops = &mt9p031_subdev_internal_ops;
> 
> +	mt9p031->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  	mt9p031->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ret = media_entity_pads_init(&mt9p031->subdev.entity, 1, &mt9p031->pad);
>  	if (ret < 0)
> diff --git a/drivers/media/i2c/mt9t001.c b/drivers/media/i2c/mt9t001.c
> index 9d981d9f5686..f683d2cb0486 100644
> --- a/drivers/media/i2c/mt9t001.c
> +++ b/drivers/media/i2c/mt9t001.c
> @@ -943,6 +943,7 @@ static int mt9t001_probe(struct i2c_client *client,
>  	mt9t001->subdev.internal_ops = &mt9t001_subdev_internal_ops;
>  	mt9t001->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> 
> +	mt9t001->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  	mt9t001->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ret = media_entity_pads_init(&mt9t001->subdev.entity, 1, &mt9t001->pad);
> 
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index 4de63b2df334..f74730d24d8f 100644
> --- a/drivers/media/i2c/mt9v032.c
> +++ b/drivers/media/i2c/mt9v032.c
> @@ -1162,6 +1162,7 @@ static int mt9v032_probe(struct i2c_client *client,
>  	mt9v032->subdev.internal_ops = &mt9v032_subdev_internal_ops;
>  	mt9v032->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> 
> +	mt9v032->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  	mt9v032->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ret = media_entity_pads_init(&mt9v032->subdev.entity, 1, &mt9v032->pad);
>  	if (ret < 0)


-- 
Regards,

Laurent Pinchart

[PATCHv5 11/12] media-ioc-enum-links.rst: improve pad index description

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

Make it clearer that the index starts at 0, and that it won't change
since future new pads will be added at the end.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 Documentation/media/uapi/mediactl/media-ioc-enum-links.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst b/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst
index 17abdeed1a9c..4cceeb8a6f73 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst
@@ -92,7 +92,9 @@ returned during the enumeration process.
 
     *  -  __u16
        -  ``index``
-       -  0-based pad index.
+       -  Pad index, starts at 0. Pad indices are stable. If new pads are added
+	  for an entity in the future, then those will be added at the end of the
+	  entity's pad array.
 
     *  -  __u32
        -  ``flags``
-- 
2.17.0

Re: [PATCHv5 11/12] media-ioc-enum-links.rst: improve pad index description

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday, 29 June 2018 14:43:30 EEST Hans Verkuil wrote:
> From: Hans Verkuil <hansverk@cisco.com>
> 
> Make it clearer that the index starts at 0, and that it won't change
> since future new pads will be added at the end.
> 
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> ---
>  Documentation/media/uapi/mediactl/media-ioc-enum-links.rst | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst
> b/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst index
> 17abdeed1a9c..4cceeb8a6f73 100644
> --- a/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst
> +++ b/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst
> @@ -92,7 +92,9 @@ returned during the enumeration process.
> 
>      *  -  __u16
>         -  ``index``
> -       -  0-based pad index.
> +       -  Pad index, starts at 0. Pad indices are stable. If new pads are
> added
> +	  for an entity in the future, then those will be added at the end of the
> +	  entity's pad array.

Is that true strictly speaking ? We do mandate pad indices to be stable, but 
couldn't new pads still be inserted in the array ? The array wouldn't be 
sorted by pad index anymore, but I don't think we require that. If we want to 
I don't have any objection, but it should then be documented.

>      *  -  __u32
>         -  ``flags``

-- 
Regards,

Laurent Pinchart

Re: [PATCHv5 11/12] media-ioc-enum-links.rst: improve pad index description

[Hans Verkuil]

On 09/07/18 15:10, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Friday, 29 June 2018 14:43:30 EEST Hans Verkuil wrote:
>> From: Hans Verkuil <hansverk@cisco.com>
>>
>> Make it clearer that the index starts at 0, and that it won't change
>> since future new pads will be added at the end.
>>
>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>> ---
>>  Documentation/media/uapi/mediactl/media-ioc-enum-links.rst | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst
>> b/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst index
>> 17abdeed1a9c..4cceeb8a6f73 100644
>> --- a/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst
>> +++ b/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst
>> @@ -92,7 +92,9 @@ returned during the enumeration process.
>>
>>      *  -  __u16
>>         -  ``index``
>> -       -  0-based pad index.
>> +       -  Pad index, starts at 0. Pad indices are stable. If new pads are
>> added
>> +	  for an entity in the future, then those will be added at the end of the
>> +	  entity's pad array.
> 
> Is that true strictly speaking ? We do mandate pad indices to be stable, but 
> couldn't new pads still be inserted in the array ? The array wouldn't be 
> sorted by pad index anymore, but I don't think we require that. If we want to 
> I don't have any objection, but it should then be documented.

You are right, you might have two pads with indices 0 and 3, then add a new pad
at index 2. As long as existing pad indices remain stable (i.e. are never renumbered)
you are fine.

I'll rephrase this.

Regards,

	Hans

> 
>>      *  -  __u32
>>         -  ``flags``
> 

[PATCHv5 12/12] media-ioc-enum-entities.rst/-g-topology.rst: clarify ID/name usage

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

Mention that IDs should not be hardcoded in applications and that the
entity name must be unique within the media topology.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 .../uapi/mediactl/media-ioc-enum-entities.rst |  9 +++++---
 .../uapi/mediactl/media-ioc-g-topology.rst    | 22 ++++++++++++++-----
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst b/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
index 961466ae821d..a4aa7deef690 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
@@ -62,15 +62,18 @@ id's until they get an error.
        -  ``id``
        -
        -
-       -  Entity id, set by the application. When the id is or'ed with
+       -  Entity id, set by the application. When the ID is or'ed with
 	  ``MEDIA_ENT_ID_FLAG_NEXT``, the driver clears the flag and returns
-	  the first entity with a larger id.
+	  the first entity with a larger ID. Do not expect that the ID will
+	  always be the same for each instance of the device. In other words,
+	  do not hardcode entity IDs in an application.
 
     *  -  char
        -  ``name``\ [32]
        -
        -
-       -  Entity name as an UTF-8 NULL-terminated string.
+       -  Entity name as an UTF-8 NULL-terminated string. This name must be unique
+          within the media topology.
 
     *  -  __u32
        -  ``type``
diff --git a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
index e572dd0d806d..3a5f165d9811 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
@@ -131,11 +131,14 @@ desired arrays with the media graph elements.
 
     *  -  __u32
        -  ``id``
-       -  Unique ID for the entity.
+       -  Unique ID for the entity. Do not expect that the ID will
+	  always be the same for each instance of the device. In other words,
+	  do not hardcode entity IDs in an application.
 
     *  -  char
        -  ``name``\ [64]
-       -  Entity name as an UTF-8 NULL-terminated string.
+       -  Entity name as an UTF-8 NULL-terminated string. This name must be unique
+          within the media topology.
 
     *  -  __u32
        -  ``function``
@@ -166,7 +169,9 @@ desired arrays with the media graph elements.
 
     *  -  __u32
        -  ``id``
-       -  Unique ID for the interface.
+       -  Unique ID for the interface. Do not expect that the ID will
+	  always be the same for each instance of the device. In other words,
+	  do not hardcode interface IDs in an application.
 
     *  -  __u32
        -  ``intf_type``
@@ -215,7 +220,9 @@ desired arrays with the media graph elements.
 
     *  -  __u32
        -  ``id``
-       -  Unique ID for the pad.
+       -  Unique ID for the pad. Do not expect that the ID will
+	  always be the same for each instance of the device. In other words,
+	  do not hardcode pad IDs in an application.
 
     *  -  __u32
        -  ``entity_id``
@@ -231,7 +238,8 @@ desired arrays with the media graph elements.
 	  returns true. The ``media_version`` is defined in struct
 	  :c:type:`media_device_info` and can be retrieved using
 	  :ref:`MEDIA_IOC_DEVICE_INFO`. Pad indices are stable. If new pads are added
-	  for an entity in the future, then those will be added at the end.
+	  for an entity in the future, then those will be added at the end of the
+	  entity's pad array.
 
     *  -  __u32
        -  ``reserved``\ [4]
@@ -250,7 +258,9 @@ desired arrays with the media graph elements.
 
     *  -  __u32
        -  ``id``
-       -  Unique ID for the link.
+       -  Unique ID for the link. Do not expect that the ID will
+	  always be the same for each instance of the device. In other words,
+	  do not hardcode link IDs in an application.
 
     *  -  __u32
        -  ``source_id``
-- 
2.17.0

Re: [PATCHv5 12/12] media-ioc-enum-entities.rst/-g-topology.rst: clarify ID/name usage

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday, 29 June 2018 14:43:31 EEST Hans Verkuil wrote:
> From: Hans Verkuil <hansverk@cisco.com>
> 
> Mention that IDs should not be hardcoded in applications and that the
> entity name must be unique within the media topology.
> 
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> ---
>  .../uapi/mediactl/media-ioc-enum-entities.rst |  9 +++++---
>  .../uapi/mediactl/media-ioc-g-topology.rst    | 22 ++++++++++++++-----
>  2 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
> b/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst index
> 961466ae821d..a4aa7deef690 100644
> --- a/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
> +++ b/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
> @@ -62,15 +62,18 @@ id's until they get an error.
>         -  ``id``
>         -
>         -
> -       -  Entity id, set by the application. When the id is or'ed with
> +       -  Entity id, set by the application. When the ID is or'ed with

Should you also s/Entity id/Entity ID/ for consistency ?

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	  ``MEDIA_ENT_ID_FLAG_NEXT``, the driver clears the flag and returns
> -	  the first entity with a larger id.
> +	  the first entity with a larger ID. Do not expect that the ID will
> +	  always be the same for each instance of the device. In other words,
> +	  do not hardcode entity IDs in an application.
> 
>      *  -  char
>         -  ``name``\ [32]
>         -
>         -
> -       -  Entity name as an UTF-8 NULL-terminated string.
> +       -  Entity name as an UTF-8 NULL-terminated string. This name must be
> unique
> +          within the media topology.
> 
>      *  -  __u32
>         -  ``type``
> diff --git a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst index
> e572dd0d806d..3a5f165d9811 100644
> --- a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> +++ b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> @@ -131,11 +131,14 @@ desired arrays with the media graph elements.
> 
>      *  -  __u32
>         -  ``id``
> -       -  Unique ID for the entity.
> +       -  Unique ID for the entity. Do not expect that the ID will
> +	  always be the same for each instance of the device. In other words,
> +	  do not hardcode entity IDs in an application.
> 
>      *  -  char
>         -  ``name``\ [64]
> -       -  Entity name as an UTF-8 NULL-terminated string.
> +       -  Entity name as an UTF-8 NULL-terminated string. This name must be
> unique
> +          within the media topology.
> 
>      *  -  __u32
>         -  ``function``
> @@ -166,7 +169,9 @@ desired arrays with the media graph elements.
> 
>      *  -  __u32
>         -  ``id``
> -       -  Unique ID for the interface.
> +       -  Unique ID for the interface. Do not expect that the ID will
> +	  always be the same for each instance of the device. In other words,
> +	  do not hardcode interface IDs in an application.
> 
>      *  -  __u32
>         -  ``intf_type``
> @@ -215,7 +220,9 @@ desired arrays with the media graph elements.
> 
>      *  -  __u32
>         -  ``id``
> -       -  Unique ID for the pad.
> +       -  Unique ID for the pad. Do not expect that the ID will
> +	  always be the same for each instance of the device. In other words,
> +	  do not hardcode pad IDs in an application.
> 
>      *  -  __u32
>         -  ``entity_id``
> @@ -231,7 +238,8 @@ desired arrays with the media graph elements.
>  	  returns true. The ``media_version`` is defined in struct
> 
>  	  :c:type:`media_device_info` and can be retrieved using
>  	  :ref:`MEDIA_IOC_DEVICE_INFO`. Pad indices are stable. If new pads are
>  	  :added
> 
> -	  for an entity in the future, then those will be added at the end.
> +	  for an entity in the future, then those will be added at the end of the
> +	  entity's pad array.
> 
>      *  -  __u32
>         -  ``reserved``\ [4]
> @@ -250,7 +258,9 @@ desired arrays with the media graph elements.
> 
>      *  -  __u32
>         -  ``id``
> -       -  Unique ID for the link.
> +       -  Unique ID for the link. Do not expect that the ID will
> +	  always be the same for each instance of the device. In other words,
> +	  do not hardcode link IDs in an application.
> 
>      *  -  __u32
>         -  ``source_id``


-- 
Regards,

Laurent Pinchart