| Message ID | 20210330173348.30135-10-p.yadav@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | CSI2RX support on J721E | expand |
Hi Pratyush, Thank you for the patch. On Tue, Mar 30, 2021 at 11:03:41PM +0530, Pratyush Yadav wrote: > The subdevice power needs to be turned on before the stream is started. > Otherwise it might not be in the proper state to stream the data. Turn > it off when stopping the stream. The .s_power() operation is deprecated. Subdev drivers should control power internally in their .s_stream() operation, and they should use runtime PM to do so (this will allow usage of runtime PM autosuspend, to avoid expensive power off/on cycles when stopping and restarting video capture). > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/media/platform/cadence/cdns-csi2rx.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 7d1ac51e0698..3385e1bc213e 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -256,6 +256,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > > writel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG); > > + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, true); > + if (ret && ret != -ENOIOCTLCMD) > + goto err_disable_pclk; > + > ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true); > if (ret) > goto err_disable_pclk; > @@ -358,6 +362,10 @@ 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"); > > + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, false); > + if (ret && ret != -ENOIOCTLCMD) > + dev_warn(csi2rx->dev, "Couldn't power off subdev\n"); > + > if (csi2rx->dphy) { > writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); >
On 02/04/21 01:55PM, Laurent Pinchart wrote: > Hi Pratyush, > > Thank you for the patch. > > On Tue, Mar 30, 2021 at 11:03:41PM +0530, Pratyush Yadav wrote: > > The subdevice power needs to be turned on before the stream is started. > > Otherwise it might not be in the proper state to stream the data. Turn > > it off when stopping the stream. > > The .s_power() operation is deprecated. Subdev drivers should control > power internally in their .s_stream() operation, and they should use > runtime PM to do so (this will allow usage of runtime PM autosuspend, to > avoid expensive power off/on cycles when stopping and restarting video > capture). The s_power documentation in v4l2-subdev.h does not mention that it is depreciated. A documentation update is in order. I will send a separate patch to do it. I tested this with OV5640. Not doing an s_power() operation before s_stream() does not work. The application freezes forever waiting for the first frame. So the OV5640 driver needs to be updated to use runtime PM to do this, right? > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/media/platform/cadence/cdns-csi2rx.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > index 7d1ac51e0698..3385e1bc213e 100644 > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > @@ -256,6 +256,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > > > > writel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG); > > > > + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, true); > > + if (ret && ret != -ENOIOCTLCMD) > > + goto err_disable_pclk; > > + > > ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true); > > if (ret) > > goto err_disable_pclk; > > @@ -358,6 +362,10 @@ 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"); > > > > + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, false); > > + if (ret && ret != -ENOIOCTLCMD) > > + dev_warn(csi2rx->dev, "Couldn't power off subdev\n"); > > + > > if (csi2rx->dphy) { > > writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > > > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c index 7d1ac51e0698..3385e1bc213e 100644 --- a/drivers/media/platform/cadence/cdns-csi2rx.c +++ b/drivers/media/platform/cadence/cdns-csi2rx.c @@ -256,6 +256,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) writel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG); + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, true); + if (ret && ret != -ENOIOCTLCMD) + goto err_disable_pclk; + ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true); if (ret) goto err_disable_pclk; @@ -358,6 +362,10 @@ 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"); + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, false); + if (ret && ret != -ENOIOCTLCMD) + dev_warn(csi2rx->dev, "Couldn't power off subdev\n"); + if (csi2rx->dphy) { writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG);
The subdevice power needs to be turned on before the stream is started. Otherwise it might not be in the proper state to stream the data. Turn it off when stopping the stream. Signed-off-by: Pratyush Yadav <p.yadav@ti.com> --- drivers/media/platform/cadence/cdns-csi2rx.c | 8 ++++++++ 1 file changed, 8 insertions(+)