[PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state

[PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state

[Laurent Pinchart]

Hello,

This small series moves the imx-mipi-csis driver to use the subdev
active state. Patches 1/5 to 3/5 are small preparatory cleanups, with
the main change in 4/5. Patch 5/5 is further cleanup that could have
been included in 4/5, but that should be easier to review as a separate
patch.

The series has been tested on the i.MX8MP with the ISI, and IMX296 and
IMX327 camera sensors.

Laurent Pinchart (5):
  media: imx-mipi-csis: Rename error labels with 'err_' prefix
  media: imx-mipi-csis: Don't take lock in runtime PM handlers
  media: imx-mipi-csis: Pass format explicitly to internal functions
  media: imx-mipi-csis: Use V4L2 subdev active state
  media: imx-mipi-csis: Implement .init_cfg() using .set_fmt()

 drivers/media/platform/nxp/imx-mipi-csis.c | 249 +++++++++------------
 1 file changed, 103 insertions(+), 146 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v1 1/5] media: imx-mipi-csis: Rename error labels with 'err_' prefix

[Laurent Pinchart]

It is customary to prefix error labels with 'err_' to make their purpose
clearer. Do so in the probe function.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 905072871ed2..d949b2de8e74 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -1496,20 +1496,20 @@ static int mipi_csis_probe(struct platform_device *pdev)
 			       dev_name(dev), csis);
 	if (ret) {
 		dev_err(dev, "Interrupt request failed\n");
-		goto disable_clock;
+		goto err_disable_clock;
 	}
 
 	/* Initialize and register the subdev. */
 	ret = mipi_csis_subdev_init(csis);
 	if (ret < 0)
-		goto disable_clock;
+		goto err_disable_clock;
 
 	platform_set_drvdata(pdev, &csis->sd);
 
 	ret = mipi_csis_async_register(csis);
 	if (ret < 0) {
 		dev_err(dev, "async register failed: %d\n", ret);
-		goto cleanup;
+		goto err_cleanup;
 	}
 
 	/* Initialize debugfs. */
@@ -1520,7 +1520,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
 	if (!pm_runtime_enabled(dev)) {
 		ret = mipi_csis_runtime_resume(dev);
 		if (ret < 0)
-			goto unregister_all;
+			goto err_unregister_all;
 	}
 
 	dev_info(dev, "lanes: %d, freq: %u\n",
@@ -1528,14 +1528,14 @@ static int mipi_csis_probe(struct platform_device *pdev)
 
 	return 0;
 
-unregister_all:
+err_unregister_all:
 	mipi_csis_debugfs_exit(csis);
-cleanup:
+err_cleanup:
 	media_entity_cleanup(&csis->sd.entity);
 	v4l2_async_nf_unregister(&csis->notifier);
 	v4l2_async_nf_cleanup(&csis->notifier);
 	v4l2_async_unregister_subdev(&csis->sd);
-disable_clock:
+err_disable_clock:
 	mipi_csis_clk_disable(csis);
 	fwnode_handle_put(csis->sd.fwnode);
 	mutex_destroy(&csis->lock);
-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 1/5] media: imx-mipi-csis: Rename error labels with 'err_' prefix

[Adam Ford]

On Thu, Jan 26, 2023 at 3:34 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> It is customary to prefix error labels with 'err_' to make their purpose
> clearer. Do so in the probe function.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---

I tested this series on the imx8mm, but I get an error stating the
capture format is invalid.

My media info looks like:

Media device information
------------------------
driver          imx7-csi
model           imx-media
serial
bus info        platform:32e20000.csi
hw revision     0x0
driver version  6.2.0

Device topology
- entity 1: csi (2 pads, 2 links)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
pad0: Sink
[fmt:UYVY8_1X16/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601
quantization:lim-range]
<- "csis-32e30000.mipi-csi":1 [ENABLED,IMMUTABLE]
pad1: Source
[fmt:UYVY8_1X16/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601
quantization:lim-range]
-> "csi capture":0 [ENABLED,IMMUTABLE]

- entity 4: csi capture (1 pad, 1 link)
            type Node subtype V4L flags 0
            device node name /dev/video0
pad0: Sink
<- "csi":1 [ENABLED,IMMUTABLE]

- entity 10: csis-32e30000.mipi-csi (2 pads, 2 links)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev1
pad0: Sink
[fmt:UYVY8_1X16/640x480 field:none]
<- "ov5640 1-003c":0 [ENABLED]
pad1: Source
[fmt:UYVY8_1X16/640x480 field:none]
-> "csi":0 [ENABLED,IMMUTABLE]

- entity 15: ov5640 1-003c (1 pad, 1 link)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev2
pad0: Source
[fmt:UYVY8_1X16/640x480@1/30 field:none colorspace:srgb xfer:srgb
ycbcr:601 quantization:full-range
crop.bounds:(0,0)/2624x1964
crop:(16,14)/2592x1944]
-> "csis-32e30000.mipi-csi":0 [ENABLED]

From what I can see, each node is configured for UYVY8_1X16/640x480

Yet, the following occurs:

gst-launch-1.0 v4l2src ! video/x-raw,format=UYVY,width=640,height=480 ! fakesink
Setting pipeline to PAUSED ...
Pipeline is live and does not[  335.986728] imx7-csi 32e20000.csi:
capture format not valid
 need PREROLL ...
