| Message ID | 20230307121729.1419120-1-martin.kepplinger@puri.sm (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state | expand |
Hi Martin, Thank you for the patch. On Tue, Mar 07, 2023 at 01:17:29PM +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 | 108 +++++++------------ > 1 file changed, 37 insertions(+), 71 deletions(-) > > diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c b/drivers/staging/media/imx/imx8mq-mipi-csi2.c > index c0d0bf770096..066409782058 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 */ The hs_settle value is computed in imx8mq_mipi_csi_calc_hs_settle() and used in imx8mq_mipi_csi_system_enable(), both called from imx8mq_mipi_csi_start_stream(). You could return it from the first function, pass it to the second one, and drop it from the structure. That can be done in a separate patch. I think the state variable could also be dropped, which would allow dropping the lock. > u32 state; > u32 hs_settle; > > @@ -328,10 +326,18 @@ static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state) > 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; > + struct v4l2_subdev_state *sd_state; > > /* Calculate the line rate from the pixel rate. */ > + > + sd_state = v4l2_subdev_lock_and_get_active_state(&state->sd); You need to unlock the state when you're done with it. I would call v4l2_subdev_lock_and_get_active_state() in imx8mq_mipi_csi_start_stream() and unlock it there, and pass the state to imx8mq_mipi_csi_calc_hs_settle(). This will prepare for the future when the V4L2 core will pass the state to the .s_stream() handler. Another candidate for a separate patch would be to rename the state variables to csi, to avoid confusion with the subdev state. > + 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", > @@ -455,29 +461,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 +482,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 +501,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 +521,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 +529,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 +538,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 +547,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 +560,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, > }; > > @@ -821,6 +777,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 +791,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) > @@ -949,6 +913,10 @@ static int imx8mq_mipi_csi_probe(struct platform_device *pdev) > if (ret) > goto mutex; > > + ret = imx8mq_mipi_csi_async_register(state); > + if (ret < 0) > + goto cleanup; > + > /* Enable runtime PM. */ > pm_runtime_enable(dev); > if (!pm_runtime_enabled(dev)) { You should still enable runtime PM after registering the subdev, otherwise someone may get hold of the subdev and call an operation that depending on runtime PM being enabled before you get a chance to enable it. It would also be nice to suspend the device at the end of probe (using pm runtime autosuspend), that's a candidate for another patch. > @@ -957,10 +925,6 @@ static int imx8mq_mipi_csi_probe(struct platform_device *pdev) > goto icc; > } > > - ret = imx8mq_mipi_csi_async_register(state); > - if (ret < 0) > - goto cleanup; > - > return 0; > > cleanup: > @@ -968,6 +932,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 +956,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 Dienstag, dem 07.03.2023 um 15:25 +0200 schrieb Laurent Pinchart: > Hi Martin, > > Thank you for the patch. > > On Tue, Mar 07, 2023 at 01:17:29PM +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 | 108 +++++++-------- > > ---- > > 1 file changed, 37 insertions(+), 71 deletions(-) > > > > diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c > > b/drivers/staging/media/imx/imx8mq-mipi-csi2.c > > index c0d0bf770096..066409782058 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 */ > > The hs_settle value is computed in imx8mq_mipi_csi_calc_hs_settle() > and > used in imx8mq_mipi_csi_system_enable(), both called from > imx8mq_mipi_csi_start_stream(). You could return it from the first > function, pass it to the second one, and drop it from the structure. > That can be done in a separate patch. > > I think the state variable could also be dropped, which would allow > dropping the lock. yes, I plan to do get rid of the driver state and lock later. > > > u32 state; > > u32 hs_settle; > > > > @@ -328,10 +326,18 @@ static int > > imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state) > > 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; > > + struct v4l2_subdev_state *sd_state; > > > > /* Calculate the line rate from the pixel rate. */ > > + > > + sd_state = v4l2_subdev_lock_and_get_active_state(&state- > > >sd); > > You need to unlock the state when you're done with it. I would call > v4l2_subdev_lock_and_get_active_state() in > imx8mq_mipi_csi_start_stream() and unlock it there, and pass the > state > to imx8mq_mipi_csi_calc_hs_settle(). This will prepare for the future > when the V4L2 core will pass the state to the .s_stream() handler. thanks! will do and send a v2 early in this case. > > Another candidate for a separate patch would be to rename the state > variables to csi, to avoid confusion with the subdev state. > > > + 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", > > @@ -455,29 +461,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 +482,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 +501,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 +521,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 +529,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 +538,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 +547,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 +560,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, > > }; > > > > @@ -821,6 +777,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 +791,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) > > @@ -949,6 +913,10 @@ static int imx8mq_mipi_csi_probe(struct > > platform_device *pdev) > > if (ret) > > goto mutex; > > > > + ret = imx8mq_mipi_csi_async_register(state); > > + if (ret < 0) > > + goto cleanup; > > + > > /* Enable runtime PM. */ > > pm_runtime_enable(dev); > > if (!pm_runtime_enabled(dev)) { > > You should still enable runtime PM after registering the subdev, > otherwise someone may get hold of the subdev and call an operation > that > depending on runtime PM being enabled before you get a chance to > enable > it. oh that slipped in, sorry. > > It would also be nice to suspend the device at the end of probe > (using > pm runtime autosuspend), that's a candidate for another patch. > thanks for the quick feedback! martin > > @@ -957,10 +925,6 @@ static int imx8mq_mipi_csi_probe(struct > > platform_device *pdev) > > goto icc; > > } > > > > - ret = imx8mq_mipi_csi_async_register(state); > > - if (ret < 0) > > - goto cleanup; > > - > > return 0; > > > > cleanup: > > @@ -968,6 +932,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 +956,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..066409782058 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; @@ -328,10 +326,18 @@ static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state) 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; + struct v4l2_subdev_state *sd_state; /* Calculate the line rate from the pixel rate. */ + + sd_state = v4l2_subdev_lock_and_get_active_state(&state->sd); + 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", @@ -455,29 +461,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 +482,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 +501,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 +521,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 +529,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 +538,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 +547,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 +560,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, }; @@ -821,6 +777,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 +791,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) @@ -949,6 +913,10 @@ static int imx8mq_mipi_csi_probe(struct platform_device *pdev) if (ret) goto mutex; + ret = imx8mq_mipi_csi_async_register(state); + if (ret < 0) + goto cleanup; + /* Enable runtime PM. */ pm_runtime_enable(dev); if (!pm_runtime_enabled(dev)) { @@ -957,10 +925,6 @@ static int imx8mq_mipi_csi_probe(struct platform_device *pdev) goto icc; } - ret = imx8mq_mipi_csi_async_register(state); - if (ret < 0) - goto cleanup; - return 0; cleanup: @@ -968,6 +932,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 +956,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 | 108 +++++++------------ 1 file changed, 37 insertions(+), 71 deletions(-)