From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_2 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CA062C433DF for ; Fri, 12 Jun 2020 09:33:57 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 97F982068E for ; Fri, 12 Jun 2020 09:33:57 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="dToYLNF4"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=mediatek.com header.i=@mediatek.com header.b="gOwIYaHe" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 97F982068E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=mediatek.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Date:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=sWfHEEuOaKhxUKEpjN0oABm/1mUWMctOPVwnJnych4g=; b=dToYLNF4kg01t5 aZVFB+IStF4Z6Qy3eQvBPDmojCZto3xTE4u3+VOOglRCqdqcLEVVAGcywnfXnZRlkDTZyCay9Pp1y SiOWd1ZloNDmfg1a9F2zwipICiC1jlV3QoguC2tfRyOXvOuFwmMZ03pKDDV9/xlrGwW0tbOY9izdS SiXTwe0PE5DnvfOFws66/pyQVjzEvNxwRAtjWpLksJO5JNRds0vyyDf9yqvMokwL7YlIdagNyzq1/ pvNrT+bicQw9lmTeBKz9rQXap6cciQe9ofZIaYvO9cNdehRWZqSWtl6e9NNQVRZHm9J31Oxfy5dNE KP33h/iskMFhuSuXnVqg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jjg4K-0001O3-9c; Fri, 12 Jun 2020 09:33:48 +0000 Received: from mailgw02.mediatek.com ([216.200.240.185]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jjg4D-0001MA-Rm; Fri, 12 Jun 2020 09:33:44 +0000 X-UUID: bf7654826a42439ca762d7cbd3071dbf-20200612 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mediatek.com; s=dk; h=Content-Transfer-Encoding:MIME-Version:Content-Type:References:In-Reply-To:Date:CC:To:From:Subject:Message-ID; bh=BaLcuqfyqtgeka1QHj6hfdc3DlQV6DK0+EkHYH3N9E4=; b=gOwIYaHePzXcPux/R9Q9VBLs8dw4CX5n6FAPbwrcuzFvAVLoPCEOpJZN82SkEv1aVe4VyninPOFcF7O64hGq6R24ZcYl8DL0hKBaozEJ8jMa6pSioLHGMU1YIrUrh5/5mcQ9P+mY2cNEfIzXklAu4dxgNt/Fm0SDPVYF5Cl25jU=; X-UUID: bf7654826a42439ca762d7cbd3071dbf-20200612 Received: from mtkcas67.mediatek.inc [(172.29.193.45)] by mailgw02.mediatek.com (envelope-from ) (musrelay.mediatek.com ESMTP with TLS) with ESMTP id 1382197401; Fri, 12 Jun 2020 01:33:25 -0800 Received: from MTKMBS31DR.mediatek.inc (172.27.6.102) by MTKMBS62DR.mediatek.inc (172.29.94.18) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Fri, 12 Jun 2020 02:33:28 -0700 Received: from MTKCAS32.mediatek.inc (172.27.4.184) by MTKMBS31DR.mediatek.inc (172.27.6.102) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Fri, 12 Jun 2020 17:33:24 +0800 Received: from [10.17.3.153] (10.17.3.153) by MTKCAS32.mediatek.inc (172.27.4.170) with Microsoft SMTP Server id 15.0.1497.2 via Frontend Transport; Fri, 12 Jun 2020 17:33:22 +0800 Message-ID: <1591954266.8804.646.camel@mhfsdcap03> Subject: Re: [V8, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver From: Dongchun Zhu To: Tomasz Figa Date: Fri, 12 Jun 2020 17:31:06 +0800 In-Reply-To: <20200610183600.GI201868@chromium.org> References: <20200509080627.23222-1-dongchun.zhu@mediatek.com> <20200509080627.23222-3-dongchun.zhu@mediatek.com> <20200521193204.GA14214@chromium.org> <1590209415.8804.431.camel@mhfsdcap03> <20200610183600.GI201868@chromium.org> X-Mailer: Evolution 3.10.4-0ubuntu2 MIME-Version: 1.0 X-TM-SNTS-SMTP: D560A70DAB39B39E17459FE12DB960DD38080384D408DDDBB17C727BB4A53AF72000:8 X-MTK: N X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200612_023343_061881_8969D04A X-CRM114-Status: GOOD ( 29.89 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, drinkcat@chromium.org, andriy.shevchenko@linux.intel.com, srv_heupstream@mediatek.com, devicetree@vger.kernel.org, linus.walleij@linaro.org, shengnan.wang@mediatek.com, louis.kuo@mediatek.com, bgolaszewski@baylibre.com, sj.huang@mediatek.com, robh+dt@kernel.org, linux-mediatek@lists.infradead.org, dongchun.zhu@mediatek.com, sakari.ailus@linux.intel.com, matthias.bgg@gmail.com, bingbu.cao@intel.com, mchehab@kernel.org, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Tomasz, On Wed, 2020-06-10 at 18:36 +0000, Tomasz Figa wrote: > On Sat, May 23, 2020 at 12:50:15PM +0800, Dongchun Zhu wrote: > > Hi Tomasz, > > > > Thanks for the review. My replies are as below. > > > > On Thu, 2020-05-21 at 19:32 +0000, Tomasz Figa wrote: > > > Hi Dongchun, > > > > > > On Sat, May 09, 2020 at 04:06:27PM +0800, Dongchun Zhu wrote: > [snip] > > > > +{ > > > > + struct i2c_client *client = to_i2c_client(dev); > > > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > + int ret; > > > > + > > > > + gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0); > > > > + gpiod_set_value_cansleep(ov02a10->pd_gpio, 0); > > > > + > > > > + ret = clk_prepare_enable(ov02a10->eclk); > > > > + if (ret < 0) { > > > > + dev_err(dev, "failed to enable eclk\n"); > > > > + return ret; > > > > + } > > > > + > > > > + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > > > > + if (ret < 0) { > > > > + dev_err(dev, "failed to enable regulators\n"); > > > > + goto disable_clk; > > > > + } > > > > + usleep_range(5000, 6000); > > > > + > > > > + gpiod_set_value_cansleep(ov02a10->pd_gpio, 1); > > > > > > This is a "powerdown" GPIO. It must be set to 0 if the sensor is to be > > > powered on. > > > > > > > The value set by gpiod_set_value_cansleep() API actually depends upon > > GPIO polarity defined in DT. > > Since I set GPIO_ACTIVE_LOW to powerdown, > > gpiod_set_value_cansleep(gpio_desc, value) would set !value to > > gpio_desc. > > Thus here powerdown would be low-state when sensor is powered on. > > For GPIO polarity, I also post a comment to the binding patch. > > > > That's true. However, this makes the driver really confusing. If someone > reads this code and compares with the datasheet, it looks incorrect, > because in the datasheet the powerdown GPIO needs to be configured low > for the sensor to operate. > > I'd recommend defining the binding in a way that makes it clear in the driver code > that it implementes the power sequencing as per the datasheet. > Uh-huh... But it all depends on how we look at the powerdown GPIO. Or where should we define the active low or active high, the driver or DT? My initial idea is using DT GPIO polarity to describe sensor active polarity according to the datasheet. As an active low shutdown signal is equivalent to an active high enable signal. > [snip] > > > > +/* > > > > + * ov02a10_set_exposure - Function called when setting exposure time > > > > + * @priv: Pointer to device structure > > > > + * @val: Variable for exposure time, in the unit of micro-second > > > > + * > > > > + * Set exposure time based on input value. > > > > + * > > > > + * Return: 0 on success > > > > + */ > > > > +static int ov02a10_set_exposure(struct ov02a10 *ov02a10, int val) > > > > +{ > > > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > > > + int ret; > > > > + > > > > + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE); > > > > + if (ret < 0) > > > > + return ret; > > > > > > How does this page switch work? According to the documentation I have, the > > > register allows selecting between a few different pages. However, there > > > should be two page pointers - one for the AP and the other for the sensor, > > > so that when the AP is programming page X, the sensor can have consistent > > > settings from page Y. But here we only set one register and always with > > > page 1. > > > > > > > Thanks for the carefully observation. > > The style or requirement of register setting here is suggested by OV > > vendor. > > From hardware signal behavior and effect-test, this setting should be > > right. > > But for your concern, we can also dig into it with OV. > > Let's have time to talk with OV. > > > > > > + > > > > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_H, > > > > + val >> OV02A10_EXP_SHIFT); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_L, val); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE, > > > > + REG_ENABLE); > > > > > > This patch defines REG_GLOBAL_EFFECTIVE to 0x01. I don't see such register > > > mentioned in the documentation. > > > > > > > There may be several editions of sensor documentation. > > From OV, 0x01 is one register shall be updated to keep > > exposure/gain/test pattern... register settings effective. > > > > Okay, let's try to get some explanation of this offline. > Fine. > > > > +} > > > > + > > > > +static int ov02a10_set_gain(struct ov02a10 *ov02a10, int val) > > > > +{ > > > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > > > + int ret; > > > > + > > > > + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_GAIN, val); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE, > > > > + REG_ENABLE); > > > > +} > > > > + > > > > +static int ov02a10_set_vblank(struct ov02a10 *ov02a10, int val) > > > > +{ > > > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > > > + u32 vts = val + ov02a10->cur_mode->height - OV02A10_BASIC_LINE; > > > > + int ret; > > > > + > > > > + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H, > > > > + vts >> OV02A10_VTS_SHIFT); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L, vts); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE, > > > > + REG_ENABLE); > > > > +} > > > > + > > > > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern) > > > > +{ > > > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > > > + int ret; > > > > + > > > > + if (pattern) > > > > + pattern = OV02A10_TEST_PATTERN_ENABLE; > > > > > > Is this necessary? Our control can be 0 for disabled and 1 for color bars. > > > The latter is the same as the above macro. > > > > > > > Yes. It looks redundant here. > > Fixed in next release. > > > > > [snip] > > > > +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10) > > > > +{ > > > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > > > + const struct ov02a10_mode *mode; > > > > + struct v4l2_ctrl_handler *handler; > > > > + struct v4l2_ctrl *ctrl; > > > > + u64 exposure_max; > > > > + u32 pixel_rate, h_blank; > > > > + int ret; > > > > + > > > > + handler = &ov02a10->ctrl_handler; > > > > + mode = ov02a10->cur_mode; > > > > + ret = v4l2_ctrl_handler_init(handler, 7); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + handler->lock = &ov02a10->mutex; > > > > + > > > > + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, 0, 0, > > > > + link_freq_menu_items); > > > > + if (ctrl) > > > > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > + > > > > + pixel_rate = to_pixel_rate(0); > > > > + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 0, pixel_rate, 1, > > > > + pixel_rate); > > > > + > > > > + h_blank = mode->hts_def - mode->width; > > > > + ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK, > > > > + h_blank, h_blank, 1, h_blank); > > > > + if (ov02a10->hblank) > > > > + ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > + > > > > > > Do we need to hold a pointer to this control? We don't seem to ever access > > > it anywhere else in the driver. > > > > > > > No. > > These lines would be removed in next release. > > > > > > + ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > > > + V4L2_CID_VBLANK, mode->vts_def - > > > > + mode->height, > > > > + OV02A10_VTS_MAX - mode->height, 1, > > > > + mode->vts_def - mode->height); > > > > + > > > > > > Ditto. > > > > > > > These lines would be removed in next release. > > Sorry that I made a mistake. Here we need to reserve V4L2_CID_VBLANK control. > > > > + exposure_max = mode->vts_def - 4; > > > > + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > > > + V4L2_CID_EXPOSURE, > > > > + OV02A10_EXPOSURE_MIN, > > > > + exposure_max, > > > > + OV02A10_EXPOSURE_STEP, > > > > + mode->exp_def); > > > > + > > > > + ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > > > + V4L2_CID_ANALOGUE_GAIN, > > > > + OV02A10_GAIN_MIN, > > > > + OV02A10_GAIN_MAX, > > > > + OV02A10_GAIN_STEP, > > > > + OV02A10_GAIN_DEFAULT); > > > > > > Ditto. > > > > > > > Fields: exposure and anal_gain would be removed in next release. > > But v4l2_ctrl_new_std remains, as user may set exp/gain. > > > > I don't mean removing the controls, but just not storing the returned > pointers inside driver data. > Got it : -) > Best regards, > Tomasz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel