Re: [PATCH 1/5] media: adv748x: Rework reset procedure

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

Thank you for the patch.

On Wednesday, 12 December 2018 12:13:44 EET Kieran Bingham wrote:
> On 12/12/2018 08:16, jacopo mondi wrote:
> > On Tue, Dec 11, 2018 at 11:52:03PM +0000, Kieran Bingham wrote:
> >> On 11/12/2018 15:16, Jacopo Mondi wrote:
> >>> Re-work the chip reset procedure to configure the CP (HDMI) and SD (AFE)
> >>> cores before resetting the MIPI CSI-2 TXs.
> >>> 
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>> 
> >>>  drivers/media/i2c/adv748x/adv748x-core.c | 24 ++++++++++--------------
> >>>  1 file changed, 10 insertions(+), 14 deletions(-)
> >>> 
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> b/drivers/media/i2c/adv748x/adv748x-core.c index
> >>> d94c63cb6a2e..5495dc7891e8 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> >>> @@ -353,9 +353,8 @@ static const struct adv748x_reg_value
> >>> adv748x_sw_reset[] = {>>> 
> >>>  	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> >>>  
> >>>  };
> >>> 
> >>> -/* Supported Formats For Script Below */
> >>> -/* - 01-29 HDMI to MIPI TxA CSI 4-Lane - RGB888: */
> >> 
> >> Is this information redundant ? (CSI-4Lane, RGB888 configuration?)
> > 
> > The CSI-2 data lane configuration has been break out from this table
> > by Niklas' patches. I've tried also moving the format configuration
> > out of this, but I haven't sent that change. The HDMI video direction
> > is now handled at link setup time, so I guess the only relevant
> > information is about the RGB888 format configured on the CP backend.
> > I'll keep that.
> 
> Thanks for the clarification.

Sounds good to me. With this change,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >>> -static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
> >>> +/* Initialize CP Core. */
> >>> +static const struct adv748x_reg_value adv748x_init_hdmi[] = {
> >> 
> >> While we're here - is there much scope - or value in changing these
> >> tables to functions with parameters using Niklas' adv748x_write_check() ?
> >> 
> >> The suggestion only has value if there are parameters that we would need
> >> to configure. So it might be reasonable to leave these tables.
> > 
> > Right now I don't see much value in that. I would prefer breaking out
> > the format configuration from this static tables, but that's for
> > later.
> 
> Perfect - I agree - doesn't need to happen in this patch.
> 
> If the format configuration can be broken out from the table later then
> that's great news.

I think it will make sense to do so, yes.

> >> A general Ack on renaming to the function instead of the
> >> TX/configuration though - as that makes the purpose clearer.
> >> 
> >>>  	/* Disable chip powerdown & Enable HDMI Rx block */
> >>>  	{ADV748X_PAGE_IO, 0x00, 0x40},
> >>> 
> >>> @@ -399,10 +398,8 @@ static const struct adv748x_reg_value
> >>> adv748x_init_txa_4lane[] = {>>> 
> >>>  	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> >>>  
> >>>  };
> >>> 
> >>> -/* 02-01 Analog CVBS to MIPI TX-B CSI 1-Lane - */
> >>> -/* Autodetect CVBS Single Ended In Ain 1 - MIPI Out */
> >>> -static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
> >>> -
> >> 
> >> Same comments as above really :)
> > 
> > I'll see what I can keep.
> > 
> > Thanks
> > 
> >   j
> >   
> >>> +/* Initialize AFE core. */
> >>> +static const struct adv748x_reg_value adv748x_init_afe[] = {
> >>> 
> >>>  	{ADV748X_PAGE_IO, 0x00, 0x30},	/* Disable chip powerdown Rx */
> >>>  	{ADV748X_PAGE_IO, 0xf2, 0x01},	/* Enable I2C Read Auto-Increment */
> >>> 
> >>> @@ -445,19 +442,18 @@ static int adv748x_reset(struct adv748x_state
> >>> *state)
> >>> 
> >>>  	if (ret < 0)
> >>>  	
> >>>  		return ret;
> >>> 
> >>> -	/* Init and power down TXA */
> >>> -	ret = adv748x_write_regs(state, adv748x_init_txa_4lane);
> >>> +	/* Initialize CP and AFE cores. */
> >>> +	ret = adv748x_write_regs(state, adv748x_init_hdmi);
> >>> 
> >>>  	if (ret)
> >>>  	
> >>>  		return ret;
> >>> 
> >>> -	adv748x_tx_power(&state->txa, 1);
> >>> -	adv748x_tx_power(&state->txa, 0);
> >>> -
> >>> -	/* Init and power down TXB */
> >>> -	ret = adv748x_write_regs(state, adv748x_init_txb_1lane);
> >>> +	ret = adv748x_write_regs(state, adv748x_init_afe);
> >>> 
> >>>  	if (ret)
> >>>  	
> >>>  		return ret;
> >>> 
> >>> +	/* Reset TXA and TXB */
> >>> +	adv748x_tx_power(&state->txa, 1);
> >>> +	adv748x_tx_power(&state->txa, 0);
> >>> 
> >>>  	adv748x_tx_power(&state->txb, 1);
> >>>  	adv748x_tx_power(&state->txb, 0);
> >>> 
> >>> --
> >>> 2.7.4
> >> 
> >> --
> >> Regards
> >> --
> >> Kieran


-- 
Regards,

Laurent Pinchart