Re: [PATCH v2 5/6] media: entity: Add support for ancillary links

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@gmail.com>
> > ---
> > 
> > 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@ideasonboard.com>

> > +
> >  #define media_entity_call(entity, operation, args...)			\
> >  	(((entity)->ops && (entity)->ops->operation) ?			\
> >  	 (entity)->ops->operation((entity) , ##args) : -ENOIOCTLCMD)

-- 
Regards,

Laurent Pinchart