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@xxxxxxxxxxxxxxx/#141118 > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > --- > > 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
