| Message ID | 20221026130658.45601-9-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: ov5645 driver enhancements | expand |
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 > > >
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
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
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 --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;