Re: [PATCH 1/1] media: Documentation: Initialisation finishes before subdev registration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Nov 01, 2023 at 05:36:35PM +0200, Laurent Pinchart wrote:
> 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.

That certainly does happen, too...

> 
> > 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.

I'll add that in v2.

> 
> > > > > >  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.

Good that we agree. :-)

I'll use this as the original sentence, after replacing sensor with
sub-device, made an unintended difference between the sub-device and teh
async sub-device:

Drivers must complete all initialization of the sub-device before
registering it using :c:func:`v4l2_async_register_subdev`, including
enabling runtime PM. This is because the sub-device becomes accessible
as soon as it gets registered.

-- 
Regards,

Sakari Ailus



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux