From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from aer-iport-2.cisco.com ([173.38.203.52]:2684 "EHLO aer-iport-2.cisco.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751516AbaEHIoS (ORCPT ); Thu, 8 May 2014 04:44:18 -0400 Message-ID: <536B43DC.30802@cisco.com> Date: Thu, 08 May 2014 10:44:12 +0200 From: Hans Verkuil MIME-Version: 1.0 To: Laurent Pinchart , linux-media@vger.kernel.org CC: Hans Verkuil Subject: Re: [PATCH] mt9p031: Really disable Black Level Calibration in test pattern mode References: <1399477255-21207-1-git-send-email-laurent.pinchart@ideasonboard.com> In-Reply-To: <1399477255-21207-1-git-send-email-laurent.pinchart@ideasonboard.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Hi Laurent, The patch is correct, but I noticed a pre-existing bug that should be fixed. See below. On 05/07/14 17:40, Laurent Pinchart wrote: > The digital side of the Black Level Calibration (BLC) function must be > disabled when generating a test pattern to avoid artifacts in the image. > The driver disables BLC correctly at the hardware level, but the feature > gets reenabled by v4l2_ctrl_handler_setup() the next time the device is > powered on. > > Fix this by marking the BLC controls as inactive when generating a test > pattern, and ignoring control set requests on inactive controls. > > Signed-off-by: Laurent Pinchart > --- > drivers/media/i2c/mt9p031.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c > index 33daace..9102b23 100644 > --- a/drivers/media/i2c/mt9p031.c > +++ b/drivers/media/i2c/mt9p031.c > @@ -655,6 +655,9 @@ static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl) > u16 data; > int ret; > > + if (ctrl->flags & V4L2_CTRL_FLAG_INACTIVE) > + return 0; > + > switch (ctrl->id) { > case V4L2_CID_EXPOSURE: > ret = mt9p031_write(client, MT9P031_SHUTTER_WIDTH_UPPER, > @@ -709,8 +712,16 @@ static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl) > MT9P031_READ_MODE_2_ROW_MIR, 0); > > case V4L2_CID_TEST_PATTERN: > + /* The digital side of the Black Level Calibration function must > + * be disabled when generating a test pattern to avoid artifacts > + * in the image. Activate (deactivate) the BLC-related controls > + * when the test pattern is enabled (disabled). > + */ > + v4l2_ctrl_activate(mt9p031->blc_auto, ctrl->val == 0); > + v4l2_ctrl_activate(mt9p031->blc_offset, ctrl->val == 0); > + > if (!ctrl->val) { > - /* Restore the black level compensation settings. */ > + /* Restore the BLC settings. */ > if (mt9p031->blc_auto->cur.val != 0) { > ret = mt9p031_s_ctrl(mt9p031->blc_auto); This doesn't do what you expect. What you want is to set the blc_auto control to the current value, but mt9p031_s_ctrl(mt9p031->blc_auto) will set it to the *new* value, which may not be the same. Ditto for doing the same for blc_offset. It's best to just call mt9p031_write directly, rather than going through mt9p031_s_ctrl. Regards, Hans > if (ret < 0) > @@ -735,9 +746,7 @@ static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl) > if (ret < 0) > return ret; > > - /* Disable digital black level compensation when using a test > - * pattern. > - */ > + /* Disable digital BLC when generating a test pattern. */ > ret = mt9p031_set_mode2(mt9p031, MT9P031_READ_MODE_2_ROW_BLC, > 0); > if (ret < 0) >