Re: [PATCH v4 18/29] media: mc: Simplify link processing in __media_pipeline_start()

Sakari Ailus
2026-04-21T12:37:28Z

Hi Laurent,

On Tue, Apr 21, 2026 at 02:18:25PM +0300, Laurent Pinchart wrote:
> On Tue, Apr 21, 2026 at 01:24:16PM +0300, Sakari Ailus wrote:
> > On Thu, Apr 16, 2026 at 05:35:24PM +0300, Laurent Pinchart wrote:
> > > On Wed, Apr 08, 2026 at 06:39:27PM +0300, Sakari Ailus wrote:
> > > > There are two conditions checking the ENABLED link flag in the loop
> > > > going through the links related to an entity. Drop the other one and
> > > > simplify the remaining code.
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Reviewed-by: Michael Riesch <michael.riesch@collabora.com>
> > > > ---
> > > > drivers/media/mc/mc-entity.c | 11 +++++------
> > > > 1 file changed, 5 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > > > index 3fa0bc687851..6bf4730b89d2 100644
> > > > --- a/drivers/media/mc/mc-entity.c
> > > > +++ b/drivers/media/mc/mc-entity.c
> > > > @@ -838,17 +838,16 @@ __must_check int __media_pipeline_start(struct media_pad *origin,
> > > > if (link->sink != pad && link->source != pad)
> > > > continue;
> > > >
> > > > - /* Record if the pad has links and enabled links. */
> > > > - if (link->flags & MEDIA_LNK_FL_ENABLED)
> > > > - has_enabled_link = true;
> > > > -
> > > > /*
> > > > - * Validate the link if it's enabled and has the
> > > > - * current pad as its sink.
> > > > + * Ensure the link is enabled and if so, record
> > > > + * it. Proceed to the next link if the current pad isn't
> > > > + * the sink pad of the link.
> > >
> > > You can reflow this;
> > >
> > > * Ensure the link is enabled and if so, record it.
> > > * Proceed to the next link if the current pad isn't the
> > > * sink pad of the link.
> > >
> > > but I find the new comment confusing.
> > >
> > > I would keep the code as-is, I think it's more readable, and the
> > > compiler will deal with optimization.
> >
> > There's only one flag to test and I can't see how it'd be more readable to
> > do that twice in the same location. I can keep the comment as-is if you
> > prefer that.
>
> The flag is tested twice for two different purposes, with two separate
> comments. A subsequent patch in the series further modifies this code,

I believe the code wouldn't have looked like this if it wasn't written over
several iterations. In other words, it was in a need of a cleanup this
patch does. :-)

I.e. first see if a link isn't enabled and if so, bail out. Otherwise the
rest will proceed from there, the link being enabled being a condition for
that, including marking that there was an enabled link.

> and makes things less readable as it inserts code in the middle while
> still keeping a single comment to explain the multiple operations. I'm

If you look at the resulting media_pipeline_validate_one(), the flow is
entirely reasonable and would not benefit from testing the enabled flag
twice. I can add better comments on why what is being done is done.

> sure we could expand the comment to explain things in more details (and
> bikeshed how to do so), but I think it will still be less readable than
> keeping those two steps separate with one comment each.
>
> > > > */
> > > > if (!(link->flags & MEDIA_LNK_FL_ENABLED))
> > > > continue;
> > > >
> > > > + has_enabled_link = true;
> > > > +
> > > > if (link->sink != pad)
> > > > continue;
> > > >
>

--
Regards,

Sakari Ailus