Hi Dave, On Thu, Nov 24, 2022 at 06:02:54PM +0000, Dave Stevenson wrote: > On Wed, 23 Nov 2022 at 14:29, Laurent Pinchart wrote: > > 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. I understand your frustration, and have experienced it too myself at times. We're trying to align our messages, unfortunately it's mostly ad-hoc. There are multiple reasons for that, sometimes we realize that we've being done things wrong for a long time, or we need to experiment to find the best option. The media subsystem being understaffed doesn't help, and I think it also drives contributors away as a result, which makes the problem worse. With more time, I would really like to work on standardization of camera sensor APIs in the media subsystem, both in-kernel and towards userspace. We've discussed that in Dublin, you know how painful it is today. > > 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. Great, I'll then test your patches on my board :-) > > 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
