Hi Laurent On Wed, 23 Nov 2022 at 14:29, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > 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. Likewise! Standardisation is a good thing! Sorry my comment was slightly tongue-in-cheek due to having had such a similar thread with Sakari so recently. When a long-standing member of the community then comes along with a similar patch it just reinforced that, in the absence of any documented guidelines, it is all very much ad-hoc. It then frustrates me that these sorts of issues are then raised at review, which either results in having to justify the choice, or respinning patches often with time constraints if trying to hit a merge window. > 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 ov9282 I'd also raised the issue that a fair number of sensor drivers include a software reset in their lists of registers, which will undo any settings done in resume. As above, it was more of an observation than issue with this patch. Alexander has already given an R-b, so there's limited point adding mine. I'll try and test the series out tomorrow, and I will get around to submitting my series on top. Cheers. Dave > 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