Pipeline is PREROLLED ...
Setting pipeline to PLAYING ...
New clock: GstSystemClock
ERROR: from element /GstPipeline:pipeline0/GstV4l2Src:v4l2src0: Failed
to allocate required memory.
Additional debug info:
../git/sys/v4l2/gstv4l2src.c(659): gst_v4l2src_decide_allocation ():
/GstPipeline:pipeline0/GstV4l2Src:v4l2src0:
Buffer pool activation failed
Execution ended after 0:00:00.009848500
Setting pipeline to NULL ...
ERROR: from element /GstPipeline:pipeline0/GstV4l2Src:v4l2src0:
Internal data stream error.
Additional debug info:
../git/libs/gst/base/gstbasesrc.c(3127): gst_base_src_loop ():
/GstPipeline:pipeline0/GstV4l2Src:v4l2src0:
streaming stopped, reason not-negotiated (-4)
Freeing pipeline ...
root@beacon-imx8mm-kit:~#


I'm going to unroll this series to see if the mini can capture, and
I'll report back my findings.

adam


>  drivers/media/platform/nxp/imx-mipi-csis.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 905072871ed2..d949b2de8e74 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -1496,20 +1496,20 @@ static int mipi_csis_probe(struct platform_device *pdev)
>                                dev_name(dev), csis);
>         if (ret) {
>                 dev_err(dev, "Interrupt request failed\n");
> -               goto disable_clock;
> +               goto err_disable_clock;
>         }
>
>         /* Initialize and register the subdev. */
>         ret = mipi_csis_subdev_init(csis);
>         if (ret < 0)
> -               goto disable_clock;
> +               goto err_disable_clock;
>
>         platform_set_drvdata(pdev, &csis->sd);
>
>         ret = mipi_csis_async_register(csis);
>         if (ret < 0) {
>                 dev_err(dev, "async register failed: %d\n", ret);
> -               goto cleanup;
> +               goto err_cleanup;
>         }
>
>         /* Initialize debugfs. */
> @@ -1520,7 +1520,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
>         if (!pm_runtime_enabled(dev)) {
>                 ret = mipi_csis_runtime_resume(dev);
>                 if (ret < 0)
> -                       goto unregister_all;
> +                       goto err_unregister_all;
>         }
>
>         dev_info(dev, "lanes: %d, freq: %u\n",
> @@ -1528,14 +1528,14 @@ static int mipi_csis_probe(struct platform_device *pdev)
>
>         return 0;
>
> -unregister_all:
> +err_unregister_all:
>         mipi_csis_debugfs_exit(csis);
> -cleanup:
> +err_cleanup:
>         media_entity_cleanup(&csis->sd.entity);
>         v4l2_async_nf_unregister(&csis->notifier);
>         v4l2_async_nf_cleanup(&csis->notifier);
>         v4l2_async_unregister_subdev(&csis->sd);
> -disable_clock:
> +err_disable_clock:
>         mipi_csis_clk_disable(csis);
>         fwnode_handle_put(csis->sd.fwnode);
>         mutex_destroy(&csis->lock);
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v1 1/5] media: imx-mipi-csis: Rename error labels with 'err_' prefix

[Adam Ford]

