| Message ID | 20210415130450.421168-16-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | v4l: subdev internal routing | expand |
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; > }; -- Regards, Laurent Pinchart
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; > > }; > -- Sakari Ailus
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
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. -- Sakari Ailus
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. -- Regards, Laurent Pinchart
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; };