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/[email protected]/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
Simplify the driver by using the V4L2 subdev active state API to store
the active format.
Signed-off-by: Martin Kepplinger <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
>
> @@ -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
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 <[email protected]>
> > ---
> > 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 <[email protected]>
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
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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
>
> 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);
>