From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F2884298258; Tue, 8 Jul 2025 07:41:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.175.65.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1751960471; cv=none; b=uskcDtA5fu0DoBaYRSfpPcE2p5uvsbSvWSvP7FUxOWedzQoqHniRgEeGzC+SNXPHjaw/ZK9meMsyFiaLPQiDSxWyvuwA2Xpux3Z3jghNVH+Yw1Cg+XLLuhn4MZi3CL0D6+pPpAbJayFItQyf01ZV/Asl4r7jyZrdEnGqSyyFHFM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1751960471; c=relaxed/simple; bh=Y5aa14T6jauvypMBTXb+fnAe1LTjxhMSNTyKV7M0rTY=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=AfJL9KWA7saGKkdmpCmQ0cd5bXO543i37RLhK5DfxyfbaOOXjRjA0b7i2l/E+XEVhxZERoYXWhzBtfuB5Y8kj5QUkm2h5nW4WLTSIDmXD9kLxuhMQh/raMXcppLFUr3KzoAaO/OMeW0TxjFuUcosjc3Wdz/ex8VJVn+KQ+oR3tc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com; spf=pass smtp.mailfrom=intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=UfO7kD62; arc=none smtp.client-ip=198.175.65.18 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="UfO7kD62" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1751960470; x=1783496470; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=Y5aa14T6jauvypMBTXb+fnAe1LTjxhMSNTyKV7M0rTY=; b=UfO7kD62MyrupeBt25wSwbmjT24sSXK7uY4PpintGWQQwlBQjn3oVK5A PMjIkxcZY/9O1BJenKNOMYSKgtrxKLSDS9BdB9GwP5F+uXh7BhtPRQik1 UTIPjMlS9NeZVLuIVQ3qEoLLkQlUUOrBXNtJihfDBPdnRzd4QvgCCKCb0 d1B5RikXGQulXMlnJHvx9N57yDce5Zt4AQIR5zgX65trWnkQjGqFOfKpJ hhJlHRvNKt+wELimYJLlaWov4+JvzKNUYmdFMBFlojdiF/y7QlNZpve8X tYli/8grPh0d0JHEftQy7HdmPtF0bmfNxeAw0pQNDaq1YowVV+m+OqNwj A==; X-CSE-ConnectionGUID: dbX6t0waS1Gouikzkv8BgQ== X-CSE-MsgGUID: FHec6TZKQDqY9goUy03+hw== X-IronPort-AV: E=McAfee;i="6800,10657,11487"; a="54317234" X-IronPort-AV: E=Sophos;i="6.16,296,1744095600"; d="scan'208";a="54317234" Received: from fmviesa005.fm.intel.com ([10.60.135.145]) by orvoesa110.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Jul 2025 00:41:09 -0700 X-CSE-ConnectionGUID: scmlibf3StyPr3xtPtjMXA== X-CSE-MsgGUID: B1uChcq+Q1uEgaYjrQ+O0Q== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.16,296,1744095600"; d="scan'208";a="159698073" Received: from unknown (HELO [10.238.224.209]) ([10.238.224.209]) by fmviesa005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Jul 2025 00:41:03 -0700 Message-ID: Date: Tue, 8 Jul 2025 15:41:00 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/2] media: i2c: add ov2735 image sensor driver To: Hardevsinh Palaniya , Andy Shevchenko Cc: "sakari.ailus@linux.intel.com" , Himanshu Bhavani , Mauro Carvalho Chehab , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Hans Verkuil , =?UTF-8?Q?Andr=C3=A9_Apitzsch?= , Bryan O'Donoghue , Hans de Goede , Tarang Raval , Jingjing Xiong , Sylvain Petinot , Benjamin Mugnier , Matthias Fend , Arnd Bergmann , Heimir Thor Sverrisson , "linux-media@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <20250707150118.20536-1-hardevsinh.palaniya@siliconsignals.io> <20250707150118.20536-3-hardevsinh.palaniya@siliconsignals.io> Content-Language: en-US From: "Yan, Dongcheng" In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi Hardevsinh, On 7/8/2025 3:04 PM, Hardevsinh Palaniya wrote: > Hi Andy, > > Thanks for the review. > > I have corrected the code, and all of your comments have now been addressed. > > However, I have a question about one of the comments. Please see below. > >> On Mon, Jul 07, 2025 at 08:31:06PM +0530, Hardevsinh Palaniya wrote: >>> Add a v4l2 subdevice driver for the Omnivision OV2735 sensor. >>> >>> The Omnivision OV2735 is a 1/2.7-Inch CMOS image sensor with an >>> active array size of 1920 x 1080. >>> >>> - Manual exposure an gain control support >>> - vblank/hblank control support >>> - Supported resolution: 1920 x 1080 @ 30fps (SGRBG10) >> >> ... >> >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >> >> More stuff is in use than just these headers provide. >> E.g., >> >> + array_size.h >> + container_of.h >> + gpio/consumer.h >> + types.h >> >> And so on... Also in some cases the forward declarations are enough to have. >> >> .,, >> >>> +#define OV2735_LINK_FREQ_420MHZ 420000000 >> >> HZ_PER_MHZ ? >> >> ... >> >>> +#define OV2735_PIXEL_RATE 168000000 >> >> What's the unit? >> >> ... >> >>> +static const s64 link_freq_menu_items[] = { >>> + OV2735_LINK_FREQ_420MHZ >> >> Keep the trailing comma like you have done in other cases. >> >>> +}; >> >> ... >> >>> +static int ov2735_enable_test_pattern(struct ov2735 *ov2735, u32 pattern) >>> +{ >>> + int ret; >>> + u64 val; >>> + >>> + ret = cci_read(ov2735->cci, OV2735_REG_TEST_PATTERN, &val, NULL); >>> + if (ret) >>> + return ret; >>> + >>> + switch (pattern) { >>> + case 0: >>> + val &= ~OV2735_TEST_PATTERN_ENABLE; >>> + break; >>> + case 1: >>> + val |= OV2735_TEST_PATTERN_ENABLE; >>> + break; >>> + } >> >>> + ret = cci_write(ov2735->cci, OV2735_REG_TEST_PATTERN, val, NULL); >>> + if (ret) >>> + return ret; >>> + >>> + return 0; >> >> Is this the required style? Because these 5 LoCs is just a simple >> >> return cci_write(ov2735->cci, OV2735_REG_TEST_PATTERN, val, NULL); >> >>> +} >> >> ... >> >>> +static int ov2735_set_ctrl(struct v4l2_ctrl *ctrl) >>> +{ >>> + struct ov2735 *ov2735 = container_of(ctrl->handler, struct ov2735, >>> + handler); >>> + const struct ov2735_mode *mode; >>> + struct v4l2_mbus_framefmt *fmt; >>> + struct v4l2_subdev_state *state; >> >>> + int vts; >> >> Can be negative? >> >>> + int ret = 0; >> >> How is this assignment useful? >> >>> + state = v4l2_subdev_get_locked_active_state(&ov2735->sd); >>> + fmt = v4l2_subdev_state_get_format(state, 0); >>> + >>> + mode = v4l2_find_nearest_size(supported_modes, >>> + ARRAY_SIZE(supported_modes), >>> + width, height, >>> + fmt->width, fmt->height); >>> + >>> + if (ctrl->id == V4L2_CID_VBLANK) { >>> + /* Honour the VBLANK limits when setting exposure. */ >>> + s64 max = mode->height + ctrl->val - 4; >>> + >>> + ret = __v4l2_ctrl_modify_range(ov2735->exposure, >>> + ov2735->exposure->minimum, max, >>> + ov2735->exposure->step, >>> + ov2735->exposure->default_value); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + /* >>> + * Applying V4L2 control value only happens >>> + * when power is up for streaming >> >> Multi-line comments shouldn't neglect punctuation. >> >>> + */ >>> + if (pm_runtime_get_if_in_use(ov2735->dev) == 0) >>> + return 0; >>> + >>> + ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, NULL); >>> + >>> + switch (ctrl->id) { >>> + case V4L2_CID_EXPOSURE: >>> + ret |= cci_write(ov2735->cci, OV2735_REG_LONG_EXPOSURE, ctrl->val, NULL); >>> + break; >>> + case V4L2_CID_ANALOGUE_GAIN: >>> + ret |= cci_write(ov2735->cci, OV2735_REG_ANALOG_GAIN, ctrl->val, NULL); >>> + break; >>> + case V4L2_CID_HBLANK: >>> + ret |= cci_write(ov2735->cci, OV2735_REG_HBLANK, ctrl->val, NULL); >>> + break; >>> + case V4L2_CID_VBLANK: >>> + vts = ctrl->val + mode->height; >>> + ret |= cci_write(ov2735->cci, OV2735_REG_FRAME_EXP_SEPERATE_EN, >>> + OV2735_FRAME_EXP_SEPERATE_EN, NULL); >>> + ret |= cci_write(ov2735->cci, OV2735_REG_FRAME_LENGTH, vts, NULL); >>> + break; >>> + case V4L2_CID_TEST_PATTERN: >>> + ret = ov2735_enable_test_pattern(ov2735, ctrl->val); >>> + break; >>> + default: >>> + dev_err(ov2735->dev, "ctrl(id:0x%x, val:0x%x) is not handled\n", >>> + ctrl->id, ctrl->val); >>> + break; >>> + } >>> + ret |= cci_write(ov2735->cci, OV2735_REG_FRAME_SYNC, 0x01, NULL); >>> + >>> + pm_runtime_put(ov2735->dev); >>> + >>> + return ret; >>> +} >> >> ... >> >>> +static int ov2735_init_controls(struct ov2735 *ov2735) >>> +{ >>> + struct v4l2_ctrl_handler *ctrl_hdlr; >>> + struct v4l2_fwnode_device_properties props; >>> + const struct ov2735_mode *mode = &supported_modes[0]; >>> + u64 hblank_def, vblank_def, exp_max; >>> + int ret; >>> + >>> + ctrl_hdlr = &ov2735->handler; >>> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 7); >>> + if (ret) >>> + return ret; >>> + >>> + ov2735->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops, V4L2_CID_PIXEL_RATE, >>> + 0, OV2735_PIXEL_RATE, 1, OV2735_PIXEL_RATE); >> >> Besides it's too long, it has trailing space. >> >>> + >>> + ov2735->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &ov2735_ctrl_ops, >>> + V4L2_CID_LINK_FREQ, >>> + 0, 0, link_freq_menu_items); >>> + if (ov2735->link_freq) >>> + ov2735->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>> + >>> + hblank_def = mode->hts_def - mode->width; >>> + ov2735->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops, V4L2_CID_HBLANK, >>> + hblank_def, hblank_def, 1, hblank_def); >>> + >>> + vblank_def = mode->vts_def - mode->height; >>> + ov2735->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops, >>> + V4L2_CID_VBLANK, vblank_def, >>> + OV2735_VTS_MAX - mode->height, >>> + 1, vblank_def); >> >> It's weird indentation. >> >>> + >>> + exp_max = mode->vts_def - 4; >>> + ov2735->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops, >>> + V4L2_CID_EXPOSURE, OV2735_EXPOSURE_MIN, >>> + exp_max, OV2735_EXPOSURE_STEP, mode->exp_def); >>> + >>> + ov2735->gain = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops, >>> + V4L2_CID_ANALOGUE_GAIN, ANALOG_GAIN_MIN, >>> + ANALOG_GAIN_MAX, ANALOG_GAIN_STEP, >>> + ANALOG_GAIN_DEFAULT); >> >> Ditto. >> >>> + ov2735->test_pattern = v4l2_ctrl_new_std_menu_items(ctrl_hdlr, >>> + &ov2735_ctrl_ops, V4L2_CID_TEST_PATTERN, >>> + ARRAY_SIZE(ov2735_test_pattern_menu) - 1, >>> + 0, 0, ov2735_test_pattern_menu); >> >> Ditto. >> >>> + if (ctrl_hdlr->error) { >>> + ret = ctrl_hdlr->error; >>> + dev_err(ov2735->dev, "control init failed (%d)\n", ret); >>> + goto error; >>> + } >>> + >>> + ret = v4l2_fwnode_device_parse(ov2735->dev, &props); >>> + if (ret) >>> + goto error; >>> + >>> + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov2735_ctrl_ops, >>> + &props); >>> + if (ret) >>> + goto error; >>> + >>> + ov2735->sd.ctrl_handler = ctrl_hdlr; >>> + >>> + return 0; >>> +error: >> >> Usual way of naming labels is to explain what is going to happen when goto. >> Moreover it's even inconsistent, the below code use err prefix, but better >> naming. >> >> Here the >> >> err_handler_free: >> >> for example is better. >> >>> + v4l2_ctrl_handler_free(ctrl_hdlr); >>> + >>> + return ret; >>> +} >> >> ... >> >>> +static int ov2735_enable_streams(struct v4l2_subdev *sd, >>> + struct v4l2_subdev_state *state, u32 pad, >>> + u64 streams_mask) >> >> Indentation issue. >> >>> +{ >>> + struct ov2735 *ov2735 = to_ov2735(sd); >>> + const struct ov2735_mode *mode; >>> + const struct ov2735_reglist *reg_list; >>> + const struct v4l2_mbus_framefmt *fmt; >>> + int ret = 0; >> >> Needless assignment. >> >>> + fmt = v4l2_subdev_state_get_format(state, 0); >>> + mode = v4l2_find_nearest_size(supported_modes, >>> + ARRAY_SIZE(supported_modes), >>> + width, height, >>> + fmt->width, fmt->height); >>> + >>> + ret = pm_runtime_resume_and_get(ov2735->dev); >>> + if (ret < 0) >>> + return ret; >>> + >>> + reg_list = &mode->reg_list; >>> + ret = cci_multi_reg_write(ov2735->cci, reg_list->regvals, reg_list->num_regs, NULL); >>> + if (ret) { >>> + dev_err(ov2735->dev, "%s failed to send mfg header\n", __func__); >>> + goto err_rpm_put; >>> + } >>> + >>> + /* Apply customized values from user */ >>> + ret = __v4l2_ctrl_handler_setup(ov2735->sd.ctrl_handler); >>> + if (ret) >>> + goto err_rpm_put; >>> + >>> + /* set stream on register */ >>> + ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, NULL); >>> + ret |= cci_write(ov2735->cci, OV2735_REG_STREAM_CTRL, OV2735_STREAM_ON, NULL); >>> + if (ret) >>> + goto err_rpm_put; >>> + >>> + return 0; >>> + >>> +err_rpm_put: >>> + pm_runtime_put(ov2735->dev); >>> + return ret; >>> +} >> >> ... >> >>> +static int ov2735_disable_streams(struct v4l2_subdev *sd, >>> + struct v4l2_subdev_state *state, u32 pad, >>> + u64 streams_mask) >>> +{ >>> + struct ov2735 *ov2735 = to_ov2735(sd); >>> + int ret = 0; >>> + >>> + /* set stream off register */ >>> + ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, NULL); >>> + ret |= cci_write(ov2735->cci, OV2735_REG_STREAM_CTRL, OV2735_STREAM_OFF, NULL); >> >> Why not using the ret parameter? Same for other similar cases above and beyond. > > I am not sure what you want to suggest here. > > Do I need to check ret like this? > > ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, NULL); > if (ret) { > // error message > } > > ret = cci_write(ov2735->cci, OV2735_REG_STREAM_CTRL, OV2735_STREAM_OFF, NULL); > if (ret) { > // error message > } > cci_write/read has a param named ret, for example: cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, &ret); cci_write(ov2735->cci, OV2735_REG_STREAM_CTRL, OV2735_STREAM_OFF, &ret); then check ret here or return ret. Thanks, Dongcheng> Best Regards, > Hardev