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
