| Message ID | 1341520728-2707-8-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Laurent, Few comments below. 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@ideasonboard.com> > --- > 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? > > 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. Kind regards, David Cohen -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); 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,
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@ideasonboard.com> --- drivers/media/video/soc_camera.c | 9 +++------ 1 files changed, 3 insertions(+), 6 deletions(-)