Message ID | 20211223191615.17803-1-p.yadav@ti.com |
---|---|
Headers | show |
Series | CSI2RX support on J721E | expand |
Hi Pratyush, Thank you for the patch. On Fri, Dec 24, 2021 at 12:46:03AM +0530, Pratyush Yadav wrote: > Call media_entity_cleanup() in probe error path and remove to make sure > the media entity is cleaned up properly. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > > Changes in v5: > - New in v5. > > drivers/media/platform/cadence/cdns-csi2rx.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 2a23da6a0b8e..2547903f2e8e 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -471,6 +471,7 @@ static int csi2rx_probe(struct platform_device *pdev) > err_cleanup: > v4l2_async_nf_unregister(&csi2rx->notifier); > v4l2_async_nf_cleanup(&csi2rx->notifier); > + media_entity_cleanup(&csi2rx->subdev.entity); > err_free_priv: > kfree(csi2rx); > return ret; > @@ -483,6 +484,7 @@ static int csi2rx_remove(struct platform_device *pdev) > v4l2_async_nf_unregister(&csi2rx->notifier); > v4l2_async_nf_cleanup(&csi2rx->notifier); > v4l2_async_unregister_subdev(&csi2rx->subdev); > + media_entity_cleanup(&csi2rx->subdev.entity); > kfree(csi2rx); > > return 0;
Hi Pratyush, Thank you for the patch. On Fri, Dec 24, 2021 at 12:46:04AM +0530, Pratyush Yadav wrote: > The format is needed to calculate the link speed for the external DPHY > configuration. It is not right to query the format from the source > subdev. Add get_fmt and set_fmt pad operations so that the format can be > configured and correct bpp be selected. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > Changes in v5: > - Use YUV 1X16 formats instead of 2X8. > - New in v5. > > drivers/media/platform/cadence/cdns-csi2rx.c | 137 +++++++++++++++++++ > 1 file changed, 137 insertions(+) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 2547903f2e8e..4a2a5a9d019b 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -54,6 +54,11 @@ enum csi2rx_pads { > CSI2RX_PAD_MAX, > }; > > +struct csi2rx_fmt { > + u32 code; > + u8 bpp; > +}; > + > struct csi2rx_priv { > struct device *dev; > unsigned int count; > @@ -79,12 +84,43 @@ struct csi2rx_priv { > struct v4l2_subdev subdev; > struct v4l2_async_notifier notifier; > struct media_pad pads[CSI2RX_PAD_MAX]; > + struct v4l2_mbus_framefmt fmt; > > /* Remote source */ > struct v4l2_subdev *source_subdev; > int source_pad; > }; > > +static const struct csi2rx_fmt formats[] = { > + { > + .code = MEDIA_BUS_FMT_YUYV8_1X16, > + .bpp = 16, > + }, > + { > + .code = MEDIA_BUS_FMT_UYVY8_1X16, > + .bpp = 16, > + }, > + { > + .code = MEDIA_BUS_FMT_YVYU8_1X16, > + .bpp = 16, > + }, > + { > + .code = MEDIA_BUS_FMT_VYUY8_1X16, > + .bpp = 16, > + }, > +}; bpp isn't used. Unless you need it in a subsequent patch in the series, you can turn the formats array into a u32 array. > + > +static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(formats); i++) > + if (formats[i].code == code) > + return &formats[i]; > + > + return NULL; > +} > + > static inline > struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev) > { > @@ -236,12 +272,109 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > return ret; > } > > +static struct v4l2_mbus_framefmt * > +csi2rx_get_pad_format(struct csi2rx_priv *csi2rx, > + struct v4l2_subdev_state *state, > + unsigned int pad, u32 which) > +{ > + switch (which) { > + case V4L2_SUBDEV_FORMAT_TRY: > + return v4l2_subdev_get_try_format(&csi2rx->subdev, state, pad); > + case V4L2_SUBDEV_FORMAT_ACTIVE: > + return &csi2rx->fmt; > + default: > + return NULL; > + } > +} > + > +static int csi2rx_get_fmt(struct v4l2_subdev *subdev, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_format *format) > +{ > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > + struct v4l2_mbus_framefmt *framefmt; > + > + mutex_lock(&csi2rx->lock); > + > + framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad, > + format->which); > + mutex_unlock(&csi2rx->lock); > + > + if (!framefmt) > + return -EINVAL; This can't happen, you can drop the check. > + > + format->format = *framefmt; This is the assignment that needs to be protected by the lock. csi2rx_get_pad_format() returns a pointer to the storage, it doesn't modify it. framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad, format->which); mutex_lock(&csi2rx->lock); format->format = *framefmt; mutex_unlock(&csi2rx->lock); Same comments for csi2rx_set_fmt(). > + > + return 0; > +} > + > +static int csi2rx_set_fmt(struct v4l2_subdev *subdev, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_format *format) > +{ > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > + struct v4l2_mbus_framefmt *framefmt; > + const struct csi2rx_fmt *fmt; > + > + /* No transcoding, source and sink formats must match. */ > + if (format->pad != CSI2RX_PAD_SINK) > + return csi2rx_get_fmt(subdev, state, format); > + > + fmt = csi2rx_get_fmt_by_code(format->format.code); > + if (!fmt) > + return -EOPNOTSUPP; This should not return an error, but instead adjust the code: if (!csi2rx_get_fmt_by_code(format->format.code)) format->format.code = formats[0].code; Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + format->format.field = V4L2_FIELD_NONE; > + > + mutex_lock(&csi2rx->lock); > + framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad, > + format->which); > + if (!framefmt) { > + mutex_unlock(&csi2rx->lock); > + return -EINVAL; > + } > + > + *framefmt = format->format; > + mutex_unlock(&csi2rx->lock); > + > + return 0; > +} > + > +static int csi2rx_init_cfg(struct v4l2_subdev *subdev, > + struct v4l2_subdev_state *state) > +{ > + struct v4l2_subdev_format format = { > + .which = state ? V4L2_SUBDEV_FORMAT_TRY > + : V4L2_SUBDEV_FORMAT_ACTIVE, > + .pad = CSI2RX_PAD_SINK, > + .format = { > + .width = 640, > + .height = 480, > + .code = MEDIA_BUS_FMT_UYVY8_1X16, > + .field = V4L2_FIELD_NONE, > + .colorspace = V4L2_COLORSPACE_SRGB, > + .ycbcr_enc = V4L2_YCBCR_ENC_601, > + .quantization = V4L2_QUANTIZATION_LIM_RANGE, > + .xfer_func = V4L2_XFER_FUNC_SRGB, > + }, > + }; > + > + return csi2rx_set_fmt(subdev, state, &format); > +} > + > +static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = { > + .get_fmt = csi2rx_get_fmt, > + .set_fmt = csi2rx_set_fmt, > + .init_cfg = csi2rx_init_cfg, > +}; > + > static const struct v4l2_subdev_video_ops csi2rx_video_ops = { > .s_stream = csi2rx_s_stream, > }; > > static const struct v4l2_subdev_ops csi2rx_subdev_ops = { > .video = &csi2rx_video_ops, > + .pad = &csi2rx_pad_ops, > }; > > static int csi2rx_async_bound(struct v4l2_async_notifier *notifier, > @@ -457,6 +590,10 @@ static int csi2rx_probe(struct platform_device *pdev) > if (ret) > goto err_cleanup; > > + ret = csi2rx_init_cfg(&csi2rx->subdev, NULL); > + if (ret) > + goto err_cleanup; > + > ret = v4l2_async_register_subdev(&csi2rx->subdev); > if (ret < 0) > goto err_cleanup;
On Fri, Dec 24, 2021 at 12:46:05AM +0530, Pratyush Yadav wrote: > Some platforms like TI's J721E can have the CSI2RX paired with an > external DPHY. Add support to enable and configure the DPHY using the > generic PHY framework. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > Changes in v5: > - Only error out when phy_pm_runtime_get_sync() returns a negative > value. A positive value can be returned if the phy was already > resumed. > - Do not query the source subdev for format. Use the newly added > internal format instead. > > Changes in v4: > - Drop the call to set PHY submode. It is now being done via compatible > on the DPHY side. > > Changes in v3: > - Use v4l2_get_link_freq() to calculate pixel clock. > > Changes in v2: > - Use phy_pm_runtime_get_sync() and phy_pm_runtime_put() before making > calls to set PHY mode, etc. to make sure it is ready. > > drivers/media/platform/cadence/cdns-csi2rx.c | 98 ++++++++++++++++++-- > 1 file changed, 88 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 4a2a5a9d019b..afd4a0da8235 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -30,6 +30,12 @@ > #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 + (llane) * 4)) > #define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8) > > +#define CSI2RX_DPHY_LANE_CTRL_REG 0x40 > +#define CSI2RX_DPHY_CL_RST BIT(16) > +#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12) > +#define CSI2RX_DPHY_CL_EN BIT(4) > +#define CSI2RX_DPHY_DL_EN(i) BIT(i) > + > #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100) > > #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000) > @@ -137,6 +143,57 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx) > writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG); > } > > +static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx) > +{ > + union phy_configure_opts opts = { }; > + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; > + const struct csi2rx_fmt *fmt; > + s64 pixel_clock; > + int ret; > + u8 bpp; > + bool got_pm = true; > + > + fmt = csi2rx_get_fmt_by_code(csi2rx->fmt.code); > + bpp = fmt->bpp; > + > + /* > + * Do not divide by the number of lanes here. That will be done by > + * phy_mipi_dphy_get_default_config(). > + */ > + pixel_clock = v4l2_get_link_freq(csi2rx->source_subdev->ctrl_handler, > + 1, 2); > + if (pixel_clock < 0) > + return pixel_clock; > + > + ret = phy_mipi_dphy_get_default_config(pixel_clock, bpp, You could use fmt->bpp here and drop the bpp variable. > + csi2rx->num_lanes, cfg); > + if (ret) > + return ret; > + > + ret = phy_pm_runtime_get_sync(csi2rx->dphy); > + if (ret == -ENOTSUPP) > + got_pm = false; phy_pm_runtime_put() returns -ENOTSUPP when runtime PM isn't enabled, without calling pm_runtime_put(). I think you could write here ret = phy_pm_runtime_get_sync(csi2rx->dphy); if (ret < 0 && ret != -ENOTSUPP) return ret; then drop the got_pm variable, and call phy_pm_runtime_put() unconditionally below. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + else if (ret < 0) > + return ret; > + > + ret = phy_power_on(csi2rx->dphy); > + if (ret) > + goto out; > + > + ret = phy_configure(csi2rx->dphy, &opts); > + if (ret) { > + /* Can't do anything if it fails. Ignore the return value. */ > + phy_power_off(csi2rx->dphy); > + goto out; > + } > + > +out: > + if (got_pm) > + phy_pm_runtime_put(csi2rx->dphy); > + > + return ret; > +} > + > static int csi2rx_start(struct csi2rx_priv *csi2rx) > { > unsigned int i; > @@ -175,6 +232,17 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > if (ret) > goto err_disable_pclk; > > + /* Enable DPHY clk and data lanes. */ > + if (csi2rx->dphy) { > + reg = CSI2RX_DPHY_CL_EN | CSI2RX_DPHY_CL_RST; > + for (i = 0; i < csi2rx->num_lanes; i++) { > + reg |= CSI2RX_DPHY_DL_EN(csi2rx->lanes[i] - 1); > + reg |= CSI2RX_DPHY_DL_RST(csi2rx->lanes[i] - 1); > + } > + > + writel(reg, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > + } > + > /* > * Create a static mapping between the CSI virtual channels > * and the output stream. > @@ -205,10 +273,21 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > if (ret) > goto err_disable_pixclk; > > + if (csi2rx->dphy) { > + ret = csi2rx_configure_external_dphy(csi2rx); > + if (ret) { > + dev_err(csi2rx->dev, > + "Failed to configure external DPHY: %d\n", ret); > + goto err_disable_sysclk; > + } > + } > + > clk_disable_unprepare(csi2rx->p_clk); > > return 0; > > +err_disable_sysclk: > + clk_disable_unprepare(csi2rx->sys_clk); > err_disable_pixclk: > for (; i > 0; i--) > clk_disable_unprepare(csi2rx->pixel_clk[i - 1]); > @@ -236,6 +315,13 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx) > > if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false)) > dev_warn(csi2rx->dev, "Couldn't disable our subdev\n"); > + > + if (csi2rx->dphy) { > + writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > + > + if (phy_power_off(csi2rx->dphy)) > + dev_warn(csi2rx->dev, "Couldn't power off DPHY\n"); > + } > } > > static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > @@ -438,15 +524,6 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > return PTR_ERR(csi2rx->dphy); > } > > - /* > - * FIXME: Once we'll have external D-PHY support, the check > - * will need to be removed. > - */ > - if (csi2rx->dphy) { > - dev_err(&pdev->dev, "External D-PHY not supported yet\n"); > - return -EINVAL; > - } > - > ret = clk_prepare_enable(csi2rx->p_clk); > if (ret) { > dev_err(&pdev->dev, "Couldn't prepare and enable P clock\n"); > @@ -476,7 +553,7 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > * FIXME: Once we'll have internal D-PHY support, the check > * will need to be removed. > */ > - if (csi2rx->has_internal_dphy) { > + if (!csi2rx->dphy && csi2rx->has_internal_dphy) { > dev_err(&pdev->dev, "Internal D-PHY not supported yet\n"); > return -EINVAL; > } > @@ -601,6 +678,7 @@ static int csi2rx_probe(struct platform_device *pdev) > dev_info(&pdev->dev, > "Probed CSI2RX with %u/%u lanes, %u streams, %s D-PHY\n", > csi2rx->num_lanes, csi2rx->max_lanes, csi2rx->max_streams, > + csi2rx->dphy ? "external" : > csi2rx->has_internal_dphy ? "internal" : "no"); > > return 0;
Hi Pratyush, Thank you for the patch. On Fri, Dec 24, 2021 at 12:46:10AM +0530, Pratyush Yadav wrote: > Add media link validation to make sure incorrectly configured pipelines > are caught. Since there is no support for transcoding, rely on the > default link validation function to make sure formats across source and > sink pads are the same. I think the commit message is a bit misleading. The default link validation function ensures that formats on the source and sink sides of a link match. It doesn't compare the formats on the sink and source pads of the subdev. Whether the subdev can transcode or not is thus not relevant here. > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > Changes in v5: > - New in v5. > > drivers/media/platform/cadence/cdns-csi2rx.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 53659776a906..119c7540c75a 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -492,6 +492,10 @@ static const struct v4l2_subdev_ops csi2rx_subdev_ops = { > .pad = &csi2rx_pad_ops, > }; > > +static struct media_entity_operations csi2rx_media_ops = { static const Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + .link_validate = v4l2_subdev_link_validate, > +}; > + > static int csi2rx_async_bound(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *s_subdev, > struct v4l2_async_subdev *asd) > @@ -691,6 +695,7 @@ static int csi2rx_probe(struct platform_device *pdev) > for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++) > csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE; > csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + csi2rx->subdev.entity.ops = &csi2rx_media_ops; > > ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX, > csi2rx->pads);
Hi Laurent, On 30/12/21 06:32AM, Laurent Pinchart wrote: > Hi Pratyush, > > Thank you for the patch. > > On Fri, Dec 24, 2021 at 12:46:04AM +0530, Pratyush Yadav wrote: > > The format is needed to calculate the link speed for the external DPHY > > configuration. It is not right to query the format from the source > > subdev. Add get_fmt and set_fmt pad operations so that the format can be > > configured and correct bpp be selected. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > --- > > > > Changes in v5: > > - Use YUV 1X16 formats instead of 2X8. > > - New in v5. > > > > drivers/media/platform/cadence/cdns-csi2rx.c | 137 +++++++++++++++++++ > > 1 file changed, 137 insertions(+) > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > index 2547903f2e8e..4a2a5a9d019b 100644 > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > @@ -54,6 +54,11 @@ enum csi2rx_pads { > > CSI2RX_PAD_MAX, > > }; > > > > +struct csi2rx_fmt { > > + u32 code; > > + u8 bpp; > > +}; > > + > > struct csi2rx_priv { > > struct device *dev; > > unsigned int count; > > @@ -79,12 +84,43 @@ struct csi2rx_priv { > > struct v4l2_subdev subdev; > > struct v4l2_async_notifier notifier; > > struct media_pad pads[CSI2RX_PAD_MAX]; > > + struct v4l2_mbus_framefmt fmt; > > > > /* Remote source */ > > struct v4l2_subdev *source_subdev; > > int source_pad; > > }; > > > > +static const struct csi2rx_fmt formats[] = { > > + { > > + .code = MEDIA_BUS_FMT_YUYV8_1X16, > > + .bpp = 16, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_UYVY8_1X16, > > + .bpp = 16, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_YVYU8_1X16, > > + .bpp = 16, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_VYUY8_1X16, > > + .bpp = 16, > > + }, > > +}; > > bpp isn't used. Unless you need it in a subsequent patch in the series, > you can turn the formats array into a u32 array. It is used in the next patch for calculating DPHY parameters. > > > + > > +static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(formats); i++) > > + if (formats[i].code == code) > > + return &formats[i]; > > + > > + return NULL; > > +} > > + > > static inline > > struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev) > > { > > @@ -236,12 +272,109 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > > return ret; > > } > > > > +static struct v4l2_mbus_framefmt * > > +csi2rx_get_pad_format(struct csi2rx_priv *csi2rx, > > + struct v4l2_subdev_state *state, > > + unsigned int pad, u32 which) > > +{ > > + switch (which) { > > + case V4L2_SUBDEV_FORMAT_TRY: > > + return v4l2_subdev_get_try_format(&csi2rx->subdev, state, pad); > > + case V4L2_SUBDEV_FORMAT_ACTIVE: > > + return &csi2rx->fmt; > > + default: > > + return NULL; > > + } > > +} > > + > > +static int csi2rx_get_fmt(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_state *state, > > + struct v4l2_subdev_format *format) > > +{ > > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > > + struct v4l2_mbus_framefmt *framefmt; > > + > > + mutex_lock(&csi2rx->lock); > > + > > + framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad, > > + format->which); > > + mutex_unlock(&csi2rx->lock); > > + > > + if (!framefmt) > > + return -EINVAL; > > This can't happen, you can drop the check. The function returns NULL when format->which is neither V4L2_SUBDEV_FORMAT_TRY nor V4L2_SUBDEV_FORMAT_ACTIVE. Does V4L2 core verify that which is always one of TRY or ACTIVE, and nothing else. Does it also guarantee that this would *never* change (like adding a new value for example)? If so, I am fine with dropping this check. Otherwise I would like to keep it. > > > + > > + format->format = *framefmt; > > This is the assignment that needs to be protected by the lock. > csi2rx_get_pad_format() returns a pointer to the storage, it doesn't > modify it. > > framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad, > format->which); > > mutex_lock(&csi2rx->lock); > format->format = *framefmt; > mutex_unlock(&csi2rx->lock); Ah, you're right. I feel very stupid now ;-) > > Same comments for csi2rx_set_fmt(). The assignment is csi2rx_set_fmt() is done under the lock. But I should move the lock to after csi2rx_get_pad_format() is called so we only hold the lock for as little as possible: framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad, format->which); if (!framefmt) { mutex_unlock(&csi2rx->lock); return -EINVAL; } mutex_lock(&csi2rx->lock); *framefmt = format->format; mutex_unlock(&csi2rx->lock); > > > + > > + return 0; > > +} > > + > > +static int csi2rx_set_fmt(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_state *state, > > + struct v4l2_subdev_format *format) > > +{ > > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > > + struct v4l2_mbus_framefmt *framefmt; > > + const struct csi2rx_fmt *fmt; > > + > > + /* No transcoding, source and sink formats must match. */ > > + if (format->pad != CSI2RX_PAD_SINK) > > + return csi2rx_get_fmt(subdev, state, format); > > + > > + fmt = csi2rx_get_fmt_by_code(format->format.code); > > + if (!fmt) > > + return -EOPNOTSUPP; > > This should not return an error, but instead adjust the code: > > if (!csi2rx_get_fmt_by_code(format->format.code)) > format->format.code = formats[0].code; Ok. I figured it would be better to fail loudly if an unsupported format is requested. But if this behavior is preferred that is also fine. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks.