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

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

[Sakari Ailus]

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@linux.intel.com>
---
 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.
 
 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.
+
 Asynchronous sub-device notifiers
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-- 
2.39.2

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

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

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@linux.intel.com>
> ---
>  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.

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

Also, wouldn't this be better placed in the "Registering asynchronous
sub-devices" section ? This isn't specific to sensors.

> +
>  Asynchronous sub-device notifiers
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

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@linux.intel.com>
> > ---
> >  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

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

[Laurent Pinchart]

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@linux.intel.com>
> > > ---
> > >  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 still prefer linking to v4l2-subdev.rst. Duplicating documentation in
multiple places increases the chances it will get stale.

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

> > > +
> > >  Asynchronous sub-device notifiers
> > >  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >  

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

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@linux.intel.com>
> > > > ---
> > > >  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

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

[Laurent Pinchart]

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@linux.intel.com>
> > > > > ---
> > > > >  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

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

[Sakari Ailus]

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@linux.intel.com>
> > > > > > ---
> > > > > >  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