diff mbox series

[v3,8/9] media: i2c: ov5645: Don't return early on failures for s_stream(0)

Message ID 20221026130658.45601-9-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Changes Requested
Delegated to: Sakari Ailus
Headers
Series media: i2c: ov5645 driver enhancements |

Commit Message

Prabhakar Oct. 26, 2022, 1:06 p.m. UTC 
  From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Make sure we dont stop the code flow in case of errors while stopping
the stream and return the error code of the first error case if any.

v4l2-core takes care of warning the user so no need to add a warning
message in the driver.

Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2->v3
* Now propagating the first error code in case of failure.

v1->v2
* New patch
---
 drivers/media/i2c/ov5645.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Marco Felsch Oct. 26, 2022, 5:17 p.m. UTC | #1 
Hi Prabhakar,

thanks for the patch, please see below my comments.

On 22-10-26, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Make sure we dont stop the code flow in case of errors while stopping
> the stream and return the error code of the first error case if any.
> 
> v4l2-core takes care of warning the user so no need to add a warning
> message in the driver.
> 
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2->v3
> * Now propagating the first error code in case of failure.
> 
> v1->v2
> * New patch
> ---
>  drivers/media/i2c/ov5645.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index eea3067ddc8b..5702a55607fc 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -996,17 +996,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  		if (ret < 0)
>  			goto err_rpm_put;
>  	} else {
> +		int stream_off_ret = 0;
> +
>  		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);

If this write failed..

>  		if (ret < 0)
> -			return ret;
> +			stream_off_ret = ret;
>  
>  		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
>  				       OV5645_SYSTEM_CTRL0_STOP);

why should this write be successful?

Regards,
  Marco

> -		if (ret < 0)
> -			return ret;
> +		if (ret < 0 && !stream_off_ret)
> +			stream_off_ret = ret;
>  
>  		pm_runtime_mark_last_busy(ov5645->dev);
>  		pm_runtime_put_autosuspend(ov5645->dev);
> +
> +		if (stream_off_ret)
> +			return stream_off_ret;
>  	}
>  
>  	return 0;
> -- 
> 2.25.1
> 
> 
>
Prabhakar Oct. 26, 2022, 5:26 p.m. UTC | #2 
Hi Marco,

Thank you for the review.

On Wed, Oct 26, 2022 at 6:17 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> Hi Prabhakar,
>
> thanks for the patch, please see below my comments.
>
> On 22-10-26, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Make sure we dont stop the code flow in case of errors while stopping
> > the stream and return the error code of the first error case if any.
> >
> > v4l2-core takes care of warning the user so no need to add a warning
> > message in the driver.
> >
> > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v2->v3
> > * Now propagating the first error code in case of failure.
> >
> > v1->v2
> > * New patch
> > ---
> >  drivers/media/i2c/ov5645.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index eea3067ddc8b..5702a55607fc 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -996,17 +996,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> >               if (ret < 0)
> >                       goto err_rpm_put;
> >       } else {
> > +             int stream_off_ret = 0;
> > +
> >               ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
>
> If this write failed..
>
> >               if (ret < 0)
> > -                     return ret;
> > +                     stream_off_ret = ret;
> >
> >               ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> >                                      OV5645_SYSTEM_CTRL0_STOP);
>
> why should this write be successful?
>
Indeed that will fail unless I have a lucky day ;-)

But it seemed to be an overkill for adding an additional check to see
if the previous write succeeded. Do you think this will create an
issue?

