Re: [PATCH v2 1/9] media: imx: Store the type of hardware implementation

From: Laurent Pinchart
Date: Mon Feb 14 2022 - 14:06:10 EST

Hi Jacopo,

On Mon, Feb 14, 2022 at 07:50:35PM +0100, Jacopo Mondi wrote:
> On Fri, Feb 11, 2022 at 03:27:44PM +0100, Alexander Stein wrote:
> > From: Dorota Czaplejewicz <dorota.czaplejewicz@xxxxxxx>
> >
> > The driver covers i.MX5/6, as well as i.MX7/8 hardware.
> > Those implementations differ, e.g. in the sizes of buffers they accept.
> >
> > Some functionality should be abstracted, and storing type achieves that.
> >
> > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@xxxxxxx>
> > Signed-off-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> > ---
> > Changes in v2:
> > * Switch back to using enum
> >
> > drivers/staging/media/imx/imx-ic-prpencvf.c | 3 ++-
> > drivers/staging/media/imx/imx-media-capture.c | 5 ++++-
> > drivers/staging/media/imx/imx-media-csi.c | 3 ++-
> > drivers/staging/media/imx/imx-media.h | 8 +++++++-
> > drivers/staging/media/imx/imx7-media-csi.c | 3 ++-
> > 5 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> > index 9b81cfbcd777..671bb9a681aa 100644
> > --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> > +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> > @@ -1266,7 +1266,8 @@ static int prp_registered(struct v4l2_subdev *sd)
> >
> > priv->vdev = imx_media_capture_device_init(ic_priv->ipu_dev,
> > &ic_priv->sd,
> > - PRPENCVF_SRC_PAD, true);
> > + PRPENCVF_SRC_PAD, true,
> > + DEVICE_TYPE_IMX56);
> > if (IS_ERR(priv->vdev))
> > return PTR_ERR(priv->vdev);
> >
> > diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> > index 93ba09236010..65dc95a48ecc 100644
> > --- a/drivers/staging/media/imx/imx-media-capture.c
> > +++ b/drivers/staging/media/imx/imx-media-capture.c
> > @@ -34,6 +34,7 @@ struct capture_priv {
> >
> > struct imx_media_video_dev vdev; /* Video device */
> > struct media_pad vdev_pad; /* Video device pad */
> > + enum imx_media_device_type type; /* Type of hardware implementation */
> >
> > struct v4l2_subdev *src_sd; /* Source subdev */
> > int src_sd_pad; /* Source subdev pad */
> > @@ -957,7 +958,8 @@ EXPORT_SYMBOL_GPL(imx_media_capture_device_unregister);
> >
> > struct imx_media_video_dev *
> > imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd,
> > - int pad, bool legacy_api)
> > + int pad, bool legacy_api,
> > + enum imx_media_device_type type)
> > {
> > struct capture_priv *priv;
> > struct video_device *vfd;
> > @@ -972,6 +974,7 @@ imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd,
> > priv->src_sd_pad = pad;
> > priv->dev = dev;
> > priv->legacy_api = legacy_api;
> > + priv->type = type;
> >
> > mutex_init(&priv->mutex);
> > INIT_LIST_HEAD(&priv->ready_q);
> > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> > index bd7f156f2d52..d5557bb4913d 100644
> > --- a/drivers/staging/media/imx/imx-media-csi.c
> > +++ b/drivers/staging/media/imx/imx-media-csi.c
> > @@ -1803,7 +1803,8 @@ static int csi_registered(struct v4l2_subdev *sd)
> > }
> >
> > priv->vdev = imx_media_capture_device_init(priv->sd.dev, &priv->sd,
> > - CSI_SRC_PAD_IDMAC, true);
> > + CSI_SRC_PAD_IDMAC, true,
> > + DEVICE_TYPE_IMX56);
> > if (IS_ERR(priv->vdev)) {
> > ret = PTR_ERR(priv->vdev);
> > goto free_fim;
> > diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> > index f263fc3adbb9..e4c22b3ccd57 100644
> > --- a/drivers/staging/media/imx/imx-media.h
> > +++ b/drivers/staging/media/imx/imx-media.h
> > @@ -96,6 +96,11 @@ enum imx_pixfmt_sel {
> > PIXFMT_SEL_ANY = PIXFMT_SEL_YUV | PIXFMT_SEL_RGB | PIXFMT_SEL_BAYER,
> > };
> >
> > +enum imx_media_device_type {
> > + DEVICE_TYPE_IMX56,
> > + DEVICE_TYPE_IMX78,
> > +};
> > +
>
> Isn't this too coarse as a distinction ?
>
> I tried adding per-soc identifiers here:
> https://lore.kernel.org/linux-media/20220214184318.409208-5-jacopo@xxxxxxxxxx/T/#u
>
> Maybe they can help ?

I'd really prefer not mixing the two. This enumeration is meant to
select which backend to use in helpers that should not be shared in the
first place. I've started decoupling the i.MX6 and i.MX7+ code, but it
will still take some time (the work in progress is available at [1] if
anyone is interested). In the meantime I'm OK with this patch, but any
need for additional device identification should be limited to the
imx7-media-csi driver or the i.MX6-specific code, not added to shared
helpers.

[1] https://gitlab.com/ideasonboard/nxp/linux/-/tree/pinchartl/csi-bridge/destage

> > struct imx_media_buffer {
> > struct vb2_v4l2_buffer vbuf; /* v4l buffer must be first */
> > struct list_head list;
> > @@ -282,7 +287,8 @@ int imx_media_ic_unregister(struct v4l2_subdev *sd);
> > /* imx-media-capture.c */
> > struct imx_media_video_dev *
> > imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd,
> > - int pad, bool legacy_api);
> > + int pad, bool legacy_api,
> > + enum imx_media_device_type type);
> > void imx_media_capture_device_remove(struct imx_media_video_dev *vdev);
> > int imx_media_capture_device_register(struct imx_media_video_dev *vdev,
> > u32 link_flags);
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > index 32311fc0e2a4..173dd014c2d6 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -1039,7 +1039,8 @@ static int imx7_csi_registered(struct v4l2_subdev *sd)
> > }
> >
> > csi->vdev = imx_media_capture_device_init(csi->sd.dev, &csi->sd,
> > - IMX7_CSI_PAD_SRC, false);
> > + IMX7_CSI_PAD_SRC, false,
> > + DEVICE_TYPE_IMX78);
> > if (IS_ERR(csi->vdev))
> > return PTR_ERR(csi->vdev);
> >

--
Regards,

Laurent Pinchart