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-10-05 01:36:11 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> 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.

> 
> >  	{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