On Thu, Feb 03, 2022 at 01:32:28AM +0200, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> On Sun, Jan 30, 2022 at 11:58:20PM +0000, Daniel Scally wrote:
> > Add functions to create ancillary links, so that they don't need to
> > be manually created by users.
> >
> > Signed-off-by: Daniel Scally <djrscally@xxxxxxxxx>
> > ---
> >
> > Changes since v1:
> >
> > - Hardcoded MEDIA_LINK_FL_IMMUTABLE and MEDIA_LINK_FL_ENABLED (Laurent)
> >
> > Changes since the rfc:
> >
> > - (Laurent) Set gobj0 and gobj1 directly instead of the other union
> > members
> > - (Laurent) Added MEDIA_LNK_FL_IMMUTABLE to the kerneldoc for the new
> > create function
> >
> > drivers/media/mc/mc-entity.c | 22 ++++++++++++++++++++++
> > include/media/media-entity.h | 21 +++++++++++++++++++++
> > 2 files changed, 43 insertions(+)
> >
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index 29d1285c805a..7bf2c73a3886 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -1050,3 +1050,25 @@ void media_remove_intf_links(struct media_interface *intf)
> > mutex_unlock(&mdev->graph_mutex);
> > }
> > EXPORT_SYMBOL_GPL(media_remove_intf_links);
> > +
> > +struct media_link *media_create_ancillary_link(struct media_entity *primary,
> > + struct media_entity *ancillary)
> > +{
> > + struct media_link *link;
> > +
> > + link = media_add_link(&primary->links);
> > + if (!link)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + link->gobj0 = &primary->graph_obj;
> > + link->gobj1 = &ancillary->graph_obj;
> > + link->flags = MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED |
> > + MEDIA_LNK_FL_ANCILLARY_LINK;
> > +
> > + /* Initialize graph object embedded in the new link */
> > + media_gobj_create(primary->graph_obj.mdev, MEDIA_GRAPH_LINK,
> > + &link->graph_obj);
> > +
> > + return link;
> > +}
> > +EXPORT_SYMBOL_GPL(media_create_ancillary_link);
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index fea489f03d57..afeda41ece4c 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -1104,6 +1104,27 @@ void media_remove_intf_links(struct media_interface *intf);
> > * it will issue a call to @operation\(@entity, @args\).
> > */
> >
> > +/**
> > + * media_create_ancillary_link() - create an ancillary link between two
> > + * instances of &media_entity
> > + *
> > + * @primary: pointer to the primary &media_entity
> > + * @ancillary: pointer to the ancillary &media_entity
> > + *
> > + * Create an ancillary link between two entities, indicating that they
> > + * represent two connected pieces of hardware that form a single logical unit.
>
> Here you say logical unit, while in patch 3/6 you use the term "physical
> relationship". I think I'd go for "logical" there too.
>
> > + * A typical example is a camera lens being linked to the sensor that it is
>
> s/lens/lens controller/
>
> > + * supporting.
> > + *
> > + * The function sets both MEDIA_LNK_FL_ENABLED and MEDIA_LNK_FL_IMMUTABLE for
> > + * the new link. This behaviour may be subject to change in the future, so
> > + * userspace applications using ancillary links should ensure that ancillary
> > + * links are enabled when in use.
>
> I'd drop the last two lines as this is kernel documentation, not
> userspace documentation.
>
> > + */
> > +struct media_link *
> > +media_create_ancillary_link(struct media_entity *primary,
> > + struct media_entity *ancillary);
>
> As reported by the kernel buildbot, this should go after
> media_entity_call().
And I forgot to add
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > +
> > #define media_entity_call(entity, operation, args...) \
> > (((entity)->ops && (entity)->ops->operation) ? \
> > (entity)->ops->operation((entity) , ##args) : -ENOIOCTLCMD)
--
Regards,
Laurent Pinchart
