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

Sakari Ailus
2026-04-21T10:24:25Z

Hi Laurent,

Thank you for the review.

On Thu, Apr 16, 2026 at 05:35:24PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> 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.

>
> > */
> > if (!(link->flags & MEDIA_LNK_FL_ENABLED))
> > continue;
> >
> > + has_enabled_link = true;
> > +
> > if (link->sink != pad)
> > continue;
> >
>

--
Kind regards,

Sakari Ailus