diff mbox series

[09/16] media: cadence: csi2rx: Turn subdev power on before starting stream

Message ID 20210330173348.30135-10-p.yadav@ti.com
State Superseded
Headers show
Series CSI2RX support on J721E | expand

Commit Message

Pratyush Yadav March 30, 2021, 5:33 p.m. UTC
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(+)

Comments

Laurent Pinchart April 2, 2021, 10:55 a.m. UTC | #1
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);
>
Pratyush Yadav April 6, 2021, 5:53 p.m. UTC | #2
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 mbox series

Patch

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);