Hi Maxime,
On 2018-04-03 15:48:59 +0200, Maxime Ripard wrote:
> Hi Niklas,
>
> On Thu, Mar 29, 2018 at 02:35:34PM +0200, Niklas Söderlund wrote:
> > > + /*
> > > + * Create a static mapping between the CSI virtual channels
> > > + * and the input streams.
> > > + *
> > > + * This should be enhanced, but v4l2 lacks the support for
> > > + * changing that mapping dynamically at the moment.
> > > + *
> > > + * We're protected from the userspace setting up links at the
> > > + * same time by the upper layer having called
> > > + * media_pipeline_start().
> > > + */
> > > + list_for_each_entry(link, &entity->links, list) {
> >
> > I wonder is this list_for_each_entry() really needed? Can't you simply
> > iterate over all sink pads as with the loop bellow but drop the pad ==
> > link->sink check? Maybe I'm missing something.
>
> This was a review made by Sakari here:
> https://patchwork.linuxtv.org/patch/44422/
>
> The idea is that we need to know if the pad is enabled, and as far as
> I know this information is only stored at the link level, not at the
> pad level.
Ahh I see, you are correct.
>
> > Apart from this and the small nit-picks (one more bellow) I think this
> > patch is fine. Once I understand this I be happy to add my tag to this
> > change, great work!
>
> Is this a reviewed by? :)
Yes, I now understand what I did not before :-) Feel free to add my
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
>
> > I also think you shall consider to add a MAINTAINERS record for the RX
> > and TX drivers. Maybe one entry for both drivers as they live in the
> > same directory but I think one of the two should add it :-)
>
> Right, I'll do it, thanks!
> Maxime
>
> --
> Maxime Ripard, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com
--
Regards,
Niklas Söderlund
