From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mga14.intel.com ([143.182.124.37]:14197 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750748Ab2GOWYl (ORCPT ); Sun, 15 Jul 2012 18:24:41 -0400 Message-ID: <50034325.50006@linux.intel.com> Date: Mon, 16 Jul 2012 01:24:37 +0300 From: David Cohen MIME-Version: 1.0 To: Laurent Pinchart CC: Guennadi Liakhovetski , linux-media@vger.kernel.org Subject: Re: [PATCH v2 7/9] soc-camera: Continue the power off sequence if one of the steps fails References: <1341520728-2707-1-git-send-email-laurent.pinchart@ideasonboard.com> <1341520728-2707-8-git-send-email-laurent.pinchart@ideasonboard.com> In-Reply-To: <1341520728-2707-8-git-send-email-laurent.pinchart@ideasonboard.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: 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 > --- > 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