On Thu, Jan 26, 2023 at 9:34 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Thu, Jan 26, 2023 at 3:34 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > It is customary to prefix error labels with 'err_' to make their purpose
> > clearer. Do so in the probe function.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
>
> I tested this series on the imx8mm, but I get an error stating the
> capture format is invalid.
>
> My media info looks like:
>
> Media device information
> ------------------------
> driver          imx7-csi
> model           imx-media
> serial
> bus info        platform:32e20000.csi
> hw revision     0x0
> driver version  6.2.0
>
> Device topology
> - entity 1: csi (2 pads, 2 links)
>             type V4L2 subdev subtype Unknown flags 0
>             device node name /dev/v4l-subdev0
> pad0: Sink
> [fmt:UYVY8_1X16/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601
> quantization:lim-range]
> <- "csis-32e30000.mipi-csi":1 [ENABLED,IMMUTABLE]
> pad1: Source
> [fmt:UYVY8_1X16/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601
> quantization:lim-range]
> -> "csi capture":0 [ENABLED,IMMUTABLE]
>
> - entity 4: csi capture (1 pad, 1 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video0
> pad0: Sink
> <- "csi":1 [ENABLED,IMMUTABLE]
>
> - entity 10: csis-32e30000.mipi-csi (2 pads, 2 links)
>              type V4L2 subdev subtype Unknown flags 0
>              device node name /dev/v4l-subdev1
> pad0: Sink
> [fmt:UYVY8_1X16/640x480 field:none]
> <- "ov5640 1-003c":0 [ENABLED]
> pad1: Source
> [fmt:UYVY8_1X16/640x480 field:none]
> -> "csi":0 [ENABLED,IMMUTABLE]
>
> - entity 15: ov5640 1-003c (1 pad, 1 link)
>              type V4L2 subdev subtype Sensor flags 0
>              device node name /dev/v4l-subdev2
> pad0: Source
> [fmt:UYVY8_1X16/640x480@1/30 field:none colorspace:srgb xfer:srgb
> ycbcr:601 quantization:full-range
> crop.bounds:(0,0)/2624x1964
> crop:(16,14)/2592x1944]
> -> "csis-32e30000.mipi-csi":0 [ENABLED]
>
> From what I can see, each node is configured for UYVY8_1X16/640x480
>
> Yet, the following occurs:
>
> gst-launch-1.0 v4l2src ! video/x-raw,format=UYVY,width=640,height=480 ! fakesink
> Setting pipeline to PAUSED ...
> Pipeline is live and does not[  335.986728] imx7-csi 32e20000.csi:
> capture format not valid
>  need PREROLL ...
> Pipeline is PREROLLED ...
> Setting pipeline to PLAYING ...
> New clock: GstSystemClock
> ERROR: from element /GstPipeline:pipeline0/GstV4l2Src:v4l2src0: Failed
> to allocate required memory.
> Additional debug info:
> ../git/sys/v4l2/gstv4l2src.c(659): gst_v4l2src_decide_allocation ():
> /GstPipeline:pipeline0/GstV4l2Src:v4l2src0:
> Buffer pool activation failed
> Execution ended after 0:00:00.009848500
> Setting pipeline to NULL ...
> ERROR: from element /GstPipeline:pipeline0/GstV4l2Src:v4l2src0:
> Internal data stream error.
> Additional debug info:
> ../git/libs/gst/base/gstbasesrc.c(3127): gst_base_src_loop ():
> /GstPipeline:pipeline0/GstV4l2Src:v4l2src0:
> streaming stopped, reason not-negotiated (-4)
> Freeing pipeline ...
> root@beacon-imx8mm-kit:~#
>
>
> I'm going to unroll this series to see if the mini can capture, and
> I'll report back my findings.

I unrolled both the imx7 capture stuff and the CSIS stuff, but I still
get the same error about the capture format not valid.  I rolled back
to 6.2-rc5 and it captures, but the colors seem off. I need to get
some sleep, and I'll be traveling Friday-Sunday, but I'll try to git
bisect to see where stuff stopped working on Mini when I get back.

adam

>
> adam
>
>
> >  drivers/media/platform/nxp/imx-mipi-csis.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > index 905072871ed2..d949b2de8e74 100644
> > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > @@ -1496,20 +1496,20 @@ static int mipi_csis_probe(struct platform_device *pdev)
> >                                dev_name(dev), csis);
> >         if (ret) {
> >                 dev_err(dev, "Interrupt request failed\n");
> > -               goto disable_clock;
> > +               goto err_disable_clock;
> >         }
> >
> >         /* Initialize and register the subdev. */
> >         ret = mipi_csis_subdev_init(csis);
> >         if (ret < 0)
> > -               goto disable_clock;
> > +               goto err_disable_clock;
> >
> >         platform_set_drvdata(pdev, &csis->sd);
> >
> >         ret = mipi_csis_async_register(csis);
> >         if (ret < 0) {
> >                 dev_err(dev, "async register failed: %d\n", ret);
> > -               goto cleanup;
> > +               goto err_cleanup;
> >         }
> >
> >         /* Initialize debugfs. */
> > @@ -1520,7 +1520,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
> >         if (!pm_runtime_enabled(dev)) {
> >                 ret = mipi_csis_runtime_resume(dev);
> >                 if (ret < 0)
> > -                       goto unregister_all;
> > +                       goto err_unregister_all;
> >         }
> >
> >         dev_info(dev, "lanes: %d, freq: %u\n",
> > @@ -1528,14 +1528,14 @@ static int mipi_csis_probe(struct platform_device *pdev)
> >
> >         return 0;
> >
> > -unregister_all:
> > +err_unregister_all:
> >         mipi_csis_debugfs_exit(csis);
> > -cleanup:
> > +err_cleanup:
> >         media_entity_cleanup(&csis->sd.entity);
> >         v4l2_async_nf_unregister(&csis->notifier);
> >         v4l2_async_nf_cleanup(&csis->notifier);
> >         v4l2_async_unregister_subdev(&csis->sd);
> > -disable_clock:
> > +err_disable_clock:
> >         mipi_csis_clk_disable(csis);
> >         fwnode_handle_put(csis->sd.fwnode);
> >         mutex_destroy(&csis->lock);
> > --
> > Regards,
> >
> > Laurent Pinchart
> >

[PATCH v1 2/5] media: imx-mipi-csis: Don't take lock in runtime PM handlers

[Laurent Pinchart]

The runtime PM handlers don't need manual locking as

- they are serialized by the runtime PM core
- they can't race with other functions taking the same lock, as they
  don't access any data protect by that lock

Drop the locking.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 28 +++++++++-------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index d949b2de8e74..4e1363ff5646 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -1348,40 +1348,34 @@ static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
-	int ret = 0;
-
-	mutex_lock(&csis->lock);
+	int ret;
 
 	ret = mipi_csis_phy_disable(csis);
 	if (ret)
-		goto unlock;
+		return -EAGAIN;
 
 	mipi_csis_clk_disable(csis);
 
-unlock:
-	mutex_unlock(&csis->lock);
-
-	return ret ? -EAGAIN : 0;
+	return 0;
 }
 
 static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
-	int ret = 0;
-
-	mutex_lock(&csis->lock);
+	int ret;
 
 	ret = mipi_csis_phy_enable(csis);
 	if (ret)
-		goto unlock;
+		return -EAGAIN;
 
-	mipi_csis_clk_enable(csis);
+	ret = mipi_csis_clk_enable(csis);
+	if (ret) {
+		mipi_csis_phy_disable(csis);
+		return ret;
+	}
 
