Re: [PATCH 3/4] media: imx: imx-mipi-csis: Implement the .get_frame_desc() operation

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Rui,

On Thu, Jun 16, 2022 at 12:53:14AM +0100, Rui Miguel Silva wrote:
> On Wed, Jun 15, 2022 at 10:26:01PM +0300, Laurent Pinchart wrote:
> > The CSIS is connected to its sink through an SoC-specific gasket that
> > needs to be configured. Depending on the platform, the gasket
> > configuration requires knowing the CSI-2 DT. To provide the needed
> > information, implement the .get_frame_desc() operation.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/platform/nxp/imx-mipi-csis.c | 27 ++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > index 8674aaad5fa0..905072871ed2 100644
> > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > @@ -1160,6 +1160,32 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > +static int mipi_csis_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > +				    struct v4l2_mbus_frame_desc *fd)
> > +{
> > +	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > +	struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[0];
> 
> Think that you should check *fd before use here, other than that.

fd isn't supposed to be null, the same way sd isn't, or the various
format pointers passed to other subdev operations aren't. We don't check
those either, that's why I haven't checked fd here.

> Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com>
> 
> > +
> > +	if (pad != CSIS_PAD_SOURCE)
> > +		return -EINVAL;
> > +
> > +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL;
> > +	fd->num_entries = 1;
> > +
> > +	memset(entry, 0, sizeof(*entry));
> > +
> > +	mutex_lock(&csis->lock);
> > +
> > +	entry->flags = 0;
> > +	entry->pixelcode = csis->csis_fmt->code;
> > +	entry->bus.csi2.vc = 0;
> > +	entry->bus.csi2.dt = csis->csis_fmt->data_type;
> > +
> > +	mutex_unlock(&csis->lock);
> > +
> > +	return 0;
> > +}
> > +
> >  static int mipi_csis_log_status(struct v4l2_subdev *sd)
> >  {
> >  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > @@ -1184,6 +1210,7 @@ static const struct v4l2_subdev_pad_ops mipi_csis_pad_ops = {
> >  	.enum_mbus_code		= mipi_csis_enum_mbus_code,
> >  	.get_fmt		= mipi_csis_get_fmt,
> >  	.set_fmt		= mipi_csis_set_fmt,
> > +	.get_frame_desc		= mipi_csis_get_frame_desc,
> >  };
> >  
> >  static const struct v4l2_subdev_ops mipi_csis_subdev_ops = {

-- 
Regards,

Laurent Pinchart