[PATCH] media: mc: Improve the media_entity_has_pad_interdep() documentation

[PATCH] media: mc: Improve the media_entity_has_pad_interdep() documentation

[Laurent Pinchart]

Document the function parameters, the requirements on the pad0 and pad1
arguments, the locking requirements and the return value. Also improve
the documentation of the corresponding .has_pad_interdep() operation,
stating clearly that the operation must be called through the
media_entity_has_pad_interdep() function only.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/mc/mc-entity.c | 15 ++++++++++++++-
 include/media/media-entity.h |  4 +++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index f19bb98071b2..e9b71b895f98 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -226,7 +226,13 @@ EXPORT_SYMBOL_GPL(media_entity_pads_init);
  * Graph traversal
  */
 
-/*
+/**
+ * media_entity_has_pad_interdep - Check interdependency between two pads
+ *
+ * @entity: The entity
+ * @pad0: The first pad index
+ * @pad1: The second pad index
+ *
  * This function checks the interdependency inside the entity between @pad0
  * and @pad1. If two pads are interdependent they are part of the same pipeline
  * and enabling one of the pads means that the other pad will become "locked"
@@ -236,6 +242,13 @@ EXPORT_SYMBOL_GPL(media_entity_pads_init);
  * to check the dependency inside the entity between @pad0 and @pad1. If the
  * has_pad_interdep operation is not implemented, all pads of the entity are
  * considered to be interdependent.
+ *
+ * One of @pad0 and @pad1 must be a sink pad and the other one a source pad.
+ * The function returns false if both pads are sinks or sources.
+ *
+ * The caller must hold entity->graph_obj.mdev->mutex.
+ *
+ * Return: true if the pads are connected internally and false otherwise.
  */
 static bool media_entity_has_pad_interdep(struct media_entity *entity,
 					  unsigned int pad0, unsigned int pad1)
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 1b820cb6fed1..741f9c629c6f 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -262,7 +262,9 @@ struct media_pad {
  *			part of the same pipeline and enabling one of the pads
  *			means that the other pad will become "locked" and
  *			doesn't allow configuration changes. pad0 and pad1 are
- *			guaranteed to not both be sinks or sources.
+ *			guaranteed to not both be sinks or sources. Never call
+ *			the .has_pad_interdep() operation directly, always use
+ *			media_entity_has_pad_interdep().
  *			Optional: If the operation isn't implemented all pads
  *			will be considered as interdependent.
  *
-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: mc: Improve the media_entity_has_pad_interdep() documentation

[Tomi Valkeinen]

On 13/12/2022 00:17, Laurent Pinchart wrote:
> Document the function parameters, the requirements on the pad0 and pad1
> arguments, the locking requirements and the return value. Also improve
> the documentation of the corresponding .has_pad_interdep() operation,
> stating clearly that the operation must be called through the
> media_entity_has_pad_interdep() function only.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/media/mc/mc-entity.c | 15 ++++++++++++++-
>   include/media/media-entity.h |  4 +++-
>   2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index f19bb98071b2..e9b71b895f98 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -226,7 +226,13 @@ EXPORT_SYMBOL_GPL(media_entity_pads_init);
>    * Graph traversal
>    */
>   
> -/*
> +/**
> + * media_entity_has_pad_interdep - Check interdependency between two pads
> + *
> + * @entity: The entity
> + * @pad0: The first pad index
> + * @pad1: The second pad index
> + *
>    * This function checks the interdependency inside the entity between @pad0
>    * and @pad1. If two pads are interdependent they are part of the same pipeline
>    * and enabling one of the pads means that the other pad will become "locked"
> @@ -236,6 +242,13 @@ EXPORT_SYMBOL_GPL(media_entity_pads_init);
>    * to check the dependency inside the entity between @pad0 and @pad1. If the
>    * has_pad_interdep operation is not implemented, all pads of the entity are
>    * considered to be interdependent.
> + *
> + * One of @pad0 and @pad1 must be a sink pad and the other one a source pad.
> + * The function returns false if both pads are sinks or sources.
> + *
> + * The caller must hold entity->graph_obj.mdev->mutex.
> + *
> + * Return: true if the pads are connected internally and false otherwise.
>    */
>   static bool media_entity_has_pad_interdep(struct media_entity *entity,
>   					  unsigned int pad0, unsigned int pad1)
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 1b820cb6fed1..741f9c629c6f 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -262,7 +262,9 @@ struct media_pad {
>    *			part of the same pipeline and enabling one of the pads
>    *			means that the other pad will become "locked" and
>    *			doesn't allow configuration changes. pad0 and pad1 are
> - *			guaranteed to not both be sinks or sources.
> + *			guaranteed to not both be sinks or sources. Never call
> + *			the .has_pad_interdep() operation directly, always use
> + *			media_entity_has_pad_interdep().

This is a bit confusing comment to have in the public header, as the 
media_entity_has_pad_interdep() is an internal func. The above gives the 
feeling that I'm supposed to call this, but via 
media_entity_has_pad_interdep(), but... there's no 
media_entity_has_pad_interdep().

If I'm not mistaken, none of the ops in media_entity_operations are 
supposed to be called by anyone outside mc-entity.c.

Other than that:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH] media: mc: Improve the media_entity_has_pad_interdep() documentation

[Laurent Pinchart]

Hi Tomi,

On Tue, Dec 13, 2022 at 09:00:19AM +0200, Tomi Valkeinen wrote:
> On 13/12/2022 00:17, Laurent Pinchart wrote:
> > Document the function parameters, the requirements on the pad0 and pad1
> > arguments, the locking requirements and the return value. Also improve
> > the documentation of the corresponding .has_pad_interdep() operation,
> > stating clearly that the operation must be called through the
> > media_entity_has_pad_interdep() function only.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/media/mc/mc-entity.c | 15 ++++++++++++++-
> >   include/media/media-entity.h |  4 +++-
> >   2 files changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index f19bb98071b2..e9b71b895f98 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -226,7 +226,13 @@ EXPORT_SYMBOL_GPL(media_entity_pads_init);
> >    * Graph traversal
> >    */
> >   
> > -/*
> > +/**
> > + * media_entity_has_pad_interdep - Check interdependency between two pads
> > + *
> > + * @entity: The entity
> > + * @pad0: The first pad index
> > + * @pad1: The second pad index
> > + *
> >    * This function checks the interdependency inside the entity between @pad0
> >    * and @pad1. If two pads are interdependent they are part of the same pipeline
> >    * and enabling one of the pads means that the other pad will become "locked"
> > @@ -236,6 +242,13 @@ EXPORT_SYMBOL_GPL(media_entity_pads_init);
> >    * to check the dependency inside the entity between @pad0 and @pad1. If the
> >    * has_pad_interdep operation is not implemented, all pads of the entity are
> >    * considered to be interdependent.
> > + *
> > + * One of @pad0 and @pad1 must be a sink pad and the other one a source pad.
> > + * The function returns false if both pads are sinks or sources.
> > + *
> > + * The caller must hold entity->graph_obj.mdev->mutex.
> > + *
> > + * Return: true if the pads are connected internally and false otherwise.
> >    */
> >   static bool media_entity_has_pad_interdep(struct media_entity *entity,
> >   					  unsigned int pad0, unsigned int pad1)
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 1b820cb6fed1..741f9c629c6f 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -262,7 +262,9 @@ struct media_pad {
> >    *			part of the same pipeline and enabling one of the pads
> >    *			means that the other pad will become "locked" and
> >    *			doesn't allow configuration changes. pad0 and pad1 are
> > - *			guaranteed to not both be sinks or sources.
> > + *			guaranteed to not both be sinks or sources. Never call
> > + *			the .has_pad_interdep() operation directly, always use
> > + *			media_entity_has_pad_interdep().
> 
> This is a bit confusing comment to have in the public header, as the 
> media_entity_has_pad_interdep() is an internal func. The above gives the 
> feeling that I'm supposed to call this, but via 
> media_entity_has_pad_interdep(), but... there's no 
> media_entity_has_pad_interdep().

I noticed that issue :-)

> If I'm not mistaken, none of the ops in media_entity_operations are 
> supposed to be called by anyone outside mc-entity.c.

That's correct. I thought about writing "Never call the
.has_pad_interdep() operation", but it would have been more confusing
:-) Referencing the helper function at least tells the reader that they
should make the function public if there's a valid use case, instead of
calling the operation directly.

> Other than that:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

-- 
Regards,

Laurent Pinchart