Re: [PATCH v1 14/15] media: i2c: imx290: Configure data lanes at start time

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dave,

On Wed, Nov 23, 2022 at 10:16:58AM +0000, Dave Stevenson wrote:
> On Tue, 22 Nov 2022 at 22:34, Laurent Pinchart wrote:
> >
> > There's no need to configure the data lanes in the runtime PM resume
> > handler. Do so in imx290_start_streaming() instead.
> 
> Interesting as I had Sakari advocating putting clock mode selection
> register control in "power on" for my recent ov9282 series. Is there
> any consistency?

No there isn't :-) There hasn't been any official rule so far, so it's
no surprise different drivers exhibit different behaviours. I'm all for
standardization when possible though.

Overall, I think there's a general agreement that the runtime PM resume
handler needs to control everything required to make the sensor
accessible by software. That covers at least hard reset, regulators and
clocks.

For software settings, it's less clear. If the sensor requires a
software reset sequence immediately after power on, it makes sense to
also handle that in the runtime PM resume handler. Same thing for any
other initialization required to reach a quiescent state (for instance
there are many sensors that start streaming automatically right after
power on for a reason I can't understand, so that needs to be disabled).

This means that the runtime PM handler will thus access the sensor over
I2C. We may not want to do so in probe() before having a chance to probe
it (by reading an ID register for instance). The power on sequence could
be split in two to handle this, with one function that powers the sensor
up, and the other one that handles software initialization. Both would
be called from the runtime PM resume handler, and in probe(), we could
first power on the sensor, identify it, and then initialize it. I think
that will be fine on DT platforms as we don't need to RPM-resume the I2C
device in probe before accessing it as far as I can tell, given that the
probe() function should be called with the I2C controller RPM-resumed.
I'll let Sakari confirms if this works for ACPI).

For other settings, I wouldn't handle them in the runtime PM resume
handler. In this particular case, the number of data lanes could even
vary based on the sensor mode (we don't do so at the moment), so
.s_stream() time seems better to me.

> https://patchwork.linuxtv.org/project/linux-media/patch/20221005152809.3785786-9-dave.stevenson@raspberrypi.com/#141118
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx290.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index dbed703fa199..4dfa090f918d 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -753,6 +753,9 @@ static int imx290_start_streaming(struct imx290 *imx290,
> >                 return ret;
> >         }
> >
> > +       /* Set data lane count */
> > +       imx290_set_data_lanes(imx290);
> > +
> >         /* Apply the register values related to current frame format */
> >         format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> >         ret = imx290_setup_format(imx290, format);
> > @@ -1052,9 +1055,6 @@ static int imx290_power_on(struct device *dev)
> >         gpiod_set_value_cansleep(imx290->rst_gpio, 0);
> >         usleep_range(30000, 31000);
> >
> > -       /* Set data lane count */
> > -       imx290_set_data_lanes(imx290);
> > -
> >         return 0;
> >  }
> >

-- 
Regards,

Laurent Pinchart