-unlock:
-	mutex_unlock(&csis->lock);
-
-	return ret ? -EAGAIN : 0;
+	return 0;
 }
 
 static const struct dev_pm_ops mipi_csis_pm_ops = {
-- 
Regards,

Laurent Pinchart

[PATCH v1 3/5] media: imx-mipi-csis: Pass format explicitly to internal functions

[Laurent Pinchart]

To prepare for usage of the subdev active state that will replace the
csis_fmt and format_mbus fields stored in the mipi_csis_device
structure, pass the format explicitly to the functions called when
starting streaming to avoid accessing those two fields. Not functional
change intended.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 34 +++++++++++++---------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 4e1363ff5646..38ed88646632 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -560,9 +560,10 @@ static void mipi_csis_system_enable(struct mipi_csis_device *csis, int on)
 }
 
 /* Called with the csis.lock mutex held */
-static void __mipi_csis_set_format(struct mipi_csis_device *csis)
+static void __mipi_csis_set_format(struct mipi_csis_device *csis,
+				   const struct v4l2_mbus_framefmt *format,
+				   const struct csis_pix_format *csis_fmt)
 {
-	struct v4l2_mbus_framefmt *mf = &csis->format_mbus[CSIS_PAD_SINK];
 	u32 val;
 
 	/* Color format */
@@ -583,25 +584,26 @@ static void __mipi_csis_set_format(struct mipi_csis_device *csis)
 	 *
 	 * TODO: Verify which other formats require DUAL (or QUAD) modes.
 	 */
-	if (csis->csis_fmt->data_type == MIPI_CSI2_DATA_TYPE_YUV422_8)
+	if (csis_fmt->data_type == MIPI_CSI2_DATA_TYPE_YUV422_8)
 		val |= MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
 
-	val |= MIPI_CSIS_ISPCFG_FMT(csis->csis_fmt->data_type);
+	val |= MIPI_CSIS_ISPCFG_FMT(csis_fmt->data_type);
 	mipi_csis_write(csis, MIPI_CSIS_ISP_CONFIG_CH(0), val);
 
 	/* Pixel resolution */
-	val = mf->width | (mf->height << 16);
+	val = format->width | (format->height << 16);
 	mipi_csis_write(csis, MIPI_CSIS_ISP_RESOL_CH(0), val);
 }
 
-static int mipi_csis_calculate_params(struct mipi_csis_device *csis)
+static int mipi_csis_calculate_params(struct mipi_csis_device *csis,
+				      const struct csis_pix_format *csis_fmt)
 {
 	s64 link_freq;
 	u32 lane_rate;
 
 	/* Calculate the line rate from the pixel rate. */
 	link_freq = v4l2_get_link_freq(csis->src_sd->ctrl_handler,
-				       csis->csis_fmt->width,
+				       csis_fmt->width,
 				       csis->bus.num_data_lanes * 2);
 	if (link_freq < 0) {
 		dev_err(csis->dev, "Unable to obtain link frequency: %d\n",
@@ -643,7 +645,9 @@ static int mipi_csis_calculate_params(struct mipi_csis_device *csis)
 	return 0;
 }
 
-static void mipi_csis_set_params(struct mipi_csis_device *csis)
+static void mipi_csis_set_params(struct mipi_csis_device *csis,
+				 const struct v4l2_mbus_framefmt *format,
+				 const struct csis_pix_format *csis_fmt)
 {
 	int lanes = csis->bus.num_data_lanes;
 	u32 val;
@@ -655,7 +659,7 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis)
 		val |= MIPI_CSIS_CMN_CTRL_INTER_MODE;
 	mipi_csis_write(csis, MIPI_CSIS_CMN_CTRL, val);
 
-	__mipi_csis_set_format(csis);
+	__mipi_csis_set_format(csis, format, csis_fmt);
 
 	mipi_csis_write(csis, MIPI_CSIS_DPHY_CMN_CTRL,
 			MIPI_CSIS_DPHY_CMN_CTRL_HSSETTLE(csis->hs_settle) |
@@ -728,10 +732,12 @@ static int mipi_csis_clk_get(struct mipi_csis_device *csis)
 	return ret;
 }
 
-static void mipi_csis_start_stream(struct mipi_csis_device *csis)
+static void mipi_csis_start_stream(struct mipi_csis_device *csis,
+				   const struct v4l2_mbus_framefmt *format,
+				   const struct csis_pix_format *csis_fmt)
 {
 	mipi_csis_sw_reset(csis);
-	mipi_csis_set_params(csis);
+	mipi_csis_set_params(csis, format, csis_fmt);
 	mipi_csis_system_enable(csis, true);
 	mipi_csis_enable_interrupts(csis, true);
 }
@@ -935,6 +941,8 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
 static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
+	const struct v4l2_mbus_framefmt *format = &csis->format_mbus[CSIS_PAD_SINK];
+	const struct csis_pix_format *csis_fmt = csis->csis_fmt;
 	int ret;
 
 	if (!enable) {
@@ -953,7 +961,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 		return 0;
 	}
 
-	ret = mipi_csis_calculate_params(csis);
+	ret = mipi_csis_calculate_params(csis, csis_fmt);
 	if (ret < 0)
 		return ret;
 
@@ -965,7 +973,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 
 	mutex_lock(&csis->lock);
 
-	mipi_csis_start_stream(csis);
+	mipi_csis_start_stream(csis, format, csis_fmt);
 	ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1);
 	if (ret < 0)
 		goto error;
-- 
Regards,

Laurent Pinchart

[PATCH v1 4/5] media: imx-mipi-csis: Use V4L2 subdev active state

[Laurent Pinchart]

Use the V4L2 subdev active state API to store the active format. This
simplifies the driver not only by dropping the mipi_csis_device csis_fmt
and format_mbus fields, but it also allows dropping the device lock,
replaced with the state lock.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 135 +++++++--------------
 1 file changed, 47 insertions(+), 88 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 38ed88646632..9e424cb1c4b1 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -327,10 +327,6 @@ struct mipi_csis_device {
 	u32 hs_settle;
 	u32 clk_settle;
 
-	struct mutex lock;	/* Protect csis_fmt and format_mbus */
-	const struct csis_pix_format *csis_fmt;
-	struct v4l2_mbus_framefmt format_mbus[CSIS_PADS_NUM];
-
 	spinlock_t slock;	/* Protect events */
 	struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
 	struct dentry *debugfs_root;
@@ -559,7 +555,6 @@ static void mipi_csis_system_enable(struct mipi_csis_device *csis, int on)
 	mipi_csis_write(csis, MIPI_CSIS_DPHY_CMN_CTRL, val);
 }
 
-/* Called with the csis.lock mutex held */
 static void __mipi_csis_set_format(struct mipi_csis_device *csis,
 				   const struct v4l2_mbus_framefmt *format,
 				   const struct csis_pix_format *csis_fmt)
@@ -941,79 +936,67 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
 static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
-	const struct v4l2_mbus_framefmt *format = &csis->format_mbus[CSIS_PAD_SINK];
-	const struct csis_pix_format *csis_fmt = csis->csis_fmt;
+	const struct v4l2_mbus_framefmt *format;
+	const struct csis_pix_format *csis_fmt;
+	struct v4l2_subdev_state *state;
 	int ret;
 
 	if (!enable) {
-		mutex_lock(&csis->lock);
-
 		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
 
 		mipi_csis_stop_stream(csis);
 		if (csis->debug.enable)
 			mipi_csis_log_counters(csis, true);
 
-		mutex_unlock(&csis->lock);
-
 		pm_runtime_put(csis->dev);
 
 		return 0;
 	}
 
+	state = v4l2_subdev_lock_and_get_active_state(sd);
+
+	format = v4l2_subdev_get_pad_format(sd, state, CSIS_PAD_SINK);
+	csis_fmt = find_csis_format(format->code);
+
 	ret = mipi_csis_calculate_params(csis, csis_fmt);
 	if (ret < 0)
-		return ret;
+		goto err_unlock;
 
 	mipi_csis_clear_counters(csis);
 
 	ret = pm_runtime_resume_and_get(csis->dev);
 	if (ret < 0)
-		return ret;
-
-	mutex_lock(&csis->lock);
+		goto err_unlock;
 
 	mipi_csis_start_stream(csis, format, csis_fmt);
+
 	ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1);
 	if (ret < 0)
-		goto error;
+		goto err_stop;
 
 	mipi_csis_log_counters(csis, true);
 
-	mutex_unlock(&csis->lock);
+	v4l2_subdev_unlock_state(state);
 
 	return 0;
 
-error:
+err_stop:
 	mipi_csis_stop_stream(csis);
-	mutex_unlock(&csis->lock);
 	pm_runtime_put(csis->dev);
+err_unlock:
+	v4l2_subdev_unlock_state(state);
 
 	return ret;
 }
 
-static struct v4l2_mbus_framefmt *
-mipi_csis_get_format(struct mipi_csis_device *csis,
-		     struct v4l2_subdev_state *sd_state,
-		     enum v4l2_subdev_format_whence which,
-		     unsigned int pad)
-{
-	if (which == V4L2_SUBDEV_FORMAT_TRY)
-		return v4l2_subdev_get_try_format(&csis->sd, sd_state, pad);
-
-	return &csis->format_mbus[pad];
-}
-
 static int mipi_csis_init_cfg(struct v4l2_subdev *sd,
 			      struct v4l2_subdev_state *sd_state)
 {
-	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
 	struct v4l2_mbus_framefmt *fmt_sink;
 	struct v4l2_mbus_framefmt *fmt_source;
-	enum v4l2_subdev_format_whence which;
 
-	which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
-	fmt_sink = mipi_csis_get_format(csis, sd_state, which, CSIS_PAD_SINK);
+	fmt_sink = v4l2_subdev_get_pad_format(sd, sd_state, CSIS_PAD_SINK);
+	fmt_source = v4l2_subdev_get_pad_format(sd, sd_state, CSIS_PAD_SOURCE);
 
 	fmt_sink->code = MEDIA_BUS_FMT_UYVY8_1X16;
 	fmt_sink->width = MIPI_CSIS_DEF_PIX_WIDTH;
@@ -1027,36 +1010,15 @@ static int mipi_csis_init_cfg(struct v4l2_subdev *sd,
 		V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink->colorspace,
 					      fmt_sink->ycbcr_enc);
 
-	fmt_source = mipi_csis_get_format(csis, sd_state, which,
-					  CSIS_PAD_SOURCE);
 	*fmt_source = *fmt_sink;
 
 	return 0;
 }
 
-static int mipi_csis_get_fmt(struct v4l2_subdev *sd,
-			     struct v4l2_subdev_state *sd_state,
-			     struct v4l2_subdev_format *sdformat)
-{
-	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
-	struct v4l2_mbus_framefmt *fmt;
-
-	fmt = mipi_csis_get_format(csis, sd_state, sdformat->which,
-				   sdformat->pad);
-
-	mutex_lock(&csis->lock);
-	sdformat->format = *fmt;
-	mutex_unlock(&csis->lock);
-
-	return 0;
-}
-
 static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
 				    struct v4l2_subdev_state *sd_state,
 				    struct v4l2_subdev_mbus_code_enum *code)
 {
-	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
-
 	/*
 	 * The CSIS can't transcode in any way, the source format is identical
 	 * to the sink format.
@@ -1067,8 +1029,7 @@ static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
 		if (code->index > 0)
 			return -EINVAL;
 
-		fmt = mipi_csis_get_format(csis, sd_state, code->which,
-					   code->pad);
+		fmt = v4l2_subdev_get_pad_format(sd, sd_state, code->pad);
 		code->code = fmt->code;
 		return 0;
 	}
@@ -1088,7 +1049,6 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
 			     struct v4l2_subdev_state *sd_state,
 			     struct v4l2_subdev_format *sdformat)
 {
-	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
 	struct csis_pix_format const *csis_fmt;
 	struct v4l2_mbus_framefmt *fmt;
 	unsigned int align;
@@ -1098,7 +1058,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
 	 * modified.
 	 */
 	if (sdformat->pad == CSIS_PAD_SOURCE)
-		return mipi_csis_get_fmt(sd, sd_state, sdformat);
+		return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
 
 	if (sdformat->pad != CSIS_PAD_SINK)
 		return -EINVAL;
@@ -1136,10 +1096,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
 			      &sdformat->format.height, 1,
 			      CSIS_MAX_PIX_HEIGHT, 0, 0);
 
-	fmt = mipi_csis_get_format(csis, sd_state, sdformat->which,
-				   sdformat->pad);
-
-	mutex_lock(&csis->lock);
+	fmt = v4l2_subdev_get_pad_format(sd, sd_state, sdformat->pad);
 
 	fmt->code = csis_fmt->code;
 	fmt->width = sdformat->format.width;
@@ -1152,44 +1109,40 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
 	sdformat->format = *fmt;
 
 	/* Propagate the format from sink to source. */
-	fmt = mipi_csis_get_format(csis, sd_state, sdformat->which,
-				   CSIS_PAD_SOURCE);
+	fmt = v4l2_subdev_get_pad_format(sd, sd_state, CSIS_PAD_SOURCE);
 	*fmt = sdformat->format;
 
 	/* The format on the source pad might change due to unpacking. */
 	fmt->code = csis_fmt->output;
 
-	/* Store the CSIS format descriptor for active formats. */
-	if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		csis->csis_fmt = csis_fmt;
-
-	mutex_unlock(&csis->lock);
-
 	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];
+	const struct csis_pix_format *csis_fmt;
+	const struct v4l2_mbus_framefmt *fmt;
+	struct v4l2_subdev_state *state;
 
 	if (pad != CSIS_PAD_SOURCE)
 		return -EINVAL;
 
+	state = v4l2_subdev_lock_and_get_active_state(sd);
+	fmt = v4l2_subdev_get_pad_format(sd, state, CSIS_PAD_SOURCE);
+	csis_fmt = find_csis_format(fmt->code);
+	v4l2_subdev_unlock_state(state);
+
 	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->pixelcode = csis_fmt->code;
 	entry->bus.csi2.vc = 0;
-	entry->bus.csi2.dt = csis->csis_fmt->data_type;
-
-	mutex_unlock(&csis->lock);
+	entry->bus.csi2.dt = csis_fmt->data_type;
 
 	return 0;
 }
@@ -1216,7 +1169,7 @@ static const struct v4l2_subdev_video_ops mipi_csis_video_ops = {
 static const struct v4l2_subdev_pad_ops mipi_csis_pad_ops = {
 	.init_cfg		= mipi_csis_init_cfg,
 	.enum_mbus_code		= mipi_csis_enum_mbus_code,
-	.get_fmt		= mipi_csis_get_fmt,
+	.get_fmt		= v4l2_subdev_get_fmt,
 	.set_fmt		= mipi_csis_set_fmt,
 	.get_frame_desc		= mipi_csis_get_frame_desc,
 };
@@ -1398,6 +1351,7 @@ static const struct dev_pm_ops mipi_csis_pm_ops = {
 static int mipi_csis_subdev_init(struct mipi_csis_device *csis)
 {
 	struct v4l2_subdev *sd = &csis->sd;
+	int ret;
 
 	v4l2_subdev_init(sd, &mipi_csis_subdev_ops);
 	sd->owner = THIS_MODULE;
@@ -1419,15 +1373,21 @@ static int mipi_csis_subdev_init(struct mipi_csis_device *csis)
 		return -ENOENT;
 	}
 
-	csis->csis_fmt = &mipi_csis_formats[0];
-	mipi_csis_init_cfg(sd, NULL);
-
 	csis->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK
 					 | MEDIA_PAD_FL_MUST_CONNECT;
 	csis->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE
 					   | MEDIA_PAD_FL_MUST_CONNECT;
-	return media_entity_pads_init(&sd->entity, CSIS_PADS_NUM,
-				      csis->pads);
+	ret = media_entity_pads_init(&sd->entity, CSIS_PADS_NUM, csis->pads);
+	if (ret)
+		return ret;
+
+	ret = v4l2_subdev_init_finalize(sd);
+	if (ret) {
+		media_entity_cleanup(&sd->entity);
+		return ret;
+	}
+
+	return 0;
 }
 
 static int mipi_csis_parse_dt(struct mipi_csis_device *csis)
@@ -1452,7 +1412,6 @@ static int mipi_csis_probe(struct platform_device *pdev)
 	if (!csis)
 		return -ENOMEM;
 
-	mutex_init(&csis->lock);
 	spin_lock_init(&csis->slock);
 
 	csis->dev = dev;
@@ -1533,6 +1492,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
 err_unregister_all:
 	mipi_csis_debugfs_exit(csis);
 err_cleanup:
+	v4l2_subdev_cleanup(&csis->sd);
 	media_entity_cleanup(&csis->sd.entity);
 	v4l2_async_nf_unregister(&csis->notifier);
 	v4l2_async_nf_cleanup(&csis->notifier);
@@ -1540,7 +1500,6 @@ static int mipi_csis_probe(struct platform_device *pdev)
 err_disable_clock:
 	mipi_csis_clk_disable(csis);
 	fwnode_handle_put(csis->sd.fwnode);
-	mutex_destroy(&csis->lock);
 
 	return ret;
 }
@@ -1558,9 +1517,9 @@ static int mipi_csis_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	mipi_csis_runtime_suspend(&pdev->dev);
 	mipi_csis_clk_disable(csis);
+	v4l2_subdev_cleanup(&csis->sd);
 	media_entity_cleanup(&csis->sd.entity);
 	fwnode_handle_put(csis->sd.fwnode);
-	mutex_destroy(&csis->lock);
 	pm_runtime_set_suspended(&pdev->dev);
 
 	return 0;
-- 
Regards,

Laurent Pinchart

[PATCH v1 5/5] media: imx-mipi-csis: Implement .init_cfg() using .set_fmt()

[Laurent Pinchart]

The .set_fmt() handler is responsible for adjusting the requested format
based on the device limitations. Implement .init_cfg() as a wrapper of
.set_fmt(), to ensure that the initial configuration always matches the
rules implemented in .set_fmt(), should they ever change.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 48 ++++++++++------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 9e424cb1c4b1..e99633565463 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -989,32 +989,6 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
-static int mipi_csis_init_cfg(struct v4l2_subdev *sd,
-			      struct v4l2_subdev_state *sd_state)
-{
-	struct v4l2_mbus_framefmt *fmt_sink;
-	struct v4l2_mbus_framefmt *fmt_source;
-
-	fmt_sink = v4l2_subdev_get_pad_format(sd, sd_state, CSIS_PAD_SINK);
-	fmt_source = v4l2_subdev_get_pad_format(sd, sd_state, CSIS_PAD_SOURCE);
-
-	fmt_sink->code = MEDIA_BUS_FMT_UYVY8_1X16;
-	fmt_sink->width = MIPI_CSIS_DEF_PIX_WIDTH;
-	fmt_sink->height = MIPI_CSIS_DEF_PIX_HEIGHT;
-	fmt_sink->field = V4L2_FIELD_NONE;
-
-	fmt_sink->colorspace = V4L2_COLORSPACE_SMPTE170M;
-	fmt_sink->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt_sink->colorspace);
-	fmt_sink->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt_sink->colorspace);
-	fmt_sink->quantization =
-		V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink->colorspace,
-					      fmt_sink->ycbcr_enc);
-
-	*fmt_source = *fmt_sink;
-
-	return 0;
-}
-
 static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
 				    struct v4l2_subdev_state *sd_state,
 				    struct v4l2_subdev_mbus_code_enum *code)
@@ -1101,6 +1075,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
 	fmt->code = csis_fmt->code;
 	fmt->width = sdformat->format.width;
 	fmt->height = sdformat->format.height;
+	fmt->field = V4L2_FIELD_NONE;
 	fmt->colorspace = sdformat->format.colorspace;
 	fmt->quantization = sdformat->format.quantization;
 	fmt->xfer_func = sdformat->format.xfer_func;
@@ -1147,6 +1122,27 @@ static int mipi_csis_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 	return 0;
 }
 
