Re: [PATCH v3 12/31] media: staging: media: imx6-mipi-csi2: move sd imx6's specific initialization into imx6-sci2.c

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

On Mon, Oct 27, 2025 at 03:44:49AM +0200, Laurent Pinchart wrote:
> On Thu, Aug 21, 2025 at 04:15:47PM -0400, Frank Li wrote:
> > Move imx6's specific sd's owner, name, csi2_internal_ops, grp_id into
> 
> grp_id needs to go. It's specific to the i.MX6 IPUv3 framework, and is
> the main part hindering splitting the CSI-2 receiver driver from the
> rest of the code.
> 
> > imx6-sci2.c.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/staging/media/imx/imx6-csi2.c      | 23 +++++++++++++++++++++++
> >  drivers/staging/media/imx/imx6-mipi-csi2.c | 29 ++++++-----------------------
> >  include/media/dw-mipi-csi2.h               |  4 ++++
> >  3 files changed, 33 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx6-csi2.c b/drivers/staging/media/imx/imx6-csi2.c
> > index 72eff685fad99b3def46cf2866565191b7de9a8b..66274d8d73b67b35682bb82a9eb745bb24da7ae4 100644
> > --- a/drivers/staging/media/imx/imx6-csi2.c
> > +++ b/drivers/staging/media/imx/imx6-csi2.c
> > @@ -19,6 +19,29 @@ struct imx6_csi2 {
> >  	struct dw_mipi_csi2_dev dw;
> >  };
> >  
> > +static int csi2_registered(struct v4l2_subdev *sd)
> > +{
> > +	struct dw_mipi_csi2_dev *csi2 = sd_to_dw_mipi_csi2_dev(sd);
> > +
> > +	/* set a default mbus format  */
> > +	return imx_media_init_mbus_fmt(&csi2->format_mbus,
> > +				      IMX_MEDIA_DEF_PIX_WIDTH,
> > +				      IMX_MEDIA_DEF_PIX_HEIGHT, 0,
> > +				      V4L2_FIELD_NONE, NULL);
> > +}
> > +
> > +static const struct v4l2_subdev_internal_ops csi2_internal_ops = {
> > +	.init_state = imx_media_init_state,
> > +	.registered = csi2_registered,

Also, these two operations should be handled by the new driver. The imx6
media framework has a set of helper functions to handle initialization
of formats, with data structures shared between drivers. It's fine
duplicating some of the code, with a copy of the required data in the
new driver. It will be a good opportunity to both avoid historical
mistake with the new code (for instance, imx_media_init_state() should
really not be implemented as a wrapper around get subdev get_fmt
operation), and to remove code from the shared helpers that is
CSI-2-specific, if any.

> > +};
> > +
> > +static const struct dw_mipi_csi2_config imx6_config = {
> > +	.module = THIS_MODULE,
> > +	.name = "imx6-mipi-csi2",
> > +	.internal_ops = &csi2_internal_ops,
> > +	.grp_id = IMX_MEDIA_GRP_ID_CSI2,
> > +};
> > +
> >  static int csi2_probe(struct platform_device *pdev)
> >  {
> >  	struct imx6_csi2 *csi2;
> > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > index b5911a37972be868e10c115ada1910fa04b2765c..ff06a739b957b305625d145c04c06999d0c13cae 100644
> > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > @@ -15,11 +15,11 @@
> >  #include <linux/platform_device.h>
> >  #include <media/dw-mipi-csi2.h>
> >  #include <media/v4l2-common.h>
> > +#include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-mc.h>
> >  #include <media/v4l2-subdev.h>
> > -#include "imx-media.h"
> >  
> >  /*
> >   * The default maximum bit-rate per lane in Mbps, if the
> > @@ -44,8 +44,6 @@ struct dw_csi2_regs {
> >  	u32	phy_tst_ctrl1;
> >  };
> >  
> > -#define DEVICE_NAME "imx6-mipi-csi2"
> > -
> >  /* Help check wrong access unexisted register at difference IP version */
> >  #define DW_REG_EXIST		0x80000000
> >  #define DW_REG(x)		(DW_REG_EXIST | (x))
> > @@ -544,17 +542,6 @@ static int csi2_set_fmt(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > -static int csi2_registered(struct v4l2_subdev *sd)
> > -{
> > -	struct dw_mipi_csi2_dev *csi2 = sd_to_dev(sd);
> > -
> > -	/* set a default mbus format  */
> > -	return imx_media_init_mbus_fmt(&csi2->format_mbus,
> > -				      IMX_MEDIA_DEF_PIX_WIDTH,
> > -				      IMX_MEDIA_DEF_PIX_HEIGHT, 0,
> > -				      V4L2_FIELD_NONE, NULL);
> > -}
> > -
> >  /* --------------- CORE OPS --------------- */
> >  
> >  static int csi2_log_status(struct v4l2_subdev *sd)
> > @@ -605,11 +592,6 @@ static const struct v4l2_subdev_ops csi2_subdev_ops = {
> >  	.pad = &csi2_pad_ops,
> >  };
> >  
> > -static const struct v4l2_subdev_internal_ops csi2_internal_ops = {
> > -	.init_state = imx_media_init_state,
> > -	.registered = csi2_registered,
> > -};
> > -
> >  static int csi2_notify_bound(struct v4l2_async_notifier *notifier,
> >  			     struct v4l2_subdev *sd,
> >  			     struct v4l2_async_connection *asd)
> > @@ -709,14 +691,15 @@ int dw_mipi_csi2_init(struct platform_device *pdev, struct dw_mipi_csi2_dev *csi
> >  	csi2->regs = &dw_csi2_v0;
> >  	v4l2_subdev_init(&csi2->sd, &csi2_subdev_ops);
> >  	v4l2_set_subdevdata(&csi2->sd, &pdev->dev);
> > -	csi2->sd.internal_ops = &csi2_internal_ops;
> >  	csi2->sd.entity.ops = &csi2_entity_ops;
> >  	csi2->sd.dev = &pdev->dev;
> > -	csi2->sd.owner = THIS_MODULE;
> >  	csi2->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > -	strscpy(csi2->sd.name, DEVICE_NAME, sizeof(csi2->sd.name));
> >  	csi2->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> > -	csi2->sd.grp_id = IMX_MEDIA_GRP_ID_CSI2;
> > +
> > +	csi2->sd.owner = config->module;
> > +	strscpy(csi2->sd.name, config->name, sizeof(csi2->sd.name));
> > +	csi2->sd.internal_ops = config->internal_ops;
> > +	csi2->sd.grp_id = config->grp_id;
> >  
> >  	for (i = 0; i < CSI2_NUM_PADS; i++) {
> >  		csi2->pad[i].flags = (i == CSI2_SINK_PAD) ?
> > diff --git a/include/media/dw-mipi-csi2.h b/include/media/dw-mipi-csi2.h
> > index 4e656f09c81db9dc9f232ed061f70803a5478c85..dfd25e23ea93fd53b064471b6a9557ef9c070fe7 100644
> > --- a/include/media/dw-mipi-csi2.h
> > +++ b/include/media/dw-mipi-csi2.h
> > @@ -10,6 +10,10 @@
> >  #include <media/v4l2-subdev.h>
> >  
> >  struct dw_mipi_csi2_config {
> > +	struct module *module;
> > +	const char *name;
> > +	int grp_id;
> > +	const struct v4l2_subdev_internal_ops *internal_ops;
> >  };
> >  
> >  /*

-- 
Regards,

Laurent Pinchart