Hi Laurent, On Wed, Nov 01, 2023 at 03:45:06PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Wed, Nov 01, 2023 at 01:31:13PM +0000, Sakari Ailus wrote: > > On Wed, Nov 01, 2023 at 02:53:50PM +0200, Laurent Pinchart wrote: > > > 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. > > Yes, I think there's lots of cargo cult there. Documentation is good, > fixing drivers so that the next person to copy code will copy good code > is better, but the best solution is to move most of the problem to > helper functions provided by the core. I wouldn't call it "cargo cult" if a sensor driver needs to deal with multiple unrelated matters. Of course it's possible to have helper functions performing multiple unrelated things but at some point it is easy to notice some drivers need things to be done differently. Admittedly, most of such differences are due to historical reasons, not something you want in new drivers anymore. > > I still prefer linking to v4l2-subdev.rst. Duplicating documentation in > multiple places increases the chances it will get stale. I'm fine with dropping this part of the patch. Although what is being said there is very unlikely to change in the foreseeable future. > > > > > 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"? > > I'm fine with that. > > > > ---- > > > > > > 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. > > Note there are two sections, one about subdev registration, one about > camera sensor registration. You've added the text to the latter. The section I added the text to does not mention camera sensors separately, the section title is "Registering asynchronous sub-devices". > > > > > + > > > > Asynchronous sub-device notifiers > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > -- Regards, Sakari Ailus
