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@xxxxxxxxxx>
> >>> ---
> >>>
> >>> 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@xxxxxxxxxxxxxxxx>
> >>> -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
