Hi Laurent, On Wed, Nov 01, 2023 at 02:53:50PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. Thank you for the review. > > On Wed, Nov 01, 2023 at 10:05:46AM +0200, Sakari Ailus wrote: > > Document that sub-device initialisation needs to complete before the async > > sub-device is registered as there is no further driver action needed > > before the sensor becomes accessible via the UAPI. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > --- > > Documentation/driver-api/media/camera-sensor.rst | 3 ++- > > Documentation/driver-api/media/v4l2-subdev.rst | 4 ++++ > > 2 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst > > index 6456145f96ed..c675ce77c4d6 100644 > > --- a/Documentation/driver-api/media/camera-sensor.rst > > +++ b/Documentation/driver-api/media/camera-sensor.rst > > @@ -60,7 +60,8 @@ management over the pipeline. > > Camera sensor drivers are responsible for controlling the power state of the > > device they otherwise control as well. They shall use runtime PM to manage > > power states. Runtime PM shall be enabled at probe time and disabled at remove > > -time. Drivers should enable runtime PM autosuspend. > > +time. Drivers should enable runtime PM autosuspend. Note that runtime PM has to > > +be enabled before registering the sensor's async sub-device. > > I wouldn't single out runtime PM initialization here, and neither would > I talk about registration, as that's not in scope for this file. I think > it would be better to instead reference v4l2-subdev.rst at the beginning > of this file. Something generic like stating that camera sensor must be > implemented as subdevs, and comply with the generic rules for subdevs, > as explaiend in v4l2-subdev.rst. I added this text here as this appears to be a very common problem in sensor drivers. Tost likely the reason is that some drivers have had this issue and it has gotten copied elsewhere. > > > > > The runtime PM handlers shall handle clocks, regulators, GPIOs, and other > > system resources required to power the sensor up and down. For drivers that > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > > index e56b50b3f203..b22d1b075fd6 100644 > > --- a/Documentation/driver-api/media/v4l2-subdev.rst > > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > > @@ -195,6 +195,10 @@ performed using the :c:func:`v4l2_async_unregister_subdev` call. Subdevices > > registered this way are stored in a global list of subdevices, ready to be > > picked up by bridge drivers. > > > > +Note that all sensor initialisation has to complete before registering the async > > +sub-device, including enabling runtime PM. This is because the sensor becomes > > +accessible via the UAPI without further action by the sensor driver. > > It's not just the UAPI, but also in-kernel users. > > The passive form is heavier and thus harder to read. Maybe something > like the following ? Feel free to rework it. > > ---- > Drivers must complete all initialization of the sensor before registering the > async sub-device. This includes enabling runtime PM. This is because the sensor > becomes accessible and may be used as soon as it gets registered. s/used/accessed/ maybe? Or remove "and maybe used"? > ---- > > Also, wouldn't this be better placed in the "Registering asynchronous > sub-devices" section ? This isn't specific to sensors. Ah, yes, I meant sub-devices there. This actually applies to all of UAPI, in general, but it's the async sub-device drivers that tend to have the problem as people tend to think it's "just" registering the async sub-device but in fact the related sub-device may be immediately registered as well. > > > + > > Asynchronous sub-device notifiers > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > -- Regards, Sakari Ailus
