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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
	linux-media@vger.kernel.org,
	Jacopo Mondi <jacopo+renesas@jmondi.org>,
	niklas.soderlund+renesas@ragnatech.se,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>
Subject: Re: [PATCH v5 15/24] v4l: Add bus type to frame descriptors
Date: Thu, 29 Apr 2021 17:09:26 +0300	[thread overview]
Message-ID: <YIq+FlDs7T+5FjCN@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20210429115811.GV3@paasikivi.fi.intel.com>

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