+static int mipi_csis_init_cfg(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_state *sd_state)
+{
+	struct v4l2_subdev_format fmt = {
+		.pad = CSIS_PAD_SINK,
+	};
+
+	fmt.format.code = mipi_csis_formats[0].code;
+	fmt.format.width = MIPI_CSIS_DEF_PIX_WIDTH;
+	fmt.format.height = MIPI_CSIS_DEF_PIX_HEIGHT;
+
+	fmt.format.colorspace = V4L2_COLORSPACE_SMPTE170M;
+	fmt.format.xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt.format.colorspace);
+	fmt.format.ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt.format.colorspace);
+	fmt.format.quantization =
+		V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt.format.colorspace,
+					      fmt.format.ycbcr_enc);
+
+	return mipi_csis_set_fmt(sd, sd_state, &fmt);
+}
+
 static int mipi_csis_log_status(struct v4l2_subdev *sd)
 {
 	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state

[Adam Ford]

On Thu, Jan 26, 2023 at 3:34 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> This small series moves the imx-mipi-csis driver to use the subdev
> active state. Patches 1/5 to 3/5 are small preparatory cleanups, with
> the main change in 4/5. Patch 5/5 is further cleanup that could have
> been included in 4/5, but that should be easier to review as a separate
> patch.
>
> The series has been tested on the i.MX8MP with the ISI, and IMX296 and
> IMX327 camera sensors.

I didn't notice any overall change in the CSIS capture on the imx8mn.
I can test the Mini if you want.

For the series:
Tested-by: Adam Ford <aford173@gmail.com> #imx8mn-beacon

adam
>
> Laurent Pinchart (5):
>   media: imx-mipi-csis: Rename error labels with 'err_' prefix
>   media: imx-mipi-csis: Don't take lock in runtime PM handlers
>   media: imx-mipi-csis: Pass format explicitly to internal functions
>   media: imx-mipi-csis: Use V4L2 subdev active state
>   media: imx-mipi-csis: Implement .init_cfg() using .set_fmt()
>
>  drivers/media/platform/nxp/imx-mipi-csis.c | 249 +++++++++------------
>  1 file changed, 103 insertions(+), 146 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state

[Laurent Pinchart]

Hi Adam,

On Thu, Jan 26, 2023 at 07:54:10PM -0600, Adam Ford wrote:
> On Thu, Jan 26, 2023 at 3:34 PM Laurent Pinchart wrote:
> >
> > Hello,
> >
> > This small series moves the imx-mipi-csis driver to use the subdev
> > active state. Patches 1/5 to 3/5 are small preparatory cleanups, with
> > the main change in 4/5. Patch 5/5 is further cleanup that could have
> > been included in 4/5, but that should be easier to review as a separate
> > patch.
> >
> > The series has been tested on the i.MX8MP with the ISI, and IMX296 and
> > IMX327 camera sensors.
> 
> I didn't notice any overall change in the CSIS capture on the imx8mn.
> I can test the Mini if you want.

That would be great. Would you be able to test it in conjunction with
the similar imx7-media-csi series I've just sent ([1]) ? I expect a
possible lockdep warning if this series is applied with the
corresponding change in imx7-media-csi, but together they should be
fine.

[1] https://lore.kernel.org/linux-media/20230127022715.27234-1-laurent.pinchart@ideasonboard.com

> For the series:
> Tested-by: Adam Ford <aford173@gmail.com> #imx8mn-beacon
> 
> > Laurent Pinchart (5):
> >   media: imx-mipi-csis: Rename error labels with 'err_' prefix
> >   media: imx-mipi-csis: Don't take lock in runtime PM handlers
> >   media: imx-mipi-csis: Pass format explicitly to internal functions
> >   media: imx-mipi-csis: Use V4L2 subdev active state
> >   media: imx-mipi-csis: Implement .init_cfg() using .set_fmt()
> >
> >  drivers/media/platform/nxp/imx-mipi-csis.c | 249 +++++++++------------
> >  1 file changed, 103 insertions(+), 146 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state

[Adam Ford]

On Thu, Jan 26, 2023 at 8:29 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Adam,
>
> On Thu, Jan 26, 2023 at 07:54:10PM -0600, Adam Ford wrote:
> > On Thu, Jan 26, 2023 at 3:34 PM Laurent Pinchart wrote:
> > >
> > > Hello,
> > >
> > > This small series moves the imx-mipi-csis driver to use the subdev
> > > active state. Patches 1/5 to 3/5 are small preparatory cleanups, with
> > > the main change in 4/5. Patch 5/5 is further cleanup that could have
> > > been included in 4/5, but that should be easier to review as a separate
> > > patch.
> > >
> > > The series has been tested on the i.MX8MP with the ISI, and IMX296 and
> > > IMX327 camera sensors.
> >
> > I didn't notice any overall change in the CSIS capture on the imx8mn.
> > I can test the Mini if you want.
>
> That would be great. Would you be able to test it in conjunction with
> the similar imx7-media-csi series I've just sent ([1]) ? I expect a
> possible lockdep warning if this series is applied with the
> corresponding change in imx7-media-csi, but together they should be
> fine.

I am working on figuring out why my mini doesn't boot, but I've
already taken the CSIS series and the imx7-media-csi series and
applied them to my working branch.  I'll report my findings as soon as
I can get it booting.

adam
>
> [1] https://lore.kernel.org/linux-media/20230127022715.27234-1-laurent.pinchart@ideasonboard.com
>
> > For the series:
> > Tested-by: Adam Ford <aford173@gmail.com> #imx8mn-beacon
> >
> > > Laurent Pinchart (5):
> > >   media: imx-mipi-csis: Rename error labels with 'err_' prefix
> > >   media: imx-mipi-csis: Don't take lock in runtime PM handlers
> > >   media: imx-mipi-csis: Pass format explicitly to internal functions
> > >   media: imx-mipi-csis: Use V4L2 subdev active state
> > >   media: imx-mipi-csis: Implement .init_cfg() using .set_fmt()
> > >
> > >  drivers/media/platform/nxp/imx-mipi-csis.c | 249 +++++++++------------
> > >  1 file changed, 103 insertions(+), 146 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state

[Rui Miguel Silva]

Hey Laurent,
Thanks for all the cleanups and updates.

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Hello,
>
> This small series moves the imx-mipi-csis driver to use the subdev
> active state. Patches 1/5 to 3/5 are small preparatory cleanups, with
> the main change in 4/5. Patch 5/5 is further cleanup that could have
> been included in 4/5, but that should be easier to review as a separate
> patch.
>
> The series has been tested on the i.MX8MP with the ISI, and IMX296 and
> IMX327 camera sensors.
>
> Laurent Pinchart (5):
>   media: imx-mipi-csis: Rename error labels with 'err_' prefix
>   media: imx-mipi-csis: Don't take lock in runtime PM handlers
>   media: imx-mipi-csis: Pass format explicitly to internal functions
>   media: imx-mipi-csis: Use V4L2 subdev active state
>   media: imx-mipi-csis: Implement .init_cfg() using .set_fmt()

LGTM, for the all series.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>

Cheers,
    Rui

>
>  drivers/media/platform/nxp/imx-mipi-csis.c | 249 +++++++++------------
>  1 file changed, 103 insertions(+), 146 deletions(-)
>
> -- 
> Regards,
>
> Laurent Pinchart