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@xxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxx>
--
Regards,
Laurent Pinchart
