Re: [PATCH v1.1 4/4] ti-vpe: Parse local endpoint for properties, not the remote one

[Sakari Ailus <sakari.ailus@linux.intel.com>]

On Tue, Mar 05, 2019 at 11:38:42AM -0600, Benoit Parrot wrote:
> Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 18:32:40 +0200]:
> > Hi Benoit,
> > 
> > On Tue, Mar 05, 2019 at 08:34:09AM -0600, Benoit Parrot wrote:
> > > Sakari,
> > > 
> > > Thank you for the patch.
> > > 
> > > Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 16:02:24 +0200]:
> > > > ti-vpe driver parsed the remote endpoints for properties but ignored the
> > > > local ones. Fix this by parsing the local endpoint properties instead.
> > > 
> > > I am not sure I understand the logic here.  For CSI2 sensor as far as I
> > > understand the lane mapping (clock and data) is driven from the sensor
> > > side. The bridge driver (in this case CAL) needs to setup the receiver side
> > > based on what the sensor (aka remote endpoint) can provide.
> > > 
> > > I failed to see how this fixes things here.
> > > 
> > > Are you suggesting that sensor relevant properties be set (and effectively
> > > duplicated) on the bridge/receiver side?
> > 
> > Yes. The endpoint configuration in general is local to the device and
> > should not be accessed from other device drivers.
> > 
> > The lane mapping, for instance, is specific to a given device --- and may
> > differ even between for two connected endpoints. It's used to reorder the
> > PHY lanes (if the device supports that). Same goes for the clock lane.
> 
> I did not see omap3isp having lane reorder capability, but I guess it would
> be possible for instance, that a sensor uses clock lane 0 and data lane 1
> & 2 but the way it is wired on the board makes it that the receiver would see
> sensor lane 0 on device lane 2 and so on... Not sure why you would wire it
> up that way but who knows...

I presume the feature is there to ease PCB design.

> 
> > 
> > See e.g. arch/arm/boot/dts/omap3-n9.dts .

	     ^

There it is.

> > 
> > > 
> > > Some sensor can and do handle multiple data lanes configuration so the
> > > sensor driver also needs to use those properties at probe time, duplicating
> > > the lane data is just asking for a mismatch to happen, no?
> > 
> > It's a different configuration on the sensor side. We currently have no
> > checks in place to verify that the two would match. I haven't heard of this
> > would have really been a problem though.
> 
> I had just never thought about this cases, to me a single source of
> information is better than 2. But anyhow I guess I'll have to update all of
> my relevant dts files in the near future.

Do you have in-kernel dts files using this? I presume the driver should
then figure out whether the local endpoint has a configuration and if it
doesn't, then look it up from the remote one. Otherwise old dts binaries
will break. :-(

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com