Re: [PATCH v7 2/2] media: i2c: Add MAX9286 driver

[Hyun Kwon <hyun.kwon@xilinx.com>]

Hi Jacobo,

On Sun, 2020-03-15 at 16:15:17 -0700, Jacopo Mondi wrote:
> Hello Hyun, Kieran,
>    it's great you are looking into this!
> 
> On Fri, Feb 28, 2020 at 10:13:04AM -0800, Hyun Kwon wrote:
> > Hi Kieran,
> >
> > Thanks for sharing a patch.
> >
> [snip]
> 
> > > > +
> > > > +	/*
> > > > +	 * Set the I2C bus speed.
> > > > +	 *
> > > > +	 * Enable I2C Local Acknowledge during the probe sequences of the camera
> > > > +	 * only. This should be disabled after the mux is initialised.
> > > > +	 */
> > > > +	max9286_configure_i2c(priv, true);
> > > > +
> > > > +	/*
> > > > +	 * Reverse channel setup.
> > > > +	 *
> > > > +	 * - Enable custom reverse channel configuration (through register 0x3f)
> > > > +	 *   and set the first pulse length to 35 clock cycles.
> > > > +	 * - Increase the reverse channel amplitude to 170mV to accommodate the
> > > > +	 *   high threshold enabled by the serializer driver.
> > > > +	 */
> > > > +	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> > > > +	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
> > > > +		      MAX9286_REV_AMP_X);
> > > > +	usleep_range(2000, 2500);
> > > > +
> > > > +	/*
> > > > +	 * Enable GMSL links, mask unused ones and autodetect link
> > > > +	 * used as CSI clock source.
> > > > +	 */
> > > > +	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> > > > +	max9286_write(priv, 0x0b, link_order[priv->route_mask]);
> > > > +	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
> > > > +
> > > > +	/*
> > > > +	 * Video format setup:
> > > > +	 * Disable CSI output, VC is set according to Link number.
> > > > +	 */
> > > > +	max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
> > > > +
> > > > +	/* Enable CSI-2 Lane D0-D3 only, DBL mode, YUV422 8-bit. */
> > > > +	max9286_write(priv, 0x12, MAX9286_CSIDBL | MAX9286_DBL |
> > > > +		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> > > > +		      MAX9286_DATATYPE_YUV422_8BIT);
> > > > +
> > > > +	/* Automatic: FRAMESYNC taken from the slowest Link. */
> > > > +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > > +		      MAX9286_FSYNCMETH_AUTO);
> > > > +
> > > > +	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> > > > +	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
> > > > +		      MAX9286_HVSRC_D14);
> >
> 
> I agree we need to make some of these configurable, we need that too
> to handle both rdacm20 and 21.
> 
> > Some of these configs in fact need some handshake between serializer and
> > de-serializer. For example, I had to invert vsync in serializer [3] to make
> > it work with this patch.
> 
> Quickly skamming through the datasheet I'm surprised there is no way
> to control the vsync input polarity and you have to go through the
> crossbar :) Oh well, I think this could be well controlled through a
> device property of the serializer, what do you think?
> 
> We have standard properties to control vsync and hsync polarities, but
> they're usually used for output signals, and I would reserve them for
> that future usage.. maybe a custom property to control the input vsync
> and hsync polarities would do...

Thanks for sharing pointers. These are not really hardened - static properties
so I'm not sure if device tree is the right place. I was thinking something
more similar to phy_configure_opts_mipi_dphy in phy subsystem.

> 
> >
> > In addition to that, I need a couple of additional programmings on max9286
> > (registers 0x0 to 0x63/0x64- disable overlap window and 0xf4 to 0x1c which
> > oddly change reserved bits) to get frames. The datasheet doesn't explain
> > enough for me to understand. I'm talking to Maxim to get more details and
> > see if those can be handled by serilizer driver.
> 
> I would be really interested in knowing what's the overlap window control
> about... it's very little detailed in the manual, I agree :) 0xf4 is
> not even documented in my version. I assume it's something related to
> fsync sync locking (Fig. 46) as I failed to achieve it without that
> setting. How did it fail for you ?
> 

I received one doc "Frame Synchronization Block" that explains the overlap
window in more details. It's essentially a window between camera vsync and
frame sync. If those 2 don't happen within the window, it errors out. So it
gives a validation check, but may not work depending on the vsync patterm from
camera or the window should be bigger, which makes the validation less
useful in my opinion.

I believe 0x1c has something to do with BWS as mentioned in my previous reply.
The max9286 on my board sets BWS pin as open, and it makes the bandwidth
to be 27 bit mode by default. So writing 0xf4 to 0x1c register sets to 24 bit
mode (while 0xf6 = 27 bit mode). But I didn't hear back from Maxim regarding 
this yet. And unfortunately, I couldn't make it work with 27 bit mode on both
max9286 and max96705.

So this and similar properties may also be something that can be handled by
the negotiating call mentioned above, while the configuraton through external
pins can be modeled as device tree properties?

Thanks,
-hyun

> On how to control overlap window a integer would do ? Setting it to 0
> disables it, so we could use a boolean property for convenience..
> 
> Not knowing what it does I would be careful.. I think we should
> actualy require a mandatory property, so all current dts select their
> behaviour explicitly. If we later find out what it does we could
> define a default behaviour by defining a boolean property. New dts
> simpler and old dts still happy. What do you think ?
> 
> >
> > In a longer term, it'd be nice if such alignment between serializer and
> > de-serializer is handled in more scalable way.
> >
> 
> Indeed :)
> 
> Thanks
>   j
> 
> > Thanks,
> > -hyun
> >
> > [1] https://github.com/xlnx-hyunkwon/linux-xlnx/commit/3bd2dded834492f4ee89e370c22877b97c2e106e
> > [2] https://github.com/xlnx-hyunkwon/linux-xlnx/commit/fb0ad7fd699d90d6bbc78fc55dd98639389cfc5b
> > [3] https://github.com/xlnx-hyunkwon/linux-xlnx/commit/fe0b33b174b2850bf0bb25b3a367319127ae12ee
> >