Hi David,
Thank you for the review.
On Monday 16 July 2012 01:24:37 David Cohen wrote:
> On 07/05/2012 11:38 PM, Laurent Pinchart wrote:
> > Powering off a device is a "best effort" task: failure to execute one of
> > the steps should not prevent the next steps to be executed. For
> > instance, an I2C communication error when putting the chip in stand-by
> > mode should not prevent the more agressive next step of turning the
> > chip's power supply off.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> >
> > drivers/media/video/soc_camera.c | 9 +++------
> > 1 files changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/video/soc_camera.c
> > b/drivers/media/video/soc_camera.c index 55b981f..bbd518f 100644
> > --- a/drivers/media/video/soc_camera.c
> > +++ b/drivers/media/video/soc_camera.c
> > @@ -89,18 +89,15 @@ static int soc_camera_power_off(struct
> > soc_camera_device *icd,>
> > struct soc_camera_link *icl)
> > {
> > struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > - int ret = v4l2_subdev_call(sd, core, s_power, 0);
> > + int ret;
> >
> > - if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> > - return ret;
> > + v4l2_subdev_call(sd, core, s_power, 0);
>
> Fair enough. I agree we should not prevent power off because of failure
> in this step. But IMO we should not silently bypass it too. How about
> an error message?
I'll add that.
> > if (icl->power) {
> >
> > ret = icl->power(icd->control, 0);
> >
> > - if (ret < 0) {
> > + if (ret < 0)
> >
> > dev_err(icd->pdev,
> >
> > "Platform failed to power-off the camera.\n");
> >
> > - return ret;
> > - }
> >
> > }
> >
> > ret = regulator_bulk_disable(icl->num_regulators,
>
> One more comment. Should this function's return value being based fully
> on last action? If any earlier error happened but this last step is
> fine, IMO we should not return 0.
Good point. What about this (on top of the current patch) ?
diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index bbd518f..7bf21da 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -89,21 +89,30 @@ static int soc_camera_power_off(struct soc_camera_device *icd,
struct soc_camera_link *icl)
{
struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
- int ret;
+ int ret = 0;
+ int err;
- v4l2_subdev_call(sd, core, s_power, 0);
+ err = v4l2_subdev_call(sd, core, s_power, 0);
+ if (err < 0 && err != -ENOIOCTLCMD && err != -ENODEV) {
+ dev_err(icd->pdev, "Subdev failed to power-off the camera.\n");
+ ret = err;
+ }
if (icl->power) {
- ret = icl->power(icd->control, 0);
- if (ret < 0)
+ err = icl->power(icd->control, 0);
+ if (err < 0) {
dev_err(icd->pdev,
"Platform failed to power-off the camera.\n");
+ ret = ret ? : err;
+ }
}
- ret = regulator_bulk_disable(icl->num_regulators,
+ err = regulator_bulk_disable(icl->num_regulators,
icl->regulators);
- if (ret < 0)
+ if (err < 0) {
dev_err(icd->pdev, "Cannot disable regulators\n");
+ ret = ret ? : err;
+ }
return ret;
}
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
