On Wed, Nov 01, 2023 at 02:08:32PM +0000, Sakari Ailus wrote: > On Wed, Nov 01, 2023 at 03:45:06PM +0200, Laurent Pinchart wrote: > > 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. I meant that most sensor drivers are written by copying code from other drivers without understanding what that code does exactly, and why. > 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. It's time to relegate historical code to the git history :-) > > 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. Would you add a link to v4l2-subdev.rst at the top of the file ? It can be done in a separate patch. > > > > > 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". My bad, I was looking at the wrong location. Replacing the reference to sensor with subdev should be good enough then. > > > > > + > > > > > Asynchronous sub-device notifiers > > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > -- Regards, Laurent Pinchart
