[PATCH v2 0/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev state

[PATCH v2 0/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev state

[Martin Kepplinger]

hi Laurent and all interested,

this is driver simplifications and cleanups for the imx8mq mipi csi receiver.
Remaining TODO is:
	* moving the driver into media/platform/nxp
	* getting rid of the internal "state" variable
	* autosuspend at the end of probe()

thanks for your time when looking at this,

                               martin


revision history
----------------

v2: thank you, Laurent:
 * properly unlock the subdev state
 * fix pm hunk in probe()
 * add patch to "inline hs_settle"

v1: initial patch:
https://lore.kernel.org/linux-media/41417ffce644975b3be0d52fb7ac584b3a7c3b1b.camel@puri.sm/T/#t


Martin Kepplinger (2):
  media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state
  media: imx: imx8mq-mipi-csi2: remove unneeded state variable and
    function

 drivers/staging/media/imx/imx8mq-mipi-csi2.c | 153 +++++++------------
 1 file changed, 59 insertions(+), 94 deletions(-)

-- 
2.30.2

[PATCH v2 1/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state

[Martin Kepplinger]

Simplify the driver by using the V4L2 subdev active state API to store
the active format.

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 drivers/staging/media/imx/imx8mq-mipi-csi2.c | 118 ++++++++-----------
 1 file changed, 46 insertions(+), 72 deletions(-)

diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
index c0d0bf770096..1aa8622a3bae 100644
--- a/drivers/staging/media/imx/imx8mq-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
@@ -119,9 +119,7 @@ struct csi_state {
 
 	struct v4l2_mbus_config_mipi_csi2 bus;
 
-	struct mutex lock; /* Protect csi2_fmt, format_mbus, state, hs_settle */
-	const struct csi2_pix_format *csi2_fmt;
-	struct v4l2_mbus_framefmt format_mbus[MIPI_CSI2_PADS_NUM];
+	struct mutex lock; /* Protect state and hs_settle */
 	u32 state;
 	u32 hs_settle;
 
@@ -322,16 +320,23 @@ static int imx8mq_mipi_csi_clk_get(struct csi_state *state)
 	return devm_clk_bulk_get(state->dev, CSI2_NUM_CLKS, state->clks);
 }
 
-static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state)
+static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
+					  struct v4l2_subdev_state *sd_state)
 {
 	s64 link_freq;
 	u32 lane_rate;
 	unsigned long esc_clk_rate;
 	u32 min_ths_settle, max_ths_settle, ths_settle_ns, esc_clk_period_ns;
+	const struct v4l2_mbus_framefmt *fmt;
+	const struct csi2_pix_format *csi2_fmt;
 
 	/* Calculate the line rate from the pixel rate. */
+
+	fmt = v4l2_subdev_get_pad_format(&state->sd, sd_state, MIPI_CSI2_PAD_SINK);
+	csi2_fmt = find_csi2_format(fmt->code);
+
 	link_freq = v4l2_get_link_freq(state->src_sd->ctrl_handler,
-				       state->csi2_fmt->width,
+				       csi2_fmt->width,
 				       state->bus.num_data_lanes * 2);
 	if (link_freq < 0) {
 		dev_err(state->dev, "Unable to obtain link frequency: %d\n",
@@ -380,7 +385,8 @@ static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state)
 	return 0;
 }
 
-static int imx8mq_mipi_csi_start_stream(struct csi_state *state)
+static int imx8mq_mipi_csi_start_stream(struct csi_state *state,
+					struct v4l2_subdev_state *sd_state)
 {
 	int ret;
 
@@ -389,7 +395,7 @@ static int imx8mq_mipi_csi_start_stream(struct csi_state *state)
 		return ret;
 
 	imx8mq_mipi_csi_set_params(state);
-	ret = imx8mq_mipi_csi_calc_hs_settle(state);
+	ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state);
 	if (ret)
 		return ret;
 
@@ -415,6 +421,7 @@ static struct csi_state *mipi_sd_to_csi2_state(struct v4l2_subdev *sdev)
 static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct csi_state *state = mipi_sd_to_csi2_state(sd);
+	struct v4l2_subdev_state *sd_state;
 	int ret = 0;
 
 	if (enable) {
@@ -431,7 +438,9 @@ static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int enable)
 			goto unlock;
 		}
 
-		ret = imx8mq_mipi_csi_start_stream(state);
+		sd_state = v4l2_subdev_lock_and_get_active_state(sd);
+
+		ret = imx8mq_mipi_csi_start_stream(state, sd_state);
 		if (ret < 0)
 			goto unlock;
 
@@ -440,6 +449,8 @@ static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int enable)
 			goto unlock;
 
 		state->state |= ST_STREAMING;
+
+		v4l2_subdev_unlock_state(sd_state);
 	} else {
 		v4l2_subdev_call(state->src_sd, video, s_stream, 0);
 		imx8mq_mipi_csi_stop_stream(state);
@@ -455,29 +466,14 @@ static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
-static struct v4l2_mbus_framefmt *
-imx8mq_mipi_csi_get_format(struct csi_state *state,
-			   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(&state->sd, sd_state, pad);
-
-	return &state->format_mbus[pad];
-}
-
 static int imx8mq_mipi_csi_init_cfg(struct v4l2_subdev *sd,
 				    struct v4l2_subdev_state *sd_state)
 {
-	struct csi_state *state = mipi_sd_to_csi2_state(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 = imx8mq_mipi_csi_get_format(state, sd_state, which,
-					      MIPI_CSI2_PAD_SINK);
+	fmt_sink = v4l2_subdev_get_pad_format(sd, sd_state, MIPI_CSI2_PAD_SINK);
+	fmt_source = v4l2_subdev_get_pad_format(sd, sd_state, MIPI_CSI2_PAD_SOURCE);
 
 	fmt_sink->code = MEDIA_BUS_FMT_SGBRG10_1X10;
 	fmt_sink->width = MIPI_CSI2_DEF_PIX_WIDTH;
@@ -491,38 +487,15 @@ static int imx8mq_mipi_csi_init_cfg(struct v4l2_subdev *sd,
 		V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink->colorspace,
 					      fmt_sink->ycbcr_enc);
 
-	fmt_source = imx8mq_mipi_csi_get_format(state, sd_state, which,
-						MIPI_CSI2_PAD_SOURCE);
 	*fmt_source = *fmt_sink;
 
 	return 0;
 }
 
-static int imx8mq_mipi_csi_get_fmt(struct v4l2_subdev *sd,
-				   struct v4l2_subdev_state *sd_state,
-				   struct v4l2_subdev_format *sdformat)
-{
-	struct csi_state *state = mipi_sd_to_csi2_state(sd);
-	struct v4l2_mbus_framefmt *fmt;
-
-	fmt = imx8mq_mipi_csi_get_format(state, sd_state, sdformat->which,
-					 sdformat->pad);
-
-	mutex_lock(&state->lock);
-
-	sdformat->format = *fmt;
-
-	mutex_unlock(&state->lock);
-
-	return 0;
-}
-
 static int imx8mq_mipi_csi_enum_mbus_code(struct v4l2_subdev *sd,
 					  struct v4l2_subdev_state *sd_state,
 					  struct v4l2_subdev_mbus_code_enum *code)
 {
-	struct csi_state *state = mipi_sd_to_csi2_state(sd);
-
 	/*
 	 * We can't transcode in any way, the source format is identical
 	 * to the sink format.
@@ -533,8 +506,7 @@ static int imx8mq_mipi_csi_enum_mbus_code(struct v4l2_subdev *sd,
 		if (code->index > 0)
 			return -EINVAL;
 
-		fmt = imx8mq_mipi_csi_get_format(state, sd_state, code->which,
-						 code->pad);
+		fmt = v4l2_subdev_get_pad_format(sd, sd_state, code->pad);
 		code->code = fmt->code;
 		return 0;
 	}
@@ -554,8 +526,7 @@ static int imx8mq_mipi_csi_set_fmt(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_state *sd_state,
 				   struct v4l2_subdev_format *sdformat)
 {
-	struct csi_state *state = mipi_sd_to_csi2_state(sd);
-	struct csi2_pix_format const *csi2_fmt;
+	const struct csi2_pix_format *csi2_fmt;
 	struct v4l2_mbus_framefmt *fmt;
 
 	/*
@@ -563,7 +534,7 @@ static int imx8mq_mipi_csi_set_fmt(struct v4l2_subdev *sd,
 	 * modified.
 	 */
 	if (sdformat->pad == MIPI_CSI2_PAD_SOURCE)
-		return imx8mq_mipi_csi_get_fmt(sd, sd_state, sdformat);
+		return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
 
 	if (sdformat->pad != MIPI_CSI2_PAD_SINK)
 		return -EINVAL;
@@ -572,10 +543,7 @@ static int imx8mq_mipi_csi_set_fmt(struct v4l2_subdev *sd,
 	if (!csi2_fmt)
 		csi2_fmt = &imx8mq_mipi_csi_formats[0];
 
-	fmt = imx8mq_mipi_csi_get_format(state, sd_state, sdformat->which,
-					 sdformat->pad);
-
-	mutex_lock(&state->lock);
+	fmt = v4l2_subdev_get_pad_format(sd, sd_state, sdformat->pad);
 
 	fmt->code = csi2_fmt->code;
 	fmt->width = sdformat->format.width;
@@ -584,16 +552,9 @@ static int imx8mq_mipi_csi_set_fmt(struct v4l2_subdev *sd,
 	sdformat->format = *fmt;
 
 	/* Propagate the format from sink to source. */
-	fmt = imx8mq_mipi_csi_get_format(state, sd_state, sdformat->which,
-					 MIPI_CSI2_PAD_SOURCE);
+	fmt = v4l2_subdev_get_pad_format(sd, sd_state, MIPI_CSI2_PAD_SOURCE);
 	*fmt = sdformat->format;
 
-	/* Store the CSI2 format descriptor for active formats. */
-	if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		state->csi2_fmt = csi2_fmt;
-
-	mutex_unlock(&state->lock);
-
 	return 0;
 }
 
@@ -604,7 +565,7 @@ static const struct v4l2_subdev_video_ops imx8mq_mipi_csi_video_ops = {
 static const struct v4l2_subdev_pad_ops imx8mq_mipi_csi_pad_ops = {
 	.init_cfg		= imx8mq_mipi_csi_init_cfg,
 	.enum_mbus_code		= imx8mq_mipi_csi_enum_mbus_code,
-	.get_fmt		= imx8mq_mipi_csi_get_fmt,
+	.get_fmt		= v4l2_subdev_get_fmt,
 	.set_fmt		= imx8mq_mipi_csi_set_fmt,
 };
 
@@ -732,6 +693,7 @@ static int imx8mq_mipi_csi_pm_resume(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct csi_state *state = mipi_sd_to_csi2_state(sd);
+	struct v4l2_subdev_state *sd_state;
 	int ret = 0;
 
 	mutex_lock(&state->lock);
@@ -741,7 +703,9 @@ static int imx8mq_mipi_csi_pm_resume(struct device *dev)
 		ret = imx8mq_mipi_csi_clk_enable(state);
 	}
 	if (state->state & ST_STREAMING) {
-		ret = imx8mq_mipi_csi_start_stream(state);
+		sd_state = v4l2_subdev_lock_and_get_active_state(sd);
+		ret = imx8mq_mipi_csi_start_stream(state, sd_state);
+		v4l2_subdev_unlock_state(sd_state);
 		if (ret)
 			goto unlock;
 	}
@@ -821,6 +785,7 @@ static const struct dev_pm_ops imx8mq_mipi_csi_pm_ops = {
 static int imx8mq_mipi_csi_subdev_init(struct csi_state *state)
 {
 	struct v4l2_subdev *sd = &state->sd;
+	int ret;
 
 	v4l2_subdev_init(sd, &imx8mq_mipi_csi_subdev_ops);
 	sd->owner = THIS_MODULE;
@@ -834,15 +799,22 @@ static int imx8mq_mipi_csi_subdev_init(struct csi_state *state)
 
 	sd->dev = state->dev;
 
-	state->csi2_fmt = &imx8mq_mipi_csi_formats[0];
-	imx8mq_mipi_csi_init_cfg(sd, NULL);
-
 	state->pads[MIPI_CSI2_PAD_SINK].flags = MEDIA_PAD_FL_SINK
 					 | MEDIA_PAD_FL_MUST_CONNECT;
 	state->pads[MIPI_CSI2_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE
 					   | MEDIA_PAD_FL_MUST_CONNECT;
-	return media_entity_pads_init(&sd->entity, MIPI_CSI2_PADS_NUM,
-				      state->pads);
+	ret = media_entity_pads_init(&sd->entity, MIPI_CSI2_PADS_NUM,
+				     state->pads);
+	if (ret)
+		return ret;
+
+	ret = v4l2_subdev_init_finalize(sd);
+	if (ret) {
+		media_entity_cleanup(&sd->entity);
+		return ret;
+	}
+
+	return 0;
 }
 
 static void imx8mq_mipi_csi_release_icc(struct platform_device *pdev)
@@ -968,6 +940,7 @@ static int imx8mq_mipi_csi_probe(struct platform_device *pdev)
 	imx8mq_mipi_csi_runtime_suspend(&pdev->dev);
 
 	media_entity_cleanup(&state->sd.entity);
+	v4l2_subdev_cleanup(&state->sd);
 	v4l2_async_nf_unregister(&state->notifier);
 	v4l2_async_nf_cleanup(&state->notifier);
 	v4l2_async_unregister_subdev(&state->sd);
@@ -991,6 +964,7 @@ static int imx8mq_mipi_csi_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	imx8mq_mipi_csi_runtime_suspend(&pdev->dev);
 	media_entity_cleanup(&state->sd.entity);
+	v4l2_subdev_cleanup(&state->sd);
 	mutex_destroy(&state->lock);
 	pm_runtime_set_suspended(&pdev->dev);
 	imx8mq_mipi_csi_release_icc(pdev);
-- 
2.30.2

[PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function

[Martin Kepplinger]

Clean up the driver a bit by inlining the imx8mq_mipi_csi_system_enable()
function to the callsites and removing the hs_settle variable from the
driver state.

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 drivers/staging/media/imx/imx8mq-mipi-csi2.c | 41 ++++++++------------
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
index 1aa8622a3bae..f10b59b8f1c0 100644
--- a/drivers/staging/media/imx/imx8mq-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
@@ -119,9 +119,8 @@ struct csi_state {
 
 	struct v4l2_mbus_config_mipi_csi2 bus;
 
-	struct mutex lock; /* Protect state and hs_settle */
+	struct mutex lock; /* Protect state */
 	u32 state;
-	u32 hs_settle;
 
 	struct regmap *phy_gpr;
 	u8 phy_gpr_reg;
@@ -264,23 +263,6 @@ static int imx8mq_mipi_csi_sw_reset(struct csi_state *state)
 	return 0;
 }
 
-static void imx8mq_mipi_csi_system_enable(struct csi_state *state, int on)
-{
-	if (!on) {
-		imx8mq_mipi_csi_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
-		return;
-	}
-
-	regmap_update_bits(state->phy_gpr,
-			   state->phy_gpr_reg,
-			   0x3fff,
-			   GPR_CSI2_1_RX_ENABLE |
-			   GPR_CSI2_1_VID_INTFC_ENB |
-			   GPR_CSI2_1_HSEL |
-			   GPR_CSI2_1_CONT_CLK_MODE |
-			   GPR_CSI2_1_S_PRG_RXHS_SETTLE(state->hs_settle));
-}
-
 static void imx8mq_mipi_csi_set_params(struct csi_state *state)
 {
 	int lanes = state->bus.num_data_lanes;
@@ -321,7 +303,8 @@ static int imx8mq_mipi_csi_clk_get(struct csi_state *state)
 }
 
 static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
-					  struct v4l2_subdev_state *sd_state)
+					  struct v4l2_subdev_state *sd_state,
+					  u32 *hs_settle)
 {
 	s64 link_freq;
 	u32 lane_rate;
@@ -377,10 +360,10 @@ static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
 	max_ths_settle = 140 + 10 * 1000000 / (lane_rate / 1000);
 	ths_settle_ns = (min_ths_settle + max_ths_settle) / 2;
 
-	state->hs_settle = ths_settle_ns / esc_clk_period_ns - 1;
+	*hs_settle = ths_settle_ns / esc_clk_period_ns - 1;
 
 	dev_dbg(state->dev, "lane rate %u Ths_settle %u hs_settle %u\n",
-		lane_rate, ths_settle_ns, state->hs_settle);
+		lane_rate, ths_settle_ns, *hs_settle);
 
 	return 0;
 }
@@ -389,24 +372,32 @@ static int imx8mq_mipi_csi_start_stream(struct csi_state *state,
 					struct v4l2_subdev_state *sd_state)
 {
 	int ret;
+	u32 hs_settle;
 
 	ret = imx8mq_mipi_csi_sw_reset(state);
 	if (ret)
 		return ret;
 
 	imx8mq_mipi_csi_set_params(state);
-	ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state);
+	ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state, &hs_settle);
 	if (ret)
 		return ret;
 
-	imx8mq_mipi_csi_system_enable(state, true);
+	regmap_update_bits(state->phy_gpr,
+			   state->phy_gpr_reg,
+			   0x3fff,
+			   GPR_CSI2_1_RX_ENABLE |
+			   GPR_CSI2_1_VID_INTFC_ENB |
+			   GPR_CSI2_1_HSEL |
+			   GPR_CSI2_1_CONT_CLK_MODE |
+			   GPR_CSI2_1_S_PRG_RXHS_SETTLE(hs_settle));
 
 	return 0;
 }
 
 static void imx8mq_mipi_csi_stop_stream(struct csi_state *state)
 {
-	imx8mq_mipi_csi_system_enable(state, false);
+	imx8mq_mipi_csi_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
 }
 
 /* -----------------------------------------------------------------------------
-- 
2.30.2

Re: [PATCH v2 1/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state

[Laurent Pinchart]

Hi Martin,

Thank you for the patch.

On Tue, Mar 07, 2023 at 04:00:46PM +0100, Martin Kepplinger wrote:
> Simplify the driver by using the V4L2 subdev active state API to store
> the active format.
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> ---
>  drivers/staging/media/imx/imx8mq-mipi-csi2.c | 118 ++++++++-----------
>  1 file changed, 46 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> index c0d0bf770096..1aa8622a3bae 100644
> --- a/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> +++ b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> @@ -119,9 +119,7 @@ struct csi_state {
>  
>  	struct v4l2_mbus_config_mipi_csi2 bus;
>  
> -	struct mutex lock; /* Protect csi2_fmt, format_mbus, state, hs_settle */
> -	const struct csi2_pix_format *csi2_fmt;
> -	struct v4l2_mbus_framefmt format_mbus[MIPI_CSI2_PADS_NUM];
> +	struct mutex lock; /* Protect state and hs_settle */
>  	u32 state;
>  	u32 hs_settle;
>  
> @@ -322,16 +320,23 @@ static int imx8mq_mipi_csi_clk_get(struct csi_state *state)
>  	return devm_clk_bulk_get(state->dev, CSI2_NUM_CLKS, state->clks);
>  }
>  
> -static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state)
> +static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
> +					  struct v4l2_subdev_state *sd_state)
>  {
>  	s64 link_freq;
>  	u32 lane_rate;
>  	unsigned long esc_clk_rate;
>  	u32 min_ths_settle, max_ths_settle, ths_settle_ns, esc_clk_period_ns;
> +	const struct v4l2_mbus_framefmt *fmt;
> +	const struct csi2_pix_format *csi2_fmt;
>  
>  	/* Calculate the line rate from the pixel rate. */
> +
> +	fmt = v4l2_subdev_get_pad_format(&state->sd, sd_state, MIPI_CSI2_PAD_SINK);
> +	csi2_fmt = find_csi2_format(fmt->code);
> +
>  	link_freq = v4l2_get_link_freq(state->src_sd->ctrl_handler,
> -				       state->csi2_fmt->width,
> +				       csi2_fmt->width,
>  				       state->bus.num_data_lanes * 2);
>  	if (link_freq < 0) {
>  		dev_err(state->dev, "Unable to obtain link frequency: %d\n",
> @@ -380,7 +385,8 @@ static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state)
>  	return 0;
>  }
>  
> -static int imx8mq_mipi_csi_start_stream(struct csi_state *state)
> +static int imx8mq_mipi_csi_start_stream(struct csi_state *state,
> +					struct v4l2_subdev_state *sd_state)
>  {
>  	int ret;
>  
> @@ -389,7 +395,7 @@ static int imx8mq_mipi_csi_start_stream(struct csi_state *state)
>  		return ret;
>  
>  	imx8mq_mipi_csi_set_params(state);
> -	ret = imx8mq_mipi_csi_calc_hs_settle(state);
> +	ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state);
>  	if (ret)
>  		return ret;
>  
> @@ -415,6 +421,7 @@ static struct csi_state *mipi_sd_to_csi2_state(struct v4l2_subdev *sdev)
>  static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct csi_state *state = mipi_sd_to_csi2_state(sd);
> +	struct v4l2_subdev_state *sd_state;
>  	int ret = 0;
>  
>  	if (enable) {
> @@ -431,7 +438,9 @@ static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int enable)
>  			goto unlock;
>  		}
>  
> -		ret = imx8mq_mipi_csi_start_stream(state);
> +		sd_state = v4l2_subdev_lock_and_get_active_state(sd);
> +
> +		ret = imx8mq_mipi_csi_start_stream(state, sd_state);
>  		if (ret < 0)
>  			goto unlock;

You're leaving the state locked here. I would write

		sd_state = v4l2_subdev_lock_and_get_active_state(sd);
		ret = imx8mq_mipi_csi_start_stream(state, sd_state);
		v4l2_subdev_unlock_state(sd_state);

		if (ret < 0)
			goto unlock;

and drop the v4l2_subdev_unlock_state() call below. Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
> @@ -440,6 +449,8 @@ static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int enable)
>  			goto unlock;
>  
>  		state->state |= ST_STREAMING;
> +
> +		v4l2_subdev_unlock_state(sd_state);
>  	} else {
>  		v4l2_subdev_call(state->src_sd, video, s_stream, 0);
>  		imx8mq_mipi_csi_stop_stream(state);
> @@ -455,29 +466,14 @@ static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int enable)
>  	return ret;
>  }
>  
> -static struct v4l2_mbus_framefmt *
> -imx8mq_mipi_csi_get_format(struct csi_state *state,
> -			   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(&state->sd, sd_state, pad);
> -
> -	return &state->format_mbus[pad];
> -}
> -
>  static int imx8mq_mipi_csi_init_cfg(struct v4l2_subdev *sd,
>  				    struct v4l2_subdev_state *sd_state)
>  {
> -	struct csi_state *state = mipi_sd_to_csi2_state(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 = imx8mq_mipi_csi_get_format(state, sd_state, which,
> -					      MIPI_CSI2_PAD_SINK);
> +	fmt_sink = v4l2_subdev_get_pad_format(sd, sd_state, MIPI_CSI2_PAD_SINK);
> +	fmt_source = v4l2_subdev_get_pad_format(sd, sd_state, MIPI_CSI2_PAD_SOURCE);
>  
>  	fmt_sink->code = MEDIA_BUS_FMT_SGBRG10_1X10;
>  	fmt_sink->width = MIPI_CSI2_DEF_PIX_WIDTH;
> @@ -491,38 +487,15 @@ static int imx8mq_mipi_csi_init_cfg(struct v4l2_subdev *sd,
>  		V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink->colorspace,
>  					      fmt_sink->ycbcr_enc);
>  
> -	fmt_source = imx8mq_mipi_csi_get_format(state, sd_state, which,
> -						MIPI_CSI2_PAD_SOURCE);
>  	*fmt_source = *fmt_sink;
>  
>  	return 0;
>  }
>  
> -static int imx8mq_mipi_csi_get_fmt(struct v4l2_subdev *sd,
> -				   struct v4l2_subdev_state *sd_state,
> -				   struct v4l2_subdev_format *sdformat)
> -{
> -	struct csi_state *state = mipi_sd_to_csi2_state(sd);
> -	struct v4l2_mbus_framefmt *fmt;
> -
> -	fmt = imx8mq_mipi_csi_get_format(state, sd_state, sdformat->which,
> -					 sdformat->pad);
> -
> -	mutex_lock(&state->lock);
> -
> -	sdformat->format = *fmt;
> -
> -	mutex_unlock(&state->lock);
> -
> -	return 0;
> -}
> -
>  static int imx8mq_mipi_csi_enum_mbus_code(struct v4l2_subdev *sd,
>  					  struct v4l2_subdev_state *sd_state,
>  					  struct v4l2_subdev_mbus_code_enum *code)
>  {
> -	struct csi_state *state = mipi_sd_to_csi2_state(sd);
> -
>  	/*
>  	 * We can't transcode in any way, the source format is identical
>  	 * to the sink format.
> @@ -533,8 +506,7 @@ static int imx8mq_mipi_csi_enum_mbus_code(struct v4l2_subdev *sd,
>  		if (code->index > 0)
>  			return -EINVAL;
>  
> -		fmt = imx8mq_mipi_csi_get_format(state, sd_state, code->which,
> -						 code->pad);
> +		fmt = v4l2_subdev_get_pad_format(sd, sd_state, code->pad);
>  		code->code = fmt->code;
>  		return 0;
>  	}
> @@ -554,8 +526,7 @@ static int imx8mq_mipi_csi_set_fmt(struct v4l2_subdev *sd,
>  				   struct v4l2_subdev_state *sd_state,
>  				   struct v4l2_subdev_format *sdformat)
>  {
> -	struct csi_state *state = mipi_sd_to_csi2_state(sd);
> -	struct csi2_pix_format const *csi2_fmt;
> +	const struct csi2_pix_format *csi2_fmt;
>  	struct v4l2_mbus_framefmt *fmt;
>  
>  	/*
> @@ -563,7 +534,7 @@ static int imx8mq_mipi_csi_set_fmt(struct v4l2_subdev *sd,
>  	 * modified.
>  	 */
>  	if (sdformat->pad == MIPI_CSI2_PAD_SOURCE)
> -		return imx8mq_mipi_csi_get_fmt(sd, sd_state, sdformat);
> +		return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
>  
>  	if (sdformat->pad != MIPI_CSI2_PAD_SINK)
>  		return -EINVAL;
> @@ -572,10 +543,7 @@ static int imx8mq_mipi_csi_set_fmt(struct v4l2_subdev *sd,
>  	if (!csi2_fmt)
>  		csi2_fmt = &imx8mq_mipi_csi_formats[0];
>  
> -	fmt = imx8mq_mipi_csi_get_format(state, sd_state, sdformat->which,
> -					 sdformat->pad);
> -
> -	mutex_lock(&state->lock);
> +	fmt = v4l2_subdev_get_pad_format(sd, sd_state, sdformat->pad);
>  
>  	fmt->code = csi2_fmt->code;
>  	fmt->width = sdformat->format.width;
> @@ -584,16 +552,9 @@ static int imx8mq_mipi_csi_set_fmt(struct v4l2_subdev *sd,
>  	sdformat->format = *fmt;
>  
>  	/* Propagate the format from sink to source. */
> -	fmt = imx8mq_mipi_csi_get_format(state, sd_state, sdformat->which,
> -					 MIPI_CSI2_PAD_SOURCE);
> +	fmt = v4l2_subdev_get_pad_format(sd, sd_state, MIPI_CSI2_PAD_SOURCE);
>  	*fmt = sdformat->format;
>  
> -	/* Store the CSI2 format descriptor for active formats. */
> -	if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		state->csi2_fmt = csi2_fmt;
> -
> -	mutex_unlock(&state->lock);
> -
>  	return 0;
>  }
>  
> @@ -604,7 +565,7 @@ static const struct v4l2_subdev_video_ops imx8mq_mipi_csi_video_ops = {
>  static const struct v4l2_subdev_pad_ops imx8mq_mipi_csi_pad_ops = {
>  	.init_cfg		= imx8mq_mipi_csi_init_cfg,
>  	.enum_mbus_code		= imx8mq_mipi_csi_enum_mbus_code,
> -	.get_fmt		= imx8mq_mipi_csi_get_fmt,
> +	.get_fmt		= v4l2_subdev_get_fmt,
>  	.set_fmt		= imx8mq_mipi_csi_set_fmt,
>  };
>  
> @@ -732,6 +693,7 @@ static int imx8mq_mipi_csi_pm_resume(struct device *dev)
>  {
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>  	struct csi_state *state = mipi_sd_to_csi2_state(sd);
> +	struct v4l2_subdev_state *sd_state;
>  	int ret = 0;
>  
>  	mutex_lock(&state->lock);
> @@ -741,7 +703,9 @@ static int imx8mq_mipi_csi_pm_resume(struct device *dev)
>  		ret = imx8mq_mipi_csi_clk_enable(state);
>  	}
>  	if (state->state & ST_STREAMING) {
> -		ret = imx8mq_mipi_csi_start_stream(state);
> +		sd_state = v4l2_subdev_lock_and_get_active_state(sd);
> +		ret = imx8mq_mipi_csi_start_stream(state, sd_state);
> +		v4l2_subdev_unlock_state(sd_state);
>  		if (ret)
>  			goto unlock;
>  	}
> @@ -821,6 +785,7 @@ static const struct dev_pm_ops imx8mq_mipi_csi_pm_ops = {
>  static int imx8mq_mipi_csi_subdev_init(struct csi_state *state)
>  {
>  	struct v4l2_subdev *sd = &state->sd;
> +	int ret;
>  
>  	v4l2_subdev_init(sd, &imx8mq_mipi_csi_subdev_ops);
>  	sd->owner = THIS_MODULE;
> @@ -834,15 +799,22 @@ static int imx8mq_mipi_csi_subdev_init(struct csi_state *state)
>  
>  	sd->dev = state->dev;
>  
> -	state->csi2_fmt = &imx8mq_mipi_csi_formats[0];
> -	imx8mq_mipi_csi_init_cfg(sd, NULL);
> -
>  	state->pads[MIPI_CSI2_PAD_SINK].flags = MEDIA_PAD_FL_SINK
>  					 | MEDIA_PAD_FL_MUST_CONNECT;
>  	state->pads[MIPI_CSI2_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE
>  					   | MEDIA_PAD_FL_MUST_CONNECT;
> -	return media_entity_pads_init(&sd->entity, MIPI_CSI2_PADS_NUM,
> -				      state->pads);
> +	ret = media_entity_pads_init(&sd->entity, MIPI_CSI2_PADS_NUM,
> +				     state->pads);
> +	if (ret)
> +		return ret;
> +
> +	ret = v4l2_subdev_init_finalize(sd);
> +	if (ret) {
> +		media_entity_cleanup(&sd->entity);
> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static void imx8mq_mipi_csi_release_icc(struct platform_device *pdev)
> @@ -968,6 +940,7 @@ static int imx8mq_mipi_csi_probe(struct platform_device *pdev)
>  	imx8mq_mipi_csi_runtime_suspend(&pdev->dev);
>  
>  	media_entity_cleanup(&state->sd.entity);
> +	v4l2_subdev_cleanup(&state->sd);
>  	v4l2_async_nf_unregister(&state->notifier);
>  	v4l2_async_nf_cleanup(&state->notifier);
>  	v4l2_async_unregister_subdev(&state->sd);
> @@ -991,6 +964,7 @@ static int imx8mq_mipi_csi_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	imx8mq_mipi_csi_runtime_suspend(&pdev->dev);
>  	media_entity_cleanup(&state->sd.entity);
> +	v4l2_subdev_cleanup(&state->sd);
>  	mutex_destroy(&state->lock);
>  	pm_runtime_set_suspended(&pdev->dev);
>  	imx8mq_mipi_csi_release_icc(pdev);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function

[Laurent Pinchart]

Hi Martin,

Thank you for the patch.

On Tue, Mar 07, 2023 at 04:00:47PM +0100, Martin Kepplinger wrote:
> Clean up the driver a bit by inlining the imx8mq_mipi_csi_system_enable()
> function to the callsites and removing the hs_settle variable from the
> driver state.
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Could I volunteer you to also drop the struct csi_state state field ?
:-)

> ---
>  drivers/staging/media/imx/imx8mq-mipi-csi2.c | 41 ++++++++------------
>  1 file changed, 16 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> index 1aa8622a3bae..f10b59b8f1c0 100644
> --- a/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> +++ b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> @@ -119,9 +119,8 @@ struct csi_state {
>  
>  	struct v4l2_mbus_config_mipi_csi2 bus;
>  
> -	struct mutex lock; /* Protect state and hs_settle */
> +	struct mutex lock; /* Protect state */
>  	u32 state;
> -	u32 hs_settle;
>  
>  	struct regmap *phy_gpr;
>  	u8 phy_gpr_reg;
> @@ -264,23 +263,6 @@ static int imx8mq_mipi_csi_sw_reset(struct csi_state *state)
>  	return 0;
>  }
>  
> -static void imx8mq_mipi_csi_system_enable(struct csi_state *state, int on)
> -{
> -	if (!on) {
> -		imx8mq_mipi_csi_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
> -		return;
> -	}
> -
> -	regmap_update_bits(state->phy_gpr,
> -			   state->phy_gpr_reg,
> -			   0x3fff,
> -			   GPR_CSI2_1_RX_ENABLE |
> -			   GPR_CSI2_1_VID_INTFC_ENB |
> -			   GPR_CSI2_1_HSEL |
> -			   GPR_CSI2_1_CONT_CLK_MODE |
> -			   GPR_CSI2_1_S_PRG_RXHS_SETTLE(state->hs_settle));
> -}
> -
>  static void imx8mq_mipi_csi_set_params(struct csi_state *state)
>  {
>  	int lanes = state->bus.num_data_lanes;
> @@ -321,7 +303,8 @@ static int imx8mq_mipi_csi_clk_get(struct csi_state *state)
>  }
>  
>  static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
> -					  struct v4l2_subdev_state *sd_state)
> +					  struct v4l2_subdev_state *sd_state,
> +					  u32 *hs_settle)
>  {
>  	s64 link_freq;
>  	u32 lane_rate;
> @@ -377,10 +360,10 @@ static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
>  	max_ths_settle = 140 + 10 * 1000000 / (lane_rate / 1000);
>  	ths_settle_ns = (min_ths_settle + max_ths_settle) / 2;
>  
> -	state->hs_settle = ths_settle_ns / esc_clk_period_ns - 1;
> +	*hs_settle = ths_settle_ns / esc_clk_period_ns - 1;
>  
>  	dev_dbg(state->dev, "lane rate %u Ths_settle %u hs_settle %u\n",
> -		lane_rate, ths_settle_ns, state->hs_settle);
> +		lane_rate, ths_settle_ns, *hs_settle);
>  
>  	return 0;
>  }
> @@ -389,24 +372,32 @@ static int imx8mq_mipi_csi_start_stream(struct csi_state *state,
>  					struct v4l2_subdev_state *sd_state)
>  {
>  	int ret;
> +	u32 hs_settle;
>  
>  	ret = imx8mq_mipi_csi_sw_reset(state);
>  	if (ret)
>  		return ret;
>  
>  	imx8mq_mipi_csi_set_params(state);
> -	ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state);
> +	ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state, &hs_settle);
>  	if (ret)
>  		return ret;
>  
> -	imx8mq_mipi_csi_system_enable(state, true);
> +	regmap_update_bits(state->phy_gpr,
> +			   state->phy_gpr_reg,
> +			   0x3fff,
> +			   GPR_CSI2_1_RX_ENABLE |
> +			   GPR_CSI2_1_VID_INTFC_ENB |
> +			   GPR_CSI2_1_HSEL |
> +			   GPR_CSI2_1_CONT_CLK_MODE |
> +			   GPR_CSI2_1_S_PRG_RXHS_SETTLE(hs_settle));
>  
>  	return 0;
>  }
>  
>  static void imx8mq_mipi_csi_stop_stream(struct csi_state *state)
>  {
> -	imx8mq_mipi_csi_system_enable(state, false);
> +	imx8mq_mipi_csi_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
>  }
>  
>  /* -----------------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state

[Laurent Pinchart]

On Sun, Mar 12, 2023 at 03:31:23PM +0200, Laurent Pinchart wrote:
> Hi Martin,
> 
> Thank you for the patch.
> 
> On Tue, Mar 07, 2023 at 04:00:46PM +0100, Martin Kepplinger wrote:
> > Simplify the driver by using the V4L2 subdev active state API to store
> > the active format.
> > 
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > ---
> >  drivers/staging/media/imx/imx8mq-mipi-csi2.c | 118 ++++++++-----------
> >  1 file changed, 46 insertions(+), 72 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > index c0d0bf770096..1aa8622a3bae 100644
> > --- a/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > +++ b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > @@ -119,9 +119,7 @@ struct csi_state {
> >  
> >  	struct v4l2_mbus_config_mipi_csi2 bus;
> >  
> > -	struct mutex lock; /* Protect csi2_fmt, format_mbus, state, hs_settle */
> > -	const struct csi2_pix_format *csi2_fmt;
> > -	struct v4l2_mbus_framefmt format_mbus[MIPI_CSI2_PADS_NUM];
> > +	struct mutex lock; /* Protect state and hs_settle */
> >  	u32 state;
> >  	u32 hs_settle;
> >  
> > @@ -322,16 +320,23 @@ static int imx8mq_mipi_csi_clk_get(struct csi_state *state)
> >  	return devm_clk_bulk_get(state->dev, CSI2_NUM_CLKS, state->clks);
> >  }
> >  
> > -static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state)
> > +static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
> > +					  struct v4l2_subdev_state *sd_state)
> >  {
> >  	s64 link_freq;
> >  	u32 lane_rate;
> >  	unsigned long esc_clk_rate;
> >  	u32 min_ths_settle, max_ths_settle, ths_settle_ns, esc_clk_period_ns;
> > +	const struct v4l2_mbus_framefmt *fmt;
> > +	const struct csi2_pix_format *csi2_fmt;
> >  
> >  	/* Calculate the line rate from the pixel rate. */
> > +
> > +	fmt = v4l2_subdev_get_pad_format(&state->sd, sd_state, MIPI_CSI2_PAD_SINK);
> > +	csi2_fmt = find_csi2_format(fmt->code);
> > +
> >  	link_freq = v4l2_get_link_freq(state->src_sd->ctrl_handler,
> > -				       state->csi2_fmt->width,
> > +				       csi2_fmt->width,
> >  				       state->bus.num_data_lanes * 2);
> >  	if (link_freq < 0) {
> >  		dev_err(state->dev, "Unable to obtain link frequency: %d\n",
> > @@ -380,7 +385,8 @@ static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state)
> >  	return 0;
> >  }
> >  
> > -static int imx8mq_mipi_csi_start_stream(struct csi_state *state)
> > +static int imx8mq_mipi_csi_start_stream(struct csi_state *state,
> > +					struct v4l2_subdev_state *sd_state)
> >  {
> >  	int ret;
> >  
> > @@ -389,7 +395,7 @@ static int imx8mq_mipi_csi_start_stream(struct csi_state *state)
> >  		return ret;
> >  
> >  	imx8mq_mipi_csi_set_params(state);
> > -	ret = imx8mq_mipi_csi_calc_hs_settle(state);
> > +	ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -415,6 +421,7 @@ static struct csi_state *mipi_sd_to_csi2_state(struct v4l2_subdev *sdev)
> >  static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >  	struct csi_state *state = mipi_sd_to_csi2_state(sd);
> > +	struct v4l2_subdev_state *sd_state;
> >  	int ret = 0;
> >  
> >  	if (enable) {
> > @@ -431,7 +438,9 @@ static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int enable)
> >  			goto unlock;
> >  		}
> >  
> > -		ret = imx8mq_mipi_csi_start_stream(state);
> > +		sd_state = v4l2_subdev_lock_and_get_active_state(sd);
> > +
> > +		ret = imx8mq_mipi_csi_start_stream(state, sd_state);
> >  		if (ret < 0)
> >  			goto unlock;
> 
> You're leaving the state locked here. I would write
> 
> 		sd_state = v4l2_subdev_lock_and_get_active_state(sd);
> 		ret = imx8mq_mipi_csi_start_stream(state, sd_state);
> 		v4l2_subdev_unlock_state(sd_state);
> 
> 		if (ret < 0)
> 			goto unlock;
> 
> and drop the v4l2_subdev_unlock_state() call below. Apart from that,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

If you're fine with this change, there's no need so submit a v3, I'll
handle this locally.

> >  
> > @@ -440,6 +449,8 @@ static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int enable)
> >  			goto unlock;
> >  
> >  		state->state |= ST_STREAMING;
> > +
> > +		v4l2_subdev_unlock_state(sd_state);
> >  	} else {
> >  		v4l2_subdev_call(state->src_sd, video, s_stream, 0);
> >  		imx8mq_mipi_csi_stop_stream(state);
> > @@ -455,29 +466,14 @@ static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int enable)
> >  	return ret;
> >  }
> >  
> > -static struct v4l2_mbus_framefmt *
> > -imx8mq_mipi_csi_get_format(struct csi_state *state,
> > -			   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(&state->sd, sd_state, pad);
> > -
> > -	return &state->format_mbus[pad];
> > -}
> > -
> >  static int imx8mq_mipi_csi_init_cfg(struct v4l2_subdev *sd,
> >  				    struct v4l2_subdev_state *sd_state)
> >  {
> > -	struct csi_state *state = mipi_sd_to_csi2_state(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 = imx8mq_mipi_csi_get_format(state, sd_state, which,
> > -					      MIPI_CSI2_PAD_SINK);
> > +	fmt_sink = v4l2_subdev_get_pad_format(sd, sd_state, MIPI_CSI2_PAD_SINK);
> > +	fmt_source = v4l2_subdev_get_pad_format(sd, sd_state, MIPI_CSI2_PAD_SOURCE);
> >  
> >  	fmt_sink->code = MEDIA_BUS_FMT_SGBRG10_1X10;
> >  	fmt_sink->width = MIPI_CSI2_DEF_PIX_WIDTH;
> > @@ -491,38 +487,15 @@ static int imx8mq_mipi_csi_init_cfg(struct v4l2_subdev *sd,
> >  		V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink->colorspace,
> >  					      fmt_sink->ycbcr_enc);
> >  
> > -	fmt_source = imx8mq_mipi_csi_get_format(state, sd_state, which,
> > -						MIPI_CSI2_PAD_SOURCE);
> >  	*fmt_source = *fmt_sink;
> >  
> >  	return 0;
> >  }
> >  
> > -static int imx8mq_mipi_csi_get_fmt(struct v4l2_subdev *sd,
> > -				   struct v4l2_subdev_state *sd_state,
> > -				   struct v4l2_subdev_format *sdformat)
> > -{
> > -	struct csi_state *state = mipi_sd_to_csi2_state(sd);
> > -	struct v4l2_mbus_framefmt *fmt;
> > -
> > -	fmt = imx8mq_mipi_csi_get_format(state, sd_state, sdformat->which,
> > -					 sdformat->pad);
> > -
> > -	mutex_lock(&state->lock);
> > -
> > -	sdformat->format = *fmt;
> > -
> > -	mutex_unlock(&state->lock);
> > -
> > -	return 0;
> > -}
> > -
> >  static int imx8mq_mipi_csi_enum_mbus_code(struct v4l2_subdev *sd,
> >  					  struct v4l2_subdev_state *sd_state,
> >  					  struct v4l2_subdev_mbus_code_enum *code)
> >  {
> > -	struct csi_state *state = mipi_sd_to_csi2_state(sd);
> > -
> >  	/*
> >  	 * We can't transcode in any way, the source format is identical
> >  	 * to the sink format.
> > @@ -533,8 +506,7 @@ static int imx8mq_mipi_csi_enum_mbus_code(struct v4l2_subdev *sd,
> >  		if (code->index > 0)
> >  			return -EINVAL;
> >  
> > -		fmt = imx8mq_mipi_csi_get_format(state, sd_state, code->which,
> > -						 code->pad);
> > +		fmt = v4l2_subdev_get_pad_format(sd, sd_state, code->pad);
> >  		code->code = fmt->code;
> >  		return 0;
> >  	}
> > @@ -554,8 +526,7 @@ static int imx8mq_mipi_csi_set_fmt(struct v4l2_subdev *sd,
> >  				   struct v4l2_subdev_state *sd_state,
> >  				   struct v4l2_subdev_format *sdformat)
> >  {
> > -	struct csi_state *state = mipi_sd_to_csi2_state(sd);
> > -	struct csi2_pix_format const *csi2_fmt;
> > +	const struct csi2_pix_format *csi2_fmt;
> >  	struct v4l2_mbus_framefmt *fmt;
> >  
> >  	/*
> > @@ -563,7 +534,7 @@ static int imx8mq_mipi_csi_set_fmt(struct v4l2_subdev *sd,
> >  	 * modified.
> >  	 */
> >  	if (sdformat->pad == MIPI_CSI2_PAD_SOURCE)
> > -		return imx8mq_mipi_csi_get_fmt(sd, sd_state, sdformat);
> > +		return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
> >  
> >  	if (sdformat->pad != MIPI_CSI2_PAD_SINK)
> >  		return -EINVAL;
> > @@ -572,10 +543,7 @@ static int imx8mq_mipi_csi_set_fmt(struct v4l2_subdev *sd,
> >  	if (!csi2_fmt)
> >  		csi2_fmt = &imx8mq_mipi_csi_formats[0];
> >  
> > -	fmt = imx8mq_mipi_csi_get_format(state, sd_state, sdformat->which,
> > -					 sdformat->pad);
> > -
> > -	mutex_lock(&state->lock);
> > +	fmt = v4l2_subdev_get_pad_format(sd, sd_state, sdformat->pad);
> >  
> >  	fmt->code = csi2_fmt->code;
> >  	fmt->width = sdformat->format.width;
> > @@ -584,16 +552,9 @@ static int imx8mq_mipi_csi_set_fmt(struct v4l2_subdev *sd,
> >  	sdformat->format = *fmt;
> >  
> >  	/* Propagate the format from sink to source. */
> > -	fmt = imx8mq_mipi_csi_get_format(state, sd_state, sdformat->which,
> > -					 MIPI_CSI2_PAD_SOURCE);
> > +	fmt = v4l2_subdev_get_pad_format(sd, sd_state, MIPI_CSI2_PAD_SOURCE);
> >  	*fmt = sdformat->format;
> >  
> > -	/* Store the CSI2 format descriptor for active formats. */
> > -	if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > -		state->csi2_fmt = csi2_fmt;
> > -
> > -	mutex_unlock(&state->lock);
> > -
> >  	return 0;
> >  }
> >  
> > @@ -604,7 +565,7 @@ static const struct v4l2_subdev_video_ops imx8mq_mipi_csi_video_ops = {
> >  static const struct v4l2_subdev_pad_ops imx8mq_mipi_csi_pad_ops = {
> >  	.init_cfg		= imx8mq_mipi_csi_init_cfg,
> >  	.enum_mbus_code		= imx8mq_mipi_csi_enum_mbus_code,
> > -	.get_fmt		= imx8mq_mipi_csi_get_fmt,
> > +	.get_fmt		= v4l2_subdev_get_fmt,
> >  	.set_fmt		= imx8mq_mipi_csi_set_fmt,
> >  };
> >  
> > @@ -732,6 +693,7 @@ static int imx8mq_mipi_csi_pm_resume(struct device *dev)
> >  {
> >  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >  	struct csi_state *state = mipi_sd_to_csi2_state(sd);
> > +	struct v4l2_subdev_state *sd_state;
> >  	int ret = 0;
> >  
> >  	mutex_lock(&state->lock);
> > @@ -741,7 +703,9 @@ static int imx8mq_mipi_csi_pm_resume(struct device *dev)
> >  		ret = imx8mq_mipi_csi_clk_enable(state);
> >  	}
> >  	if (state->state & ST_STREAMING) {
> > -		ret = imx8mq_mipi_csi_start_stream(state);
> > +		sd_state = v4l2_subdev_lock_and_get_active_state(sd);
> > +		ret = imx8mq_mipi_csi_start_stream(state, sd_state);
> > +		v4l2_subdev_unlock_state(sd_state);
> >  		if (ret)
> >  			goto unlock;
> >  	}
> > @@ -821,6 +785,7 @@ static const struct dev_pm_ops imx8mq_mipi_csi_pm_ops = {
> >  static int imx8mq_mipi_csi_subdev_init(struct csi_state *state)
> >  {
> >  	struct v4l2_subdev *sd = &state->sd;
> > +	int ret;
> >  
> >  	v4l2_subdev_init(sd, &imx8mq_mipi_csi_subdev_ops);
> >  	sd->owner = THIS_MODULE;
> > @@ -834,15 +799,22 @@ static int imx8mq_mipi_csi_subdev_init(struct csi_state *state)
> >  
> >  	sd->dev = state->dev;
> >  
> > -	state->csi2_fmt = &imx8mq_mipi_csi_formats[0];
> > -	imx8mq_mipi_csi_init_cfg(sd, NULL);
> > -
> >  	state->pads[MIPI_CSI2_PAD_SINK].flags = MEDIA_PAD_FL_SINK
> >  					 | MEDIA_PAD_FL_MUST_CONNECT;
> >  	state->pads[MIPI_CSI2_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE
> >  					   | MEDIA_PAD_FL_MUST_CONNECT;
> > -	return media_entity_pads_init(&sd->entity, MIPI_CSI2_PADS_NUM,
> > -				      state->pads);
> > +	ret = media_entity_pads_init(&sd->entity, MIPI_CSI2_PADS_NUM,
> > +				     state->pads);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = v4l2_subdev_init_finalize(sd);
> > +	if (ret) {
> > +		media_entity_cleanup(&sd->entity);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> >  }
> >  
> >  static void imx8mq_mipi_csi_release_icc(struct platform_device *pdev)
> > @@ -968,6 +940,7 @@ static int imx8mq_mipi_csi_probe(struct platform_device *pdev)
> >  	imx8mq_mipi_csi_runtime_suspend(&pdev->dev);
> >  
> >  	media_entity_cleanup(&state->sd.entity);
> > +	v4l2_subdev_cleanup(&state->sd);
> >  	v4l2_async_nf_unregister(&state->notifier);
> >  	v4l2_async_nf_cleanup(&state->notifier);
> >  	v4l2_async_unregister_subdev(&state->sd);
> > @@ -991,6 +964,7 @@ static int imx8mq_mipi_csi_remove(struct platform_device *pdev)
> >  	pm_runtime_disable(&pdev->dev);
> >  	imx8mq_mipi_csi_runtime_suspend(&pdev->dev);
> >  	media_entity_cleanup(&state->sd.entity);
> > +	v4l2_subdev_cleanup(&state->sd);
> >  	mutex_destroy(&state->lock);
> >  	pm_runtime_set_suspended(&pdev->dev);
> >  	imx8mq_mipi_csi_release_icc(pdev);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function

[Martin Kepplinger]

Am Sonntag, dem 12.03.2023 um 15:37 +0200 schrieb Laurent Pinchart:
> Hi Martin,
> 
> Thank you for the patch.
> 
> On Tue, Mar 07, 2023 at 04:00:47PM +0100, Martin Kepplinger wrote:
> > Clean up the driver a bit by inlining the
> > imx8mq_mipi_csi_system_enable()
> > function to the callsites and removing the hs_settle variable from
> > the
> > driver state.
> > 
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Could I volunteer you to also drop the struct csi_state state field ?
> :-)

sure :) it can become at least a bit more tricky than this patch. I'll
take the time after this is merged.

thanks for the fast reviewing

> 
> > ---
> >  drivers/staging/media/imx/imx8mq-mipi-csi2.c | 41 ++++++++--------
> > ----
> >  1 file changed, 16 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > index 1aa8622a3bae..f10b59b8f1c0 100644
> > --- a/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > +++ b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > @@ -119,9 +119,8 @@ struct csi_state {
> >  
> >         struct v4l2_mbus_config_mipi_csi2 bus;
> >  
> > -       struct mutex lock; /* Protect state and hs_settle */
> > +       struct mutex lock; /* Protect state */
> >         u32 state;
> > -       u32 hs_settle;
> >  
> >         struct regmap *phy_gpr;
> >         u8 phy_gpr_reg;
> > @@ -264,23 +263,6 @@ static int imx8mq_mipi_csi_sw_reset(struct
> > csi_state *state)
> >         return 0;
> >  }
> >  
> > -static void imx8mq_mipi_csi_system_enable(struct csi_state *state,
> > int on)
> > -{
> > -       if (!on) {
> > -               imx8mq_mipi_csi_write(state,
> > CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
> > -               return;
> > -       }
> > -
> > -       regmap_update_bits(state->phy_gpr,
> > -                          state->phy_gpr_reg,
> > -                          0x3fff,
> > -                          GPR_CSI2_1_RX_ENABLE |
> > -                          GPR_CSI2_1_VID_INTFC_ENB |
> > -                          GPR_CSI2_1_HSEL |
> > -                          GPR_CSI2_1_CONT_CLK_MODE |
> > -                          GPR_CSI2_1_S_PRG_RXHS_SETTLE(state-
> > >hs_settle));
> > -}
> > -
> >  static void imx8mq_mipi_csi_set_params(struct csi_state *state)
> >  {
> >         int lanes = state->bus.num_data_lanes;
> > @@ -321,7 +303,8 @@ static int imx8mq_mipi_csi_clk_get(struct
> > csi_state *state)
> >  }
> >  
> >  static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
> > -                                         struct v4l2_subdev_state
> > *sd_state)
> > +                                         struct v4l2_subdev_state
> > *sd_state,
> > +                                         u32 *hs_settle)
> >  {
> >         s64 link_freq;
> >         u32 lane_rate;
> > @@ -377,10 +360,10 @@ static int
> > imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
> >         max_ths_settle = 140 + 10 * 1000000 / (lane_rate / 1000);
> >         ths_settle_ns = (min_ths_settle + max_ths_settle) / 2;
> >  
> > -       state->hs_settle = ths_settle_ns / esc_clk_period_ns - 1;
> > +       *hs_settle = ths_settle_ns / esc_clk_period_ns - 1;
> >  
> >         dev_dbg(state->dev, "lane rate %u Ths_settle %u hs_settle
> > %u\n",
> > -               lane_rate, ths_settle_ns, state->hs_settle);
> > +               lane_rate, ths_settle_ns, *hs_settle);
> >  
> >         return 0;
> >  }
> > @@ -389,24 +372,32 @@ static int
> > imx8mq_mipi_csi_start_stream(struct csi_state *state,
> >                                         struct v4l2_subdev_state
> > *sd_state)
> >  {
> >         int ret;
> > +       u32 hs_settle;
> >  
> >         ret = imx8mq_mipi_csi_sw_reset(state);
> >         if (ret)
> >                 return ret;
> >  
> >         imx8mq_mipi_csi_set_params(state);
> > -       ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state);
> > +       ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state,
> > &hs_settle);
> >         if (ret)
> >                 return ret;
> >  
> > -       imx8mq_mipi_csi_system_enable(state, true);
> > +       regmap_update_bits(state->phy_gpr,
> > +                          state->phy_gpr_reg,
> > +                          0x3fff,
> > +                          GPR_CSI2_1_RX_ENABLE |
> > +                          GPR_CSI2_1_VID_INTFC_ENB |
> > +                          GPR_CSI2_1_HSEL |
> > +                          GPR_CSI2_1_CONT_CLK_MODE |
> > +                         
> > GPR_CSI2_1_S_PRG_RXHS_SETTLE(hs_settle));
> >  
> >         return 0;
> >  }
> >  
> >  static void imx8mq_mipi_csi_stop_stream(struct csi_state *state)
> >  {
> > -       imx8mq_mipi_csi_system_enable(state, false);
> > +       imx8mq_mipi_csi_write(state, CSI2RX_CFG_DISABLE_DATA_LANES,
> > 0xf);
> >  }
> >  
> >  /* ---------------------------------------------------------------
> > --------------
> 

Re: [PATCH v2 1/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state

[Martin Kepplinger]

Am Sonntag, dem 12.03.2023 um 15:39 +0200 schrieb Laurent Pinchart:
> On Sun, Mar 12, 2023 at 03:31:23PM +0200, Laurent Pinchart wrote:
> > Hi Martin,
> > 
> > Thank you for the patch.
> > 
> > On Tue, Mar 07, 2023 at 04:00:46PM +0100, Martin Kepplinger wrote:
> > > Simplify the driver by using the V4L2 subdev active state API to
> > > store
> > > the active format.
> > > 
> > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > > ---
> > >  drivers/staging/media/imx/imx8mq-mipi-csi2.c | 118 ++++++++-----
> > > ------
> > >  1 file changed, 46 insertions(+), 72 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > > b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > > index c0d0bf770096..1aa8622a3bae 100644
> > > --- a/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > > +++ b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > > @@ -119,9 +119,7 @@ struct csi_state {
> > >  
> > >         struct v4l2_mbus_config_mipi_csi2 bus;
> > >  
> > > -       struct mutex lock; /* Protect csi2_fmt, format_mbus,
> > > state, hs_settle */
> > > -       const struct csi2_pix_format *csi2_fmt;
> > > -       struct v4l2_mbus_framefmt
> > > format_mbus[MIPI_CSI2_PADS_NUM];
> > > +       struct mutex lock; /* Protect state and hs_settle */
> > >         u32 state;
> > >         u32 hs_settle;
> > >  
> > > @@ -322,16 +320,23 @@ static int imx8mq_mipi_csi_clk_get(struct
> > > csi_state *state)
> > >         return devm_clk_bulk_get(state->dev, CSI2_NUM_CLKS,
> > > state->clks);
> > >  }
> > >  
> > > -static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state
> > > *state)
> > > +static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state
> > > *state,
> > > +                                         struct
> > > v4l2_subdev_state *sd_state)
> > >  {
> > >         s64 link_freq;
> > >         u32 lane_rate;
> > >         unsigned long esc_clk_rate;
> > >         u32 min_ths_settle, max_ths_settle, ths_settle_ns,
> > > esc_clk_period_ns;
> > > +       const struct v4l2_mbus_framefmt *fmt;
> > > +       const struct csi2_pix_format *csi2_fmt;
> > >  
> > >         /* Calculate the line rate from the pixel rate. */
> > > +
> > > +       fmt = v4l2_subdev_get_pad_format(&state->sd, sd_state,
> > > MIPI_CSI2_PAD_SINK);
> > > +       csi2_fmt = find_csi2_format(fmt->code);
> > > +
> > >         link_freq = v4l2_get_link_freq(state->src_sd-
> > > >ctrl_handler,
> > > -                                      state->csi2_fmt->width,
> > > +                                      csi2_fmt->width,
> > >                                        state->bus.num_data_lanes
> > > * 2);
> > >         if (link_freq < 0) {
> > >                 dev_err(state->dev, "Unable to obtain link
> > > frequency: %d\n",
> > > @@ -380,7 +385,8 @@ static int
> > > imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state)
> > >         return 0;
> > >  }
> > >  
> > > -static int imx8mq_mipi_csi_start_stream(struct csi_state *state)
> > > +static int imx8mq_mipi_csi_start_stream(struct csi_state *state,
> > > +                                       struct v4l2_subdev_state
> > > *sd_state)
> > >  {
> > >         int ret;
> > >  
> > > @@ -389,7 +395,7 @@ static int
> > > imx8mq_mipi_csi_start_stream(struct csi_state *state)
> > >                 return ret;
> > >  
> > >         imx8mq_mipi_csi_set_params(state);
> > > -       ret = imx8mq_mipi_csi_calc_hs_settle(state);
> > > +       ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state);
> > >         if (ret)
> > >                 return ret;
> > >  
> > > @@ -415,6 +421,7 @@ static struct csi_state
> > > *mipi_sd_to_csi2_state(struct v4l2_subdev *sdev)
> > >  static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int
> > > enable)
> > >  {
> > >         struct csi_state *state = mipi_sd_to_csi2_state(sd);
> > > +       struct v4l2_subdev_state *sd_state;
> > >         int ret = 0;
> > >  
> > >         if (enable) {
> > > @@ -431,7 +438,9 @@ static int imx8mq_mipi_csi_s_stream(struct
> > > v4l2_subdev *sd, int enable)
> > >                         goto unlock;
> > >                 }
> > >  
> > > -               ret = imx8mq_mipi_csi_start_stream(state);
> > > +               sd_state =
> > > v4l2_subdev_lock_and_get_active_state(sd);
> > > +
> > > +               ret = imx8mq_mipi_csi_start_stream(state,
> > > sd_state);
> > >                 if (ret < 0)
> > >                         goto unlock;
> > 
> > You're leaving the state locked here. I would write
> > 
> >                 sd_state =
> > v4l2_subdev_lock_and_get_active_state(sd);
> >                 ret = imx8mq_mipi_csi_start_stream(state,
> > sd_state);
> >                 v4l2_subdev_unlock_state(sd_state);
> > 
> >                 if (ret < 0)
> >                         goto unlock;
> > 
> > and drop the v4l2_subdev_unlock_state() call below. Apart from
> > that,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> If you're fine with this change, there's no need so submit a v3, I'll
> handle this locally.

Please do. I'm fine with this change, yes. Thanks a lot,

> 
> > >  
> > > @@ -440,6 +449,8 @@ static int imx8mq_mipi_csi_s_stream(struct
> > > v4l2_subdev *sd, int enable)
> > >                         goto unlock;
> > >  
> > >                 state->state |= ST_STREAMING;
> > > +
> > > +               v4l2_subdev_unlock_state(sd_state);
> > >         } else {
> > >                 v4l2_subdev_call(state->src_sd, video, s_stream,
> > > 0);
> > >                 imx8mq_mipi_csi_stop_stream(state);
> > > @@ -455,29 +466,14 @@ static int imx8mq_mipi_csi_s_stream(struct
> > > v4l2_subdev *sd, int enable)
> > >         return ret;
> > >  }
> > >  
> > > -static struct v4l2_mbus_framefmt *
> > > -imx8mq_mipi_csi_get_format(struct csi_state *state,
> > > -                          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(&state->sd,
> > > sd_state, pad);
> > > -
> > > -       return &state->format_mbus[pad];
> > > -}
> > > -
> > >  static int imx8mq_mipi_csi_init_cfg(struct v4l2_subdev *sd,
> > >                                     struct v4l2_subdev_state
> > > *sd_state)
> > >  {
> > > -       struct csi_state *state = mipi_sd_to_csi2_state(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 = imx8mq_mipi_csi_get_format(state, sd_state,
> > > which,
> > > -                                            
> > > MIPI_CSI2_PAD_SINK);
> > > +       fmt_sink = v4l2_subdev_get_pad_format(sd, sd_state,
> > > MIPI_CSI2_PAD_SINK);
> > > +       fmt_source = v4l2_subdev_get_pad_format(sd, sd_state,
> > > MIPI_CSI2_PAD_SOURCE);
> > >  
> > >         fmt_sink->code = MEDIA_BUS_FMT_SGBRG10_1X10;
> > >         fmt_sink->width = MIPI_CSI2_DEF_PIX_WIDTH;
> > > @@ -491,38 +487,15 @@ static int imx8mq_mipi_csi_init_cfg(struct
> > > v4l2_subdev *sd,
> > >                 V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink-
> > > >colorspace,
> > >                                               fmt_sink-
> > > >ycbcr_enc);
> > >  
> > > -       fmt_source = imx8mq_mipi_csi_get_format(state, sd_state,
> > > which,
> > > -
> > >                                                MIPI_CSI2_PAD_SOURC
> > > E);
> > >         *fmt_source = *fmt_sink;
> > >  
> > >         return 0;
> > >  }
> > >  
> > > -static int imx8mq_mipi_csi_get_fmt(struct v4l2_subdev *sd,
> > > -                                  struct v4l2_subdev_state
> > > *sd_state,
> > > -                                  struct v4l2_subdev_format
> > > *sdformat)
> > > -{
> > > -       struct csi_state *state = mipi_sd_to_csi2_state(sd);
> > > -       struct v4l2_mbus_framefmt *fmt;
> > > -
> > > -       fmt = imx8mq_mipi_csi_get_format(state, sd_state,
> > > sdformat->which,
> > > -                                        sdformat->pad);
> > > -
> > > -       mutex_lock(&state->lock);
> > > -
> > > -       sdformat->format = *fmt;
> > > -
> > > -       mutex_unlock(&state->lock);
> > > -
> > > -       return 0;
> > > -}
> > > -
> > >  static int imx8mq_mipi_csi_enum_mbus_code(struct v4l2_subdev
> > > *sd,
> > >                                           struct
> > > v4l2_subdev_state *sd_state,
> > >                                           struct
> > > v4l2_subdev_mbus_code_enum *code)
> > >  {
> > > -       struct csi_state *state = mipi_sd_to_csi2_state(sd);
> > > -
> > >         /*
> > >          * We can't transcode in any way, the source format is
> > > identical
> > >          * to the sink format.
> > > @@ -533,8 +506,7 @@ static int
> > > imx8mq_mipi_csi_enum_mbus_code(struct v4l2_subdev *sd,
> > >                 if (code->index > 0)
> > >                         return -EINVAL;
> > >  
> > > -               fmt = imx8mq_mipi_csi_get_format(state, sd_state,
> > > code->which,
> > > -                                                code->pad);
> > > +               fmt = v4l2_subdev_get_pad_format(sd, sd_state,
> > > code->pad);
> > >                 code->code = fmt->code;
> > >                 return 0;
> > >         }
> > > @@ -554,8 +526,7 @@ static int imx8mq_mipi_csi_set_fmt(struct
> > > v4l2_subdev *sd,
> > >                                    struct v4l2_subdev_state
> > > *sd_state,
> > >                                    struct v4l2_subdev_format
> > > *sdformat)
> > >  {
> > > -       struct csi_state *state = mipi_sd_to_csi2_state(sd);
> > > -       struct csi2_pix_format const *csi2_fmt;
> > > +       const struct csi2_pix_format *csi2_fmt;
> > >         struct v4l2_mbus_framefmt *fmt;
> > >  
> > >         /*
> > > @@ -563,7 +534,7 @@ static int imx8mq_mipi_csi_set_fmt(struct
> > > v4l2_subdev *sd,
> > >          * modified.
> > >          */
> > >         if (sdformat->pad == MIPI_CSI2_PAD_SOURCE)
> > > -               return imx8mq_mipi_csi_get_fmt(sd, sd_state,
> > > sdformat);
> > > +               return v4l2_subdev_get_fmt(sd, sd_state,
> > > sdformat);
> > >  
> > >         if (sdformat->pad != MIPI_CSI2_PAD_SINK)
> > >                 return -EINVAL;
> > > @@ -572,10 +543,7 @@ static int imx8mq_mipi_csi_set_fmt(struct
> > > v4l2_subdev *sd,
> > >         if (!csi2_fmt)
> > >                 csi2_fmt = &imx8mq_mipi_csi_formats[0];
> > >  
> > > -       fmt = imx8mq_mipi_csi_get_format(state, sd_state,
> > > sdformat->which,
> > > -                                        sdformat->pad);
> > > -
> > > -       mutex_lock(&state->lock);
> > > +       fmt = v4l2_subdev_get_pad_format(sd, sd_state, sdformat-
> > > >pad);
> > >  
> > >         fmt->code = csi2_fmt->code;
> > >         fmt->width = sdformat->format.width;
> > > @@ -584,16 +552,9 @@ static int imx8mq_mipi_csi_set_fmt(struct
> > > v4l2_subdev *sd,
> > >         sdformat->format = *fmt;
> > >  
> > >         /* Propagate the format from sink to source. */
> > > -       fmt = imx8mq_mipi_csi_get_format(state, sd_state,
> > > sdformat->which,
> > > -                                        MIPI_CSI2_PAD_SOURCE);
> > > +       fmt = v4l2_subdev_get_pad_format(sd, sd_state,
> > > MIPI_CSI2_PAD_SOURCE);
> > >         *fmt = sdformat->format;
> > >  
> > > -       /* Store the CSI2 format descriptor for active formats.
> > > */
> > > -       if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > > -               state->csi2_fmt = csi2_fmt;
> > > -
> > > -       mutex_unlock(&state->lock);
> > > -
> > >         return 0;
> > >  }
> > >  
> > > @@ -604,7 +565,7 @@ static const struct v4l2_subdev_video_ops
> > > imx8mq_mipi_csi_video_ops = {
> > >  static const struct v4l2_subdev_pad_ops imx8mq_mipi_csi_pad_ops
> > > = {
> > >         .init_cfg               = imx8mq_mipi_csi_init_cfg,
> > >         .enum_mbus_code         = imx8mq_mipi_csi_enum_mbus_code,
> > > -       .get_fmt                = imx8mq_mipi_csi_get_fmt,
> > > +       .get_fmt                = v4l2_subdev_get_fmt,
> > >         .set_fmt                = imx8mq_mipi_csi_set_fmt,
> > >  };
> > >  
> > > @@ -732,6 +693,7 @@ static int imx8mq_mipi_csi_pm_resume(struct
> > > device *dev)
> > >  {
> > >         struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > >         struct csi_state *state = mipi_sd_to_csi2_state(sd);
> > > +       struct v4l2_subdev_state *sd_state;
> > >         int ret = 0;
> > >  
> > >         mutex_lock(&state->lock);
> > > @@ -741,7 +703,9 @@ static int imx8mq_mipi_csi_pm_resume(struct
> > > device *dev)
> > >                 ret = imx8mq_mipi_csi_clk_enable(state);
> > >         }
> > >         if (state->state & ST_STREAMING) {
> > > -               ret = imx8mq_mipi_csi_start_stream(state);
> > > +               sd_state =
> > > v4l2_subdev_lock_and_get_active_state(sd);
> > > +               ret = imx8mq_mipi_csi_start_stream(state,
> > > sd_state);
> > > +               v4l2_subdev_unlock_state(sd_state);
> > >                 if (ret)
> > >                         goto unlock;
> > >         }
> > > @@ -821,6 +785,7 @@ static const struct dev_pm_ops
> > > imx8mq_mipi_csi_pm_ops = {
> > >  static int imx8mq_mipi_csi_subdev_init(struct csi_state *state)
> > >  {
> > >         struct v4l2_subdev *sd = &state->sd;
> > > +       int ret;
> > >  
> > >         v4l2_subdev_init(sd, &imx8mq_mipi_csi_subdev_ops);
> > >         sd->owner = THIS_MODULE;
> > > @@ -834,15 +799,22 @@ static int
> > > imx8mq_mipi_csi_subdev_init(struct csi_state *state)
> > >  
> > >         sd->dev = state->dev;
> > >  
> > > -       state->csi2_fmt = &imx8mq_mipi_csi_formats[0];
> > > -       imx8mq_mipi_csi_init_cfg(sd, NULL);
> > > -
> > >         state->pads[MIPI_CSI2_PAD_SINK].flags = MEDIA_PAD_FL_SINK
> > >                                          |
> > > MEDIA_PAD_FL_MUST_CONNECT;
> > >         state->pads[MIPI_CSI2_PAD_SOURCE].flags =
> > > MEDIA_PAD_FL_SOURCE
> > >                                            |
> > > MEDIA_PAD_FL_MUST_CONNECT;
> > > -       return media_entity_pads_init(&sd->entity,
> > > MIPI_CSI2_PADS_NUM,
> > > -                                     state->pads);
> > > +       ret = media_entity_pads_init(&sd->entity,
> > > MIPI_CSI2_PADS_NUM,
> > > +                                    state->pads);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       ret = v4l2_subdev_init_finalize(sd);
> > > +       if (ret) {
> > > +               media_entity_cleanup(&sd->entity);
> > > +               return ret;
> > > +       }
> > > +
> > > +       return 0;
> > >  }
> > >  
> > >  static void imx8mq_mipi_csi_release_icc(struct platform_device
> > > *pdev)
> > > @@ -968,6 +940,7 @@ static int imx8mq_mipi_csi_probe(struct
> > > platform_device *pdev)
> > >         imx8mq_mipi_csi_runtime_suspend(&pdev->dev);
> > >  
> > >         media_entity_cleanup(&state->sd.entity);
> > > +       v4l2_subdev_cleanup(&state->sd);
> > >         v4l2_async_nf_unregister(&state->notifier);
> > >         v4l2_async_nf_cleanup(&state->notifier);
> > >         v4l2_async_unregister_subdev(&state->sd);
> > > @@ -991,6 +964,7 @@ static int imx8mq_mipi_csi_remove(struct
> > > platform_device *pdev)
> > >         pm_runtime_disable(&pdev->dev);
> > >         imx8mq_mipi_csi_runtime_suspend(&pdev->dev);
> > >         media_entity_cleanup(&state->sd.entity);
> > > +       v4l2_subdev_cleanup(&state->sd);
> > >         mutex_destroy(&state->lock);
> > >         pm_runtime_set_suspended(&pdev->dev);
> > >         imx8mq_mipi_csi_release_icc(pdev);
> 

Re: [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function

[Martin Kepplinger]

Am Sonntag, dem 12.03.2023 um 15:04 +0100 schrieb Martin Kepplinger:
> Am Sonntag, dem 12.03.2023 um 15:37 +0200 schrieb Laurent Pinchart:
> > Hi Martin,
> > 
> > Thank you for the patch.
> > 
> > On Tue, Mar 07, 2023 at 04:00:47PM +0100, Martin Kepplinger wrote:
> > > Clean up the driver a bit by inlining the
> > > imx8mq_mipi_csi_system_enable()
> > > function to the callsites and removing the hs_settle variable
> > > from
> > > the
> > > driver state.
> > > 
> > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Could I volunteer you to also drop the struct csi_state state field
> > ?
> > :-)
> 
> sure :) it can become at least a bit more tricky than this patch.
> I'll
> take the time after this is merged.
> 
> thanks for the fast reviewing

Laurent, are these 2 patches queued up somewhere? I'm used to waiting
until they are part of a tree that is part of linux-next before sending
something new. Does that make sense?

thanks,
                      martin

Re: [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function

[Laurent Pinchart]

Hi Martin,

On Sat, Mar 25, 2023 at 10:59:47AM +0100, Martin Kepplinger wrote:
> Am Sonntag, dem 12.03.2023 um 15:04 +0100 schrieb Martin Kepplinger:
> > Am Sonntag, dem 12.03.2023 um 15:37 +0200 schrieb Laurent Pinchart:
> > > On Tue, Mar 07, 2023 at 04:00:47PM +0100, Martin Kepplinger wrote:
> > > > Clean up the driver a bit by inlining the
> > > > imx8mq_mipi_csi_system_enable()
> > > > function to the callsites and removing the hs_settle variable
> > > > from
> > > > the
> > > > driver state.
> > > > 
> > > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > Could I volunteer you to also drop the struct csi_state state field ?
> > > :-)
> > 
> > sure :) it can become at least a bit more tricky than this patch. I'll
> > take the time after this is merged.
> > 
> > thanks for the fast reviewing
> 
> Laurent, are these 2 patches queued up somewhere? I'm used to waiting
> until they are part of a tree that is part of linux-next before sending
> something new. Does that make sense?

I've just sent a pull request to Mauro for v6.4. You can find the patch
in my tree at

	git://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git media-imx-next-20230325

-- 
Regards,

Laurent Pinchart