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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 5B727C05027 for ; Thu, 2 Feb 2023 09:02:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Z346qXissek9KzFSHgS6O4u7ndOwls/EeV25+xL1cuY=; b=i4EcGLHIpCZa0u tbplKg7stVDd/UZDbir/gsPnoA47dJjzcTS0Qu38/h7hBJfWvjJbESQJilJ3E4yHg2E4RfKd8t4Oq lhdycCOg5xp35SEanKVj86O1YGPemSzQgyk/YyMlOqeSiHeha2B0JB8tPRGww1MvwiTbZlB6jeX7K xS3vzX0lSUy6h5yX5ifCw5jPzH5u/j6vdz9oWQtdLLbgaJPnSb/5VcHfwmi47rlb3DfjI+WEVDx4l Ci6aelqvfBF3lfrm1TjUziWNLdhSWW+1NyyWcFPilBErzDh2RDP8DLurNd+5+nkdtoYOODYSoegUc 8F3H+e/3dQ629qAtYndA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pNVTJ-00F4as-6y; Thu, 02 Feb 2023 09:01:33 +0000 Received: from mga03.intel.com ([134.134.136.65]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pNVTE-00F4Y5-Ih for linux-arm-kernel@lists.infradead.org; Thu, 02 Feb 2023 09:01:30 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1675328488; x=1706864488; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=za/ubQr/+93gCMuJJ1xC8ZQWhY/KbUwbbbwQt6HxEIc=; b=V+Xi2oSTV0UUheVSZ3vwYLvzgN5ajKnix8AjsOi2JlshKHfOs/7L3qBc vm7N5Jt8+aYFdb6tqmB6KxnB4lc1SNcOR9xnhuocUf2JCYngvXjivf3MD aNUPCIHDhzjDnnDRVlGOXF+bWDyvVEox3PgChy8DlOtLJE5MTCq/gEVrc l6cU95czWEZmDLi+4phPCSeDNfFlplXlHVh5EgBc/3LBXZZN6ZnU5+Ww2 JxzZG/ff7PKawfbNKArQMJQnhdtI2XJisVbA9u8vhwfwb/+v6ohCFoPxl reH4MMPTqqllATIlaOHe/07KsAHkiQBNzLgMlklnXxtCH+SRyhdqZvlqL Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10608"; a="330530288" X-IronPort-AV: E=Sophos;i="5.97,267,1669104000"; d="scan'208";a="330530288" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Feb 2023 01:01:24 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10608"; a="728775050" X-IronPort-AV: E=Sophos;i="5.97,267,1669104000"; d="scan'208";a="728775050" Received: from turnipsi.fi.intel.com (HELO kekkonen.fi.intel.com) ([10.237.72.44]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Feb 2023 01:01:19 -0800 Received: from kekkonen.localdomain (localhost [IPv6:::1]) by kekkonen.fi.intel.com (Postfix) with SMTP id E68591218DC; Thu, 2 Feb 2023 11:01:13 +0200 (EET) Date: Thu, 2 Feb 2023 11:01:13 +0200 From: Sakari Ailus To: Michael Riesch Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Gerald Loacker , Mauro Carvalho Chehab , Rob Herring , Krzysztof Kozlowski , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Laurent Pinchart , Hans Verkuil , Marco Felsch , Mikhail Rudenko , Krzysztof =?utf-8?Q?Ha=C5=82asa?= , Marek Vasut Subject: Re: [PATCH v2 2/2] media: i2c: add imx415 cmos image sensor driver Message-ID: References: <20230124060107.3922237-1-michael.riesch@wolfvision.net> <20230124060107.3922237-3-michael.riesch@wolfvision.net> <395807d3-e242-5779-5c6d-06d750357b8c@wolfvision.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <395807d3-e242-5779-5c6d-06d750357b8c@wolfvision.net> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230202_010128_715528_AE85E263 X-CRM114-Status: GOOD ( 31.26 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Michael, On Fri, Jan 27, 2023 at 11:43:33AM +0100, Michael Riesch wrote: > Hi Sakari, > > Thanks for your review. The majority of your comments are clear, I'll > spin a v3 next week. Just a few things: > > On 1/25/23 11:55, Sakari Ailus wrote: > > [...] > >> +++ b/drivers/media/i2c/imx415.c > >> @@ -0,0 +1,1296 @@ > >> +// SPDX-License-Identifier: GPL-2.0-only > >> +/* > >> + * Driver for the Sony IMX415 CMOS Image Sensor. > >> + * > >> + * Copyright (C) 2022 WolfVision GmbH. > > > > You can use 2023 now. > > Time flies, doesn't it... :-) > > > [...] > >> +static int imx415_stream_on(struct imx415 *sensor) > >> +{ > >> + int ret; > >> + > >> + ret = imx415_write(sensor, IMX415_MODE, IMX415_MODE_OPERATING); > >> + if (ret) > >> + return ret; > >> + > >> + /* wait at least 24 ms for internal regulator stabilization */ > >> + msleep(30); > > > > This is a very, very long time to wait for a regulator. Most probably > > either the time is too long or we're waiting for something else. > > I just realized that both msleep calls are after setting the mode to > operating, i.e., after getting the sensor out of standby. The other > instance of this code (see below) documents that clearly, but this > "regulator stabilization" comment here is seems wrong indeed. > > >> + > >> + return imx415_write(sensor, IMX415_XMSTA, IMX415_XMSTA_START); > >> +} > >> [...]>> +static int imx415_subdev_init(struct imx415 *sensor) > >> +{ > >> + struct i2c_client *client = to_i2c_client(sensor->dev); > >> + int ret; > >> + > >> + v4l2_i2c_subdev_init(&sensor->subdev, client, &imx415_subdev_ops); > >> + > >> + ret = imx415_ctrls_init(sensor); > >> + if (ret < 0) > >> + return ret; > >> + > >> + sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > > > Add V4L2_SUBDEV_FL_HAS_EVENTS. > > Just for my understanding: why is this required/a good idea? > > >> [...] > >> +static int imx415_identify_model(struct imx415 *sensor) > >> +{ > >> + int model, ret; > >> + > >> + /* > >> + * While most registers can be read when the sensor is in standby, this > >> + * is not the case of the sensor info register :-( > >> + */ > >> + ret = imx415_write(sensor, IMX415_MODE, IMX415_MODE_OPERATING); > >> + if (ret < 0) > >> + return dev_err_probe(sensor->dev, ret, > >> + "failed to get sensor out of standby\n"); > >> + > >> + /* > >> + * According to the datasheet we have to wait at least 63 us after > >> + * leaving standby mode. But this doesn't work even after 30 ms. > >> + * So probably this should be 63 ms and therefore we wait for 80 ms. > >> + */ > >> + msleep(80); > > > > Wow. > > This is the other occurrence of this long sleep. We could refactor this > code into a imx415_wakeup() method if desired. Otherwise, we need to > align the sleep period and the explanation at least. I'm ok with the code, it's just a very, very long delay. -- Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel