On Thu, 2017-09-21 at 12:41 +0100, Dave Stevenson wrote: > Hi Philipp > > On 21 September 2017 at 11:24, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> > wrote: [...] > > + if (state->csi_lanes_in_use > 4) > > One could suggest > if (state->csi_lanes_in_use > state->bus.num_data_lanes) > here. Needing to use more lanes than are configured is surely an > error, although that may be detectable at the other end. See below > too. True. The OF parser could be improved to make sure that num_data_lanes <= 4, and also that all lanes are in the correct order. [...] > > +/* > > + * Number of lanes in use, 0 == use all available lanes (default) > > + * > > + * This is a temporary fix for devices that need to reduce the number of active > > + * lanes for certain modes, until g_mbus_config() can be replaced with a better > > + * solution. > > + */ > > +#define V4L2_MBUS_CSI2_LANE_MASK (3 << 10) > > I know this was Hans' suggested define, but are we saying 4 lanes is > not a valid value? If it is then the mask needs to be at least (7 << > 10). 0 must map to "all lanes" for backwards compatibility, but I see no reason why we shouldn't add another bit here to support reporting 4 lanes explicitly. > 4 lanes is not necessarily "all available lanes". Correct. > - There are now CSI2 devices supporting up to 8 lanes (although > V4L2_FWNODE_CSI2_MAX_DATA_LANES limits you to 4 at the moment). So how about we just add two bits, then? #define V4L2_MBUS_CSI2_LANE_MASK (0xf << 10) > - Or you could have 2 lanes configured in DT and ask TC358743 for (eg) > 1080P60 UYVY at 594Mbps (needs 4 lanes) which passes the current logic > in the TC358743 driver and would return 0, when it is actually going > to use 4 lanes. That could be classed as a driver bug though. Right, the driver shouldn't try to start streaming at all if it knows that the available CSI-2 bandwidth will be exceeded. > My view is that if a driver is going to report how many lanes to use > then it should always report it explicitly. The default 0 value should > only be used for devices that will never change it from the DT > settings. Perhaps others disagree > > Otherwise the patch works for me. I'm fine with changing this as you suggest. regards Philipp
