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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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