diff mbox series

[v5,15/24] v4l: Add bus type to frame descriptors

Message ID 20210415130450.421168-16-tomi.valkeinen@ideasonboard.com (mailing list archive)
State New
Headers show
Series v4l: subdev internal routing | expand

Commit Message

Tomi Valkeinen April 15, 2021, 1:04 p.m. UTC
From: Sakari Ailus <sakari.ailus@linux.intel.com>

Add the media bus type to the frame descriptor. CSI-2 specific
information will be added in next patch to the frame descriptor.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

- Make the bus type a named enum
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/media/v4l2-subdev.h | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Laurent Pinchart April 18, 2021, 7:23 p.m. UTC | #1
Hi Tomi and Sakari,

Thank you for the patch.

On Thu, Apr 15, 2021 at 04:04:41PM +0300, Tomi Valkeinen wrote:
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Add the media bus type to the frame descriptor. CSI-2 specific
> information will be added in next patch to the frame descriptor.

I'd squash the next patch with this one.

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> - Make the bus type a named enum
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d0e9a5bdb08b..85977abbea46 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -340,12 +340,21 @@ struct v4l2_mbus_frame_desc_entry {
>  
>  #define V4L2_FRAME_DESC_ENTRY_MAX	4
>  
> +enum v4l2_mbus_frame_desc_type {
> +	V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
> +	V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
> +	V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
> +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
> +};

This should be documented. In particular, I have no idea what
V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM is. I also wonder if we shouldn't
drop CCP2 (at least for now), does anyone use that anymore ?

> +
>  /**
>   * struct v4l2_mbus_frame_desc - media bus data frame description
> + * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
>   * @entry: frame descriptors array
>   * @num_entries: number of entries in @entry array
>   */
>  struct v4l2_mbus_frame_desc {
> +	enum v4l2_mbus_frame_desc_type type;
>  	struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
>  	unsigned short num_entries;
>  };
Sakari Ailus April 20, 2021, 11:50 a.m. UTC | #2
Hi Laurent,

On Sun, Apr 18, 2021 at 10:23:35PM +0300, Laurent Pinchart wrote:
> Hi Tomi and Sakari,
> 
> Thank you for the patch.
> 
> On Thu, Apr 15, 2021 at 04:04:41PM +0300, Tomi Valkeinen wrote:
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > Add the media bus type to the frame descriptor. CSI-2 specific
> > information will be added in next patch to the frame descriptor.
> 
> I'd squash the next patch with this one.
> 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > - Make the bus type a named enum
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/media/v4l2-subdev.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index d0e9a5bdb08b..85977abbea46 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -340,12 +340,21 @@ struct v4l2_mbus_frame_desc_entry {
> >  
> >  #define V4L2_FRAME_DESC_ENTRY_MAX	4
> >  
> > +enum v4l2_mbus_frame_desc_type {
> > +	V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
> > +	V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
> > +	V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
> > +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
> > +};
> 
> This should be documented. In particular, I have no idea what
> V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM is. I also wonder if we shouldn't
> drop CCP2 (at least for now), does anyone use that anymore ?

I guess we don't need one here, not now at least.

I agree on the documentation.

> 
> > +
> >  /**
> >   * struct v4l2_mbus_frame_desc - media bus data frame description
> > + * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
> >   * @entry: frame descriptors array
> >   * @num_entries: number of entries in @entry array
> >   */
> >  struct v4l2_mbus_frame_desc {
> > +	enum v4l2_mbus_frame_desc_type type;
> >  	struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
> >  	unsigned short num_entries;
> >  };
>
Tomi Valkeinen April 22, 2021, 12:30 p.m. UTC | #3
On 20/04/2021 14:50, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Sun, Apr 18, 2021 at 10:23:35PM +0300, Laurent Pinchart wrote:
>> Hi Tomi and Sakari,
>>
>> Thank you for the patch.
>>
>> On Thu, Apr 15, 2021 at 04:04:41PM +0300, Tomi Valkeinen wrote:
>>> From: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>
>>> Add the media bus type to the frame descriptor. CSI-2 specific
>>> information will be added in next patch to the frame descriptor.
>>
>> I'd squash the next patch with this one.
>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>
>>> - Make the bus type a named enum
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>   include/media/v4l2-subdev.h | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>> index d0e9a5bdb08b..85977abbea46 100644
>>> --- a/include/media/v4l2-subdev.h
>>> +++ b/include/media/v4l2-subdev.h
>>> @@ -340,12 +340,21 @@ struct v4l2_mbus_frame_desc_entry {
>>>   
>>>   #define V4L2_FRAME_DESC_ENTRY_MAX	4
>>>   
>>> +enum v4l2_mbus_frame_desc_type {
>>> +	V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
>>> +	V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
>>> +	V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
>>> +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
>>> +};
>>
>> This should be documented. In particular, I have no idea what
>> V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM is. I also wonder if we shouldn't
>> drop CCP2 (at least for now), does anyone use that anymore ?
> 
> I guess we don't need one here, not now at least.
> 
> I agree on the documentation.

As it's the first one in the list, I think it really means "undefined", 
so that current users have a value there (I presume they initialize the 
struct to 0). Sakari?

  Tomi
Sakari Ailus April 29, 2021, 11:58 a.m. UTC | #4
On Thu, Apr 22, 2021 at 03:30:55PM +0300, Tomi Valkeinen wrote:
> On 20/04/2021 14:50, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > On Sun, Apr 18, 2021 at 10:23:35PM +0300, Laurent Pinchart wrote:
> > > Hi Tomi and Sakari,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Thu, Apr 15, 2021 at 04:04:41PM +0300, Tomi Valkeinen wrote:
> > > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > 
> > > > Add the media bus type to the frame descriptor. CSI-2 specific
> > > > information will be added in next patch to the frame descriptor.
> > > 
> > > I'd squash the next patch with this one.
> > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > 
> > > > - Make the bus type a named enum
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >   include/media/v4l2-subdev.h | 9 +++++++++
> > > >   1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index d0e9a5bdb08b..85977abbea46 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -340,12 +340,21 @@ struct v4l2_mbus_frame_desc_entry {
> > > >   #define V4L2_FRAME_DESC_ENTRY_MAX	4
> > > > +enum v4l2_mbus_frame_desc_type {
> > > > +	V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
> > > > +	V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
> > > > +	V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
> > > > +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
> > > > +};
> > > 
> > > This should be documented. In particular, I have no idea what
> > > V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM is. I also wonder if we shouldn't
> > > drop CCP2 (at least for now), does anyone use that anymore ?
> > 
> > I guess we don't need one here, not now at least.
> > 
> > I agree on the documentation.
> 
> As it's the first one in the list, I think it really means "undefined", so
> that current users have a value there (I presume they initialize the struct
> to 0). Sakari?

I guess you could drop PLATFORM if there's no need for it now. In practice
PARALLEL is probably a good choice.
Laurent Pinchart April 29, 2021, 2:09 p.m. UTC | #5
Hi Sakari,

On Thu, Apr 29, 2021 at 02:58:11PM +0300, Sakari Ailus wrote:
> On Thu, Apr 22, 2021 at 03:30:55PM +0300, Tomi Valkeinen wrote:
> > On 20/04/2021 14:50, Sakari Ailus wrote:
> > > On Sun, Apr 18, 2021 at 10:23:35PM +0300, Laurent Pinchart wrote:
> > > > On Thu, Apr 15, 2021 at 04:04:41PM +0300, Tomi Valkeinen wrote:
> > > > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > 
> > > > > Add the media bus type to the frame descriptor. CSI-2 specific
> > > > > information will be added in next patch to the frame descriptor.
> > > > 
> > > > I'd squash the next patch with this one.
> > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > > 
> > > > > - Make the bus type a named enum
> > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > ---
> > > > >   include/media/v4l2-subdev.h | 9 +++++++++
> > > > >   1 file changed, 9 insertions(+)
> > > > > 
> > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > > index d0e9a5bdb08b..85977abbea46 100644
> > > > > --- a/include/media/v4l2-subdev.h
> > > > > +++ b/include/media/v4l2-subdev.h
> > > > > @@ -340,12 +340,21 @@ struct v4l2_mbus_frame_desc_entry {
> > > > >   #define V4L2_FRAME_DESC_ENTRY_MAX	4
> > > > > +enum v4l2_mbus_frame_desc_type {
> > > > > +	V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
> > > > > +	V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
> > > > > +	V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
> > > > > +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
> > > > > +};
> > > > 
> > > > This should be documented. In particular, I have no idea what
> > > > V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM is. I also wonder if we shouldn't
> > > > drop CCP2 (at least for now), does anyone use that anymore ?
> > > 
> > > I guess we don't need one here, not now at least.
> > > 
> > > I agree on the documentation.
> > 
> > As it's the first one in the list, I think it really means "undefined", so
> > that current users have a value there (I presume they initialize the struct
> > to 0). Sakari?
> 
> I guess you could drop PLATFORM if there's no need for it now. In practice
> PARALLEL is probably a good choice.

Works for me too.
diff mbox series

Patch

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d0e9a5bdb08b..85977abbea46 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -340,12 +340,21 @@  struct v4l2_mbus_frame_desc_entry {
 
 #define V4L2_FRAME_DESC_ENTRY_MAX	4
 
+enum v4l2_mbus_frame_desc_type {
+	V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
+	V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
+	V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
+	V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
+};
+
 /**
  * struct v4l2_mbus_frame_desc - media bus data frame description
+ * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
  * @entry: frame descriptors array
  * @num_entries: number of entries in @entry array
  */
 struct v4l2_mbus_frame_desc {
+	enum v4l2_mbus_frame_desc_type type;
 	struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
 	unsigned short num_entries;
 };