Re: [PATCH v7 2/2] v4l: cadence: Add Cadence MIPI-CSI2 TX driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> 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? :)

> 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

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux