| Message ID | 20221212221719.18053-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 1ec061243edeb729ac249b278bb241a846cc8e64 |
| Headers | show |
| Series | media: mc: Improve the media_entity_has_pad_interdep() documentation | expand |
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
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>
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. *
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(-)