* [PATCH v2 0/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev state
@ 2023-03-07 15:00 Martin Kepplinger
2023-03-07 15:00 ` [PATCH v2 1/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state Martin Kepplinger
2023-03-07 15:00 ` [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function Martin Kepplinger
0 siblings, 2 replies; 10+ messages in thread
From: Martin Kepplinger @ 2023-03-07 15:00 UTC (permalink / raw)
To: laurent.pinchart, slongerbeam, p.zabel, mchehab, gregkh, shawnguo
Cc: kernel, festevam, linux-imx, linux-media, linux-staging,
linux-arm-kernel, linux-kernel, kernel, 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
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v2 1/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state 2023-03-07 15:00 [PATCH v2 0/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev state Martin Kepplinger @ 2023-03-07 15:00 ` Martin Kepplinger 2023-03-12 13:31 ` Laurent Pinchart 2023-03-07 15:00 ` [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function Martin Kepplinger 1 sibling, 1 reply; 10+ messages in thread From: Martin Kepplinger @ 2023-03-07 15:00 UTC (permalink / raw) To: laurent.pinchart, slongerbeam, p.zabel, mchehab, gregkh, shawnguo Cc: kernel, festevam, linux-imx, linux-media, linux-staging, linux-arm-kernel, linux-kernel, kernel, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state 2023-03-07 15:00 ` [PATCH v2 1/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state Martin Kepplinger @ 2023-03-12 13:31 ` Laurent Pinchart 2023-03-12 13:39 ` Laurent Pinchart 0 siblings, 1 reply; 10+ messages in thread From: Laurent Pinchart @ 2023-03-12 13:31 UTC (permalink / raw) To: Martin Kepplinger Cc: slongerbeam, p.zabel, mchehab, gregkh, shawnguo, kernel, festevam, linux-imx, linux-media, linux-staging, linux-arm-kernel, linux-kernel, kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state 2023-03-12 13:31 ` Laurent Pinchart @ 2023-03-12 13:39 ` Laurent Pinchart 2023-03-12 14:05 ` Martin Kepplinger 0 siblings, 1 reply; 10+ messages in thread From: Laurent Pinchart @ 2023-03-12 13:39 UTC (permalink / raw) To: Martin Kepplinger Cc: slongerbeam, p.zabel, mchehab, gregkh, shawnguo, kernel, festevam, linux-imx, linux-media, linux-staging, linux-arm-kernel, linux-kernel, kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state 2023-03-12 13:39 ` Laurent Pinchart @ 2023-03-12 14:05 ` Martin Kepplinger 0 siblings, 0 replies; 10+ messages in thread From: Martin Kepplinger @ 2023-03-12 14:05 UTC (permalink / raw) To: Laurent Pinchart Cc: slongerbeam, p.zabel, mchehab, gregkh, shawnguo, kernel, festevam, linux-imx, linux-media, linux-staging, linux-arm-kernel, linux-kernel, kernel 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); > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function 2023-03-07 15:00 [PATCH v2 0/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev state Martin Kepplinger 2023-03-07 15:00 ` [PATCH v2 1/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state Martin Kepplinger @ 2023-03-07 15:00 ` Martin Kepplinger 2023-03-12 13:37 ` Laurent Pinchart 1 sibling, 1 reply; 10+ messages in thread From: Martin Kepplinger @ 2023-03-07 15:00 UTC (permalink / raw) To: laurent.pinchart, slongerbeam, p.zabel, mchehab, gregkh, shawnguo Cc: kernel, festevam, linux-imx, linux-media, linux-staging, linux-arm-kernel, linux-kernel, kernel, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function 2023-03-07 15:00 ` [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function Martin Kepplinger @ 2023-03-12 13:37 ` Laurent Pinchart 2023-03-12 14:04 ` Martin Kepplinger 0 siblings, 1 reply; 10+ messages in thread From: Laurent Pinchart @ 2023-03-12 13:37 UTC (permalink / raw) To: Martin Kepplinger Cc: slongerbeam, p.zabel, mchehab, gregkh, shawnguo, kernel, festevam, linux-imx, linux-media, linux-staging, linux-arm-kernel, linux-kernel, kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function 2023-03-12 13:37 ` Laurent Pinchart @ 2023-03-12 14:04 ` Martin Kepplinger 2023-03-25 9:59 ` Martin Kepplinger 0 siblings, 1 reply; 10+ messages in thread From: Martin Kepplinger @ 2023-03-12 14:04 UTC (permalink / raw) To: Laurent Pinchart Cc: slongerbeam, p.zabel, mchehab, gregkh, shawnguo, kernel, festevam, linux-imx, linux-media, linux-staging, linux-arm-kernel, linux-kernel, kernel 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); > > } > > > > /* --------------------------------------------------------------- > > -------------- > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function 2023-03-12 14:04 ` Martin Kepplinger @ 2023-03-25 9:59 ` Martin Kepplinger 2023-03-25 18:49 ` Laurent Pinchart 0 siblings, 1 reply; 10+ messages in thread From: Martin Kepplinger @ 2023-03-25 9:59 UTC (permalink / raw) To: Laurent Pinchart Cc: slongerbeam, p.zabel, mchehab, gregkh, shawnguo, kernel, festevam, linux-imx, linux-media, linux-staging, linux-arm-kernel, linux-kernel, kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function 2023-03-25 9:59 ` Martin Kepplinger @ 2023-03-25 18:49 ` Laurent Pinchart 0 siblings, 0 replies; 10+ messages in thread From: Laurent Pinchart @ 2023-03-25 18:49 UTC (permalink / raw) To: Martin Kepplinger Cc: slongerbeam, p.zabel, mchehab, gregkh, shawnguo, kernel, festevam, linux-imx, linux-media, linux-staging, linux-arm-kernel, linux-kernel, kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-03-25 18:49 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-07 15:00 [PATCH v2 0/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev state Martin Kepplinger 2023-03-07 15:00 ` [PATCH v2 1/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state Martin Kepplinger 2023-03-12 13:31 ` Laurent Pinchart 2023-03-12 13:39 ` Laurent Pinchart 2023-03-12 14:05 ` Martin Kepplinger 2023-03-07 15:00 ` [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function Martin Kepplinger 2023-03-12 13:37 ` Laurent Pinchart 2023-03-12 14:04 ` Martin Kepplinger 2023-03-25 9:59 ` Martin Kepplinger 2023-03-25 18:49 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).