Cheers,
Prabhakar
Marco Felsch Oct. 26, 2022, 5:35 p.m. UTC | #3 
On 22-10-26, Lad, Prabhakar wrote:
> Hi Marco,
> 
> Thank you for the review.
> 
> On Wed, Oct 26, 2022 at 6:17 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
> >
> > Hi Prabhakar,
> >
> > thanks for the patch, please see below my comments.
> >
> > On 22-10-26, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Make sure we dont stop the code flow in case of errors while stopping
> > > the stream and return the error code of the first error case if any.
> > >
> > > v4l2-core takes care of warning the user so no need to add a warning
> > > message in the driver.
> > >
> > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > v2->v3
> > > * Now propagating the first error code in case of failure.
> > >
> > > v1->v2
> > > * New patch
> > > ---
> > >  drivers/media/i2c/ov5645.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > index eea3067ddc8b..5702a55607fc 100644
> > > --- a/drivers/media/i2c/ov5645.c
> > > +++ b/drivers/media/i2c/ov5645.c
> > > @@ -996,17 +996,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> > >               if (ret < 0)
> > >                       goto err_rpm_put;
> > >       } else {
> > > +             int stream_off_ret = 0;
> > > +
> > >               ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> >
> > If this write failed..
> >
> > >               if (ret < 0)
> > > -                     return ret;
> > > +                     stream_off_ret = ret;
> > >
> > >               ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> > >                                      OV5645_SYSTEM_CTRL0_STOP);
> >
> > why should this write be successful?
> >
> Indeed that will fail unless I have a lucky day ;-)
> 
> But it seemed to be an overkill for adding an additional check to see
> if the previous write succeeded. Do you think this will create an
> issue?

Why not just say?

	ret = ov5645_write_reg();
	if (ret < 0)
		goto out;

	...

	out:

	dev_pm_xxx();

	return ret;

Regards,
  Marco
Prabhakar Oct. 26, 2022, 6:26 p.m. UTC | #4 
Hi Marco,

On Wed, Oct 26, 2022 at 6:35 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> On 22-10-26, Lad, Prabhakar wrote:
> > Hi Marco,
> >
> > Thank you for the review.
> >
> > On Wed, Oct 26, 2022 at 6:17 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > thanks for the patch, please see below my comments.
> > >
> > > On 22-10-26, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Make sure we dont stop the code flow in case of errors while stopping
> > > > the stream and return the error code of the first error case if any.
> > > >
> > > > v4l2-core takes care of warning the user so no need to add a warning
> > > > message in the driver.
> > > >
> > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > v2->v3
> > > > * Now propagating the first error code in case of failure.
> > > >
> > > > v1->v2
> > > > * New patch
> > > > ---
> > > >  drivers/media/i2c/ov5645.c | 11 ++++++++---
> > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > > index eea3067ddc8b..5702a55607fc 100644
> > > > --- a/drivers/media/i2c/ov5645.c
> > > > +++ b/drivers/media/i2c/ov5645.c
> > > > @@ -996,17 +996,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> > > >               if (ret < 0)
> > > >                       goto err_rpm_put;
> > > >       } else {
> > > > +             int stream_off_ret = 0;
> > > > +
> > > >               ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> > >
> > > If this write failed..
> > >
> > > >               if (ret < 0)
> > > > -                     return ret;
> > > > +                     stream_off_ret = ret;
> > > >
> > > >               ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> > > >                                      OV5645_SYSTEM_CTRL0_STOP);
> > >
> > > why should this write be successful?
> > >
> > Indeed that will fail unless I have a lucky day ;-)
> >
> > But it seemed to be an overkill for adding an additional check to see
> > if the previous write succeeded. Do you think this will create an
> > issue?
>
> Why not just say?
>
>         ret = ov5645_write_reg();
>         if (ret < 0)
>                 goto out;
>
>         ...
>
>         out:
>
>         dev_pm_xxx();
>
>         return ret;
>
Thanks for the suggestion, I will rework this in the next version.

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index eea3067ddc8b..5702a55607fc 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -996,17 +996,22 @@  static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 		if (ret < 0)
 			goto err_rpm_put;
 	} else {
+		int stream_off_ret = 0;
+
 		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
 		if (ret < 0)
-			return ret;
+			stream_off_ret = ret;
 
 		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
 				       OV5645_SYSTEM_CTRL0_STOP);
-		if (ret < 0)
-			return ret;
+		if (ret < 0 && !stream_off_ret)
+			stream_off_ret = ret;
 
 		pm_runtime_mark_last_busy(ov5645->dev);
 		pm_runtime_put_autosuspend(ov5645->dev);
+
+		if (stream_off_ret)
+			return stream_off_ret;
 	}
 
 	return 0;