On Tue, Nov 05, 2024 at 10:27:17AM +0200, Laurent Pinchart wrote:
> On Tue, Nov 05, 2024 at 08:14:43AM +0000, Sakari Ailus wrote:
> > Hi Laurent,
> >
> > Thanks for the review.
> >
> > On Tue, Nov 05, 2024 at 10:03:07AM +0200, Laurent Pinchart wrote:
> > > Hi Sakari,
> > >
> > > Thank you for the patch.
> > >
> > > On Tue, Nov 05, 2024 at 08:40:12AM +0200, Sakari Ailus wrote:
> > > > Rename the pad field in __media_pipeline_start() to both better describe
> > > > what it is and avoid masking it during the loop.
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > > > ---
> > > > since v1:
> > > >
> > > > - Also change the argument name in the prototype.
> > > >
> > > > drivers/media/mc/mc-entity.c | 8 ++++----
> > > > include/media/media-entity.h | 4 ++--
> > > > 2 files changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > > > index 96dd0f6ccd0d..0df9fc90cf33 100644
> > > > --- a/drivers/media/mc/mc-entity.c
> > > > +++ b/drivers/media/mc/mc-entity.c
> > > > @@ -768,10 +768,10 @@ static int media_pipeline_populate(struct media_pipeline *pipe,
> > > > return ret;
> > > > }
> > > >
> > > > -__must_check int __media_pipeline_start(struct media_pad *pad,
> > > > +__must_check int __media_pipeline_start(struct media_pad *origin,
> > > > struct media_pipeline *pipe)
> > > > {
> > > > - struct media_device *mdev = pad->graph_obj.mdev;
> > > > + struct media_device *mdev = origin->graph_obj.mdev;
> > > > struct media_pipeline_pad *err_ppad;
> > > > struct media_pipeline_pad *ppad;
> > > > int ret;
> > > > @@ -782,7 +782,7 @@ __must_check int __media_pipeline_start(struct media_pad *pad,
> > > > * If the pad is already part of a pipeline, that pipeline must be the
> > > > * same as the pipe given to media_pipeline_start().
> > > > */
> > > > - if (WARN_ON(pad->pipe && pad->pipe != pipe))
> > > > + if (WARN_ON(origin->pipe && origin->pipe != pipe))
> > > > return -EINVAL;
> > > >
> > > > /*
> > > > @@ -799,7 +799,7 @@ __must_check int __media_pipeline_start(struct media_pad *pad,
> > > > * with media_pipeline_pad instances for each pad found during graph
> > > > * walk.
> > > > */
> > > > - ret = media_pipeline_populate(pipe, pad);
> > > > + ret = media_pipeline_populate(pipe, origin);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > > index 0393b23129eb..2fca4556c311 100644
> > > > --- a/include/media/media-entity.h
> > > > +++ b/include/media/media-entity.h
> > > > @@ -1143,7 +1143,7 @@ struct media_entity *media_graph_walk_next(struct media_graph *graph);
> > > >
> > > > /**
> > > > * media_pipeline_start - Mark a pipeline as streaming
> > > > - * @pad: Starting pad
> > > > + * @origin: Starting pad
> > > > * @pipe: Media pipeline to be assigned to all pads in the pipeline.
> > > > *
> > > > * Mark all pads connected to a given pad through enabled links, either
> > >
> > > s/to a given pad/to the given @starting pad/
> >
> > s/to a given pad/@origin pad/
> >
> > ?
>
> I meant
>
> * Mark all pads connected to the given @starting pad through enabled links,
There's no "starting" among the arguments.
>
> > > > @@ -1155,7 +1155,7 @@ struct media_entity *media_graph_walk_next(struct media_graph *graph);
> > > > * pipeline pointer must be identical for all nested calls to
> > > > * media_pipeline_start().
> > > > */
> > > > -__must_check int media_pipeline_start(struct media_pad *pad,
> > > > +__must_check int media_pipeline_start(struct media_pad *origin,
> > > > struct media_pipeline *pipe);
> > > > /**
> > > > * __media_pipeline_start - Mark a pipeline as streaming
> > >
> > > The documentation for __media_pipeline_start needs a similar update.
> >
> > Too many similarly named functions. :-P
> >
> > I'll address this in v3.
>
--
Sakari Ailus
