diff mbox series

[2/2] media: Documentation: Update {enable,disable}_streams documentation

Message ID 20240917124345.16681-2-sakari.ailus@linux.intel.com (mailing list archive)
State New
Headers
Series [1/2] media: Documentation: Deprecate s_stream video op, update docs |

Checks

Context Check Description
media-ci/HTML_report success Link
media-ci/report success Link
media-ci/virtme32 success Link
media-ci/virtme64 success Link
media-ci/bisect success Link
media-ci/doc success Link
media-ci/build success Link
media-ci/static-upstream success Link
media-ci/abi success Link
media-ci/media-patchstyle success Link
media-ci/checkpatch success Link

Commit Message

Sakari Ailus Sept. 17, 2024, 12:43 p.m. UTC 
  Document the expected {enable,disable}_streams callback behaviour for
drivers that are stream-unaware i.e. don't specify the
V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
the mask argument can be ignored.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/v4l2-subdev.h | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Laurent Pinchart Sept. 17, 2024, 1 p.m. UTC | #1 
On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote:
> Document the expected {enable,disable}_streams callback behaviour for
> drivers that are stream-unaware i.e. don't specify the
> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
> the mask argument can be ignored.

Wouldn't it be better to use BIT(0) in that case to simplifiy
interoperability with stream-aware devices ?

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/media/v4l2-subdev.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 3cc6b4a5935f..67a6e6ec58b8 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -834,11 +834,19 @@ struct v4l2_subdev_state {
>   *	v4l2_subdev_init_finalize() at initialization time). Do not call
>   *	directly, use v4l2_subdev_enable_streams() instead.
>   *
> + *	Drivers that support only a single stream without setting the
> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
> + *	be concerned with the mask argument.
> + *
>   * @disable_streams: Disable 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_disable_streams() instead.
> + *
> + *	Drivers that support only a single stream without setting the
> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
> + *	be concerned with the mask argument.
>   */
>  struct v4l2_subdev_pad_ops {
>  	int (*enum_mbus_code)(struct v4l2_subdev *sd,
Sakari Ailus Sept. 17, 2024, 1:17 p.m. UTC | #2 
Hi Laurent,

On Tue, Sep 17, 2024 at 04:00:47PM +0300, Laurent Pinchart wrote:
> On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote:
> > Document the expected {enable,disable}_streams callback behaviour for
> > drivers that are stream-unaware i.e. don't specify the
> > V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
> > the mask argument can be ignored.
> 
> Wouldn't it be better to use BIT(0) in that case to simplifiy
> interoperability with stream-aware devices ?

That's indeed the current implementation.
Tomi Valkeinen Sept. 17, 2024, 2:16 p.m. UTC | #3 
On 17/09/2024 16:00, Laurent Pinchart wrote:
> On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote:
>> Document the expected {enable,disable}_streams callback behaviour for
>> drivers that are stream-unaware i.e. don't specify the
>> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
>> the mask argument can be ignored.
> 
> Wouldn't it be better to use BIT(0) in that case to simplifiy
> interoperability with stream-aware devices ?

The caller has to set BIT(0), but I think here the documentation is 
about the callee.

If the driver is not stream aware and implements the callbacks, it will 
get BIT(0) as the mask parameter (do we enforce this?), but as there's 
nothing it can do with the parameter it "does not need to be concerned 
with the mask argument".

  Tomi

>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>   include/media/v4l2-subdev.h | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 3cc6b4a5935f..67a6e6ec58b8 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -834,11 +834,19 @@ struct v4l2_subdev_state {
>>    *	v4l2_subdev_init_finalize() at initialization time). Do not call
>>    *	directly, use v4l2_subdev_enable_streams() instead.
>>    *
>> + *	Drivers that support only a single stream without setting the
>> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
>> + *	be concerned with the mask argument.
>> + *
>>    * @disable_streams: Disable 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_disable_streams() instead.
>> + *
>> + *	Drivers that support only a single stream without setting the
>> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
>> + *	be concerned with the mask argument.
>>    */
>>   struct v4l2_subdev_pad_ops {
>>   	int (*enum_mbus_code)(struct v4l2_subdev *sd,
>
Laurent Pinchart Sept. 17, 2024, 2:57 p.m. UTC | #4 
On Tue, Sep 17, 2024 at 05:16:25PM +0300, Tomi Valkeinen wrote:
> On 17/09/2024 16:00, Laurent Pinchart wrote:
> > On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote:
> >> Document the expected {enable,disable}_streams callback behaviour for
> >> drivers that are stream-unaware i.e. don't specify the
> >> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
> >> the mask argument can be ignored.
> > 
> > Wouldn't it be better to use BIT(0) in that case to simplifiy
> > interoperability with stream-aware devices ?
> 
> The caller has to set BIT(0), but I think here the documentation is 
> about the callee.
> 
> If the driver is not stream aware and implements the callbacks, it will 
> get BIT(0) as the mask parameter (do we enforce this?), but as there's 
> nothing it can do with the parameter it "does not need to be concerned 
> with the mask argument".

Right. I had misunderstood the patch.

> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> ---
> >>   include/media/v4l2-subdev.h | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >> index 3cc6b4a5935f..67a6e6ec58b8 100644
> >> --- a/include/media/v4l2-subdev.h
> >> +++ b/include/media/v4l2-subdev.h
> >> @@ -834,11 +834,19 @@ struct v4l2_subdev_state {
> >>    *	v4l2_subdev_init_finalize() at initialization time). Do not call
> >>    *	directly, use v4l2_subdev_enable_streams() instead.
> >>    *
> >> + *	Drivers that support only a single stream without setting the
> >> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to

s/capatility/capability/

Same below.

> >> + *	be concerned with the mask argument.

How about "can ignore the mask argument" instead ? I interpreted as "not
need to be concerned with" from the point of view of the caller.

> >> + *
> >>    * @disable_streams: Disable 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_disable_streams() instead.
> >> + *
> >> + *	Drivers that support only a single stream without setting the
> >> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
> >> + *	be concerned with the mask argument.
> >>    */
> >>   struct v4l2_subdev_pad_ops {
> >>   	int (*enum_mbus_code)(struct v4l2_subdev *sd,
Tomi Valkeinen Sept. 17, 2024, 3 p.m. UTC | #5 
On 17/09/2024 17:57, Laurent Pinchart wrote:
> On Tue, Sep 17, 2024 at 05:16:25PM +0300, Tomi Valkeinen wrote:
>> On 17/09/2024 16:00, Laurent Pinchart wrote:
>>> On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote:
>>>> Document the expected {enable,disable}_streams callback behaviour for
>>>> drivers that are stream-unaware i.e. don't specify the
>>>> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
>>>> the mask argument can be ignored.
>>>
>>> Wouldn't it be better to use BIT(0) in that case to simplifiy
>>> interoperability with stream-aware devices ?
>>
>> The caller has to set BIT(0), but I think here the documentation is
>> about the callee.
>>
>> If the driver is not stream aware and implements the callbacks, it will
>> get BIT(0) as the mask parameter (do we enforce this?), but as there's
>> nothing it can do with the parameter it "does not need to be concerned
>> with the mask argument".
> 
> Right. I had misunderstood the patch.
> 
>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> ---
>>>>    include/media/v4l2-subdev.h | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>>> index 3cc6b4a5935f..67a6e6ec58b8 100644
>>>> --- a/include/media/v4l2-subdev.h
>>>> +++ b/include/media/v4l2-subdev.h
>>>> @@ -834,11 +834,19 @@ struct v4l2_subdev_state {
>>>>     *	v4l2_subdev_init_finalize() at initialization time). Do not call
>>>>     *	directly, use v4l2_subdev_enable_streams() instead.
>>>>     *
>>>> + *	Drivers that support only a single stream without setting the
>>>> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
> 
> s/capatility/capability/
> 
> Same below.
> 
>>>> + *	be concerned with the mask argument.
> 
> How about "can ignore the mask argument" instead ? I interpreted as "not
> need to be concerned with" from the point of view of the caller.

Or maybe, to be transparent, "can ignore the mask argument as it is 
always 1".

  Tomi
Sakari Ailus Sept. 17, 2024, 3:05 p.m. UTC | #6 
Hi Laurent,

On Tue, Sep 17, 2024 at 05:57:35PM +0300, Laurent Pinchart wrote:
> On Tue, Sep 17, 2024 at 05:16:25PM +0300, Tomi Valkeinen wrote:
> > On 17/09/2024 16:00, Laurent Pinchart wrote:
> > > On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote:
> > >> Document the expected {enable,disable}_streams callback behaviour for
> > >> drivers that are stream-unaware i.e. don't specify the
> > >> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
> > >> the mask argument can be ignored.
> > > 
> > > Wouldn't it be better to use BIT(0) in that case to simplifiy
> > > interoperability with stream-aware devices ?
> > 
> > The caller has to set BIT(0), but I think here the documentation is 
> > about the callee.
> > 
> > If the driver is not stream aware and implements the callbacks, it will 
> > get BIT(0) as the mask parameter (do we enforce this?), but as there's 
> > nothing it can do with the parameter it "does not need to be concerned 
> > with the mask argument".
> 
> Right. I had misunderstood the patch.
> 
> > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >> ---
> > >>   include/media/v4l2-subdev.h | 8 ++++++++
> > >>   1 file changed, 8 insertions(+)
> > >>
> > >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > >> index 3cc6b4a5935f..67a6e6ec58b8 100644
> > >> --- a/include/media/v4l2-subdev.h
> > >> +++ b/include/media/v4l2-subdev.h
> > >> @@ -834,11 +834,19 @@ struct v4l2_subdev_state {
> > >>    *	v4l2_subdev_init_finalize() at initialization time). Do not call
> > >>    *	directly, use v4l2_subdev_enable_streams() instead.
> > >>    *
> > >> + *	Drivers that support only a single stream without setting the
> > >> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
> 
> s/capatility/capability/
> 
> Same below.
> 
> > >> + *	be concerned with the mask argument.
> 
> How about "can ignore the mask argument" instead ? I interpreted as "not
> need to be concerned with" from the point of view of the caller.

Sounds good. I'll address these in v3, after waiting a bit for further
comments.

> 
> > >> + *
> > >>    * @disable_streams: Disable 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_disable_streams() instead.
> > >> + *
> > >> + *	Drivers that support only a single stream without setting the
> > >> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
> > >> + *	be concerned with the mask argument.
> > >>    */
> > >>   struct v4l2_subdev_pad_ops {
> > >>   	int (*enum_mbus_code)(struct v4l2_subdev *sd,
>
diff mbox series

Patch

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 3cc6b4a5935f..67a6e6ec58b8 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -834,11 +834,19 @@  struct v4l2_subdev_state {
  *	v4l2_subdev_init_finalize() at initialization time). Do not call
  *	directly, use v4l2_subdev_enable_streams() instead.
  *
+ *	Drivers that support only a single stream without setting the
+ *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
+ *	be concerned with the mask argument.
+ *
  * @disable_streams: Disable 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_disable_streams() instead.
+ *
+ *	Drivers that support only a single stream without setting the
+ *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
+ *	be concerned with the mask argument.
  */
 struct v4l2_subdev_pad_ops {
 	int (*enum_mbus_code)(struct v4l2_subdev *sd,