Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization

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

 



Hi Laurent,

Thanks for your feedback.

On 2018-11-02 13:43:21 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Friday, 2 November 2018 12:38:34 EET Niklas Söderlund wrote:
> > On 2018-10-05 01:36:11 +0300, Laurent Pinchart wrote:
> > > On Thursday, 4 October 2018 23:41:35 EEST Niklas Söderlund wrote:
> > > > From: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > > > 
> > > > Reorder the initialization order of registers to allow for refactoring.
> > > > The move could have been done at the same time as the refactoring but
> > > > since the documentation about some registers involved are missing do it
> > > > separately.
> > > > 
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > > > ---
> > > > 
> > > >  drivers/media/i2c/adv748x/adv748x-core.c | 12 +++++++-----
> > > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > > > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > > > 6854d898fdd1f192..721ed6552bc1cde6 100644
> > > > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > > > @@ -383,8 +383,6 @@ static const struct adv748x_reg_value
> > > > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_IO, 0x0c, 0xe0},	/* Enable
> > > > LLC_DLL & Double LLC Timing */ {ADV748X_PAGE_IO, 0x0e, 0xdd},	/*
> > > > LLC/PIX/SPI PINS TRISTATED AUD */
> > > > 
> > > > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > > > -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > > > 
> > > >  	{ADV748X_PAGE_TXA, 0xdb, 0x10},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
> > > > 
> > > > @@ -392,6 +390,9 @@ static const struct adv748x_reg_value
> > > > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI
> > > > Required Write */
> > > > 
> > > >  	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > > > 
> > > > +	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > > > +	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > > > +
> > > > 
> > > >  	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > > > 
> > > > @@ -435,17 +436,18 @@ static const struct adv748x_reg_value
> > > > adv748x_init_txb_1lane[] = { {ADV748X_PAGE_SDP, 0x31, 0x12},	/* ADI
> > > > Required Write */
> > > > 
> > > >  	{ADV748X_PAGE_SDP, 0xe6, 0x4f},  /* V bit end pos manually in NTSC 
> */
> > > > 
> > > > -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > > > -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> > > > 
> > > >  	{ADV748X_PAGE_TXB, 0xd2, 0x40},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXB, 0xc4, 0x0a},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXB, 0x71, 0x33},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXB, 0x72, 0x11},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXB, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > > > 
> > > > +
> > > > +	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > > > +	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> > > > +
> > > 
> > > This is pretty hard to review, as there's a bunch of undocumented register
> > > writes. I think the first write is safe, as the tables are written
> > > immediately following a software reset, and the default value of the
> > > register is 0x81 (CSI-TX disabled, 1 lane). The second write, however,
> > > enables usage of the computed DPHY parameters, and I don't know whether
> > > the undocumented register writes in-between may interact with that.
> > 
> > I agree it's hard to grasp all implications with undocumented registers
> > involved. That is why I choose to do it in a separate commit so if
> > regressions are found it could be bisectable to this change.
> > 
> > > That being said, this change enables further important refactoring, so I'm
> > > tempted to accept it. I assume you've tested it and haven't noticed a
> > > regression. The part that still bothers me in particular is that the write
> > > to register 0xf0 just above this takes the DPHY out of power down
> > > according to the datasheet, and I wonder whether at that point the DPHY
> > > might not react to that information. Have you analyzed the power-up
> > > sequence in section 9.5.1 of the hardware manual ? I wonder whether the
> > > dphy_pwdn shouldn't be handled in the power up and power down sequences,
> > > which might involve also moving the above four (and five for TXA)
> > > undocumented writes to the power up sequence as well.
> > 
> > I looked at the documentation and ran lots of tests based on this change
> > and noticed no change in behavior.
> 
> As a last test, could you try programming completely invalid values to the 
> undocumented registers before taking the DPHY out of power down ? I'm worried 
> that things might work just because the registers happen to contain acceptable 
> values when the DPHY is powered up, and that it might break later because the 
> sequence of operations resulting from starting and stopping the video streams 
> in different configurations would end up taking the DPHY out of power down 
> with invalid values in those registers.

I did the test you describe above and the ordering have no effect on the 
end result.

However looking at the problem with fresh eyes I realised that the more 
correct option is to take your first suggestion and bring the 
undocumented registers into the power up function and that is what I 
will do in v3. Thanks for pointing this out!

> 
> > >>  	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> > >>  	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
> > >>  	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > >> -
> > >>  	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> > >>  	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> > >>  	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Regards,
Niklas Söderlund



[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