On Tue, Sep 17, 2024 at 05:22:29PM +0300, Tomi Valkeinen wrote:
> On 17/09/2024 15:43, Sakari Ailus wrote:
> > The scope of the s_stream video operation is now fully supported by
> > {enable,disable}_straems. Explicitly document the s_stream() op as
> > deprecated and update the related documentation.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > Documentation/driver-api/media/camera-sensor.rst | 8 ++++----
> > Documentation/driver-api/media/tx-rx.rst | 11 ++++++-----
> > include/media/v4l2-subdev.h | 5 +++--
> > 3 files changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > index b4920b34cebc..c290833165e6 100644
> > --- a/Documentation/driver-api/media/camera-sensor.rst
> > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > @@ -81,10 +81,10 @@ restart when the system is resumed. This requires coordination between the
> > camera sensor and the rest of the camera pipeline. Bridge drivers are
> > responsible for this coordination, and instruct camera sensors to stop and
> > restart streaming by calling the appropriate subdev operations
> > -(``.s_stream()``, ``.enable_streams()`` or ``.disable_streams()``). Camera
> > -sensor drivers shall therefore **not** keep track of the streaming state to
> > -stop streaming in the PM suspend handler and restart it in the resume handler.
> > -Drivers should in general not implement the system PM handlers.
> > +(``.enable_streams()`` or ``.disable_streams()``). Camera sensor drivers shall
>
> I didn't go through the docs, but I think we need to make it clear
> somewhere that v4l2_subdev_enable_streams() and
> v4l2_subdev_disable_streams() _must_ be used to enable/disable the
> streaming in the source subdevice, and the related subdev callbacks must
> not be called directly.
include/media/v4l2-subdev.h states
* @enable_streams: Enable the streams defined in streams_mask on the given
* source pad. Subdevs that implement this operation must use the active
* state management provided by the subdev core (enabled through a call to
* v4l2_subdev_init_finalize() at initialization time). Do not call
* directly, use v4l2_subdev_enable_streams() instead.
This patch looks good to me.
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> Also, v4l2_subdev_enable_streams() and v4l2_subdev_disable_streams()
> kernel doc doesn't seem to clarify that if the driver using those
> functions does not support streams, it should use BIT_ULL(1) as the
> streams_mask parameter.
>
> > +therefore **not** keep track of the streaming state to stop streaming in the PM
> > +suspend handler and restart it in the resume handler. Drivers should in general
> > +not implement the system PM handlers.
> >
> > Camera sensor drivers shall **not** implement the subdev ``.s_power()``
> > operation, as it is deprecated. While this operation is implemented in some
> > diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
> > index 29d66a47b56e..a339df61fca8 100644
> > --- a/Documentation/driver-api/media/tx-rx.rst
> > +++ b/Documentation/driver-api/media/tx-rx.rst
> > @@ -49,11 +49,12 @@ Link frequency
> > The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> >
> > -``.s_stream()`` callback
> > -^^^^^^^^^^^^^^^^^^^^^^^^
> > +``.enable_streams()`` and ``.disable_streams()`` callbacks
> > +^^^^^^^^^^^^^^^^^^^^^^^^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > -The struct struct v4l2_subdev_video_ops->s_stream() callback is used by the
> > -receiver driver to control the transmitter driver's streaming state.
> > +The struct v4l2_subdev_pad_ops->enable_streams() and struct
> > +v4l2_subdev_pad_ops->disable_streams() callbacks are used by the receiver driver
> > +to control the transmitter driver's streaming state.
> >
> >
> > CSI-2 transmitter drivers
> > @@ -127,7 +128,7 @@ Stopping the transmitter
> > ^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > A transmitter stops sending the stream of images as a result of
> > -calling the ``.s_stream()`` callback. Some transmitters may stop the
> > +calling the ``.disable_streams()`` callback. Some transmitters may stop the
> > stream at a frame boundary whereas others stop immediately,
> > effectively leaving the current frame unfinished. The receiver driver
> > should not make assumptions either way, but function properly in both
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 8daa0929865c..3cc6b4a5935f 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -450,8 +450,9 @@ enum v4l2_subdev_pre_streamon_flags {
> > * already started or stopped subdev. Also see call_s_stream wrapper in
> > * v4l2-subdev.c.
> > *
> > - * New drivers should instead implement &v4l2_subdev_pad_ops.enable_streams
> > - * and &v4l2_subdev_pad_ops.disable_streams operations, and use
> > + * This callback is DEPRECATED. New drivers should instead implement
> > + * &v4l2_subdev_pad_ops.enable_streams and
> > + * &v4l2_subdev_pad_ops.disable_streams operations, and use
> > * v4l2_subdev_s_stream_helper for the &v4l2_subdev_video_ops.s_stream
> > * operation to support legacy users.
> > *
--
Regards,
Laurent Pinchart
