| Message ID | 20230307150047.1486186-2-martin.kepplinger@puri.sm (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: imx8mq-mipi-csi2: Use V4L2 subdev state | expand |
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);
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);
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); >
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);
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(-)