Date:   Thu, 2 Feb 2023 11:01:13 +0200
From:   Sakari Ailus <sakari.ailus@linux.intel.com>
To:     Michael Riesch <michael.riesch@wolfvision.net>
Cc:     linux-media@vger.kernel.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Gerald Loacker <gerald.loacker@wolfvision.net>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Marco Felsch <m.felsch@pengutronix.de>,
        Mikhail Rudenko <mike.rudenko@gmail.com>,
        Krzysztof =?utf-8?Q?Ha=C5=82asa?= <khalasa@piap.pl>,
        Marek Vasut <marex@denx.de>
Subject: Re: [PATCH v2 2/2] media: i2c: add imx415 cmos image sensor driver
Message-ID: <Y9t72at49U97brb/@kekkonen.localdomain>
References: <20230124060107.3922237-1-michael.riesch@wolfvision.net>
 <20230124060107.3922237-3-michael.riesch@wolfvision.net>
 <Y9EKqwfDSsF31dLZ@kekkonen.localdomain>
 <395807d3-e242-5779-5c6d-06d750357b8c@wolfvision.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <395807d3-e242-5779-5c6d-06d750357b8c@wolfvision.net>
Precedence: bulk

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