Re: [PATCH v3 1/2] media: ov5640: Re-work MIPI startup sequence

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 3377 bytes --]

Hello Loic,

On Thu, Sep 06, 2018 at 10:13:53AM +0200, Loic Poulain wrote:
> On 6 September 2018 at 09:48, jacopo mondi <jacopo@jmondi.org> wrote:
> > Hello Loic,
> >    thanks for looking into this
> >
> > On Tue, Sep 04, 2018 at 07:22:50PM +0200, Loic Poulain wrote:
> >> Hi Jacopo,
> >>
> >> > -       ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
> >> > -                            on ? 0 : BIT(5));
> >> > -       if (ret)
> >> > -               return ret;
> >> > -       ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
> >> > -                              on ? 0x00 : 0x70);
> >> > +       /*
> >> > +        * Enable/disable the MIPI interface
> >> > +        *
> >> > +        * 0x300e = on ? 0x45 : 0x40
> >> > +        * [7:5] = 001  : 2 data lanes mode
> >>
> >> Does 2-Lanes work with this config?
> >> AFAIU, if 2-Lanes is bit 5, value should be 0x25 and 0x20.
> >>
> >
> > Yes, confusing.
> >
> > The sensor manual reports
> > 0x300e[7:5] = 000 one lane mode
> > 0x300e[7:5] = 001 two lanes mode
> >
> > Although this configuration works with 2 lanes, and the application
> > note I have, with the suggested settings for MIPI CSI-2 2 lanes
> > reports 0x40 to be the 2 lanes mode...
> >
> > I used that one, also because the removed entry from the settings blob
> > is:
> > -       {0x300e, 0x45, 0, 0}, {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> > +       {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> >
> > So it was using BIT(6) already.
>
> Yes, it was setting BIT(6) from static config and BIT(5) from the
> ov5640_set_stream_mipi function. In your patch you don't set
> BIT(5) anymore.

I've resumed looking into this series.
Just FYI, the snippet you refer to is:

-       ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
-                            on ? 0 : BIT(5));
-       if (ret)
-               return ret;
-       ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
-                              on ? 0x00 : 0x70);
+       /*
+        * Enable/disable the MIPI interface
+        *
+        * 0x300e = on ? 0x45 : 0x40
+        * [7:5] = 001  : 2 data lanes mode
+        * [4] = 0      : Power up MIPI HS Tx
+        * [3] = 0      : Power up MIPI LS Rx
+        * [2] = 1/0    : MIPI interface enable/disable
+        * [1:0] = 01/00: FIXME: 'debug'
+        */
+       ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
+                              on ? 0x45 : 0x40)

As you can see (it took me a while) the old code was indeed setting
BIT(5) as you mentioned, but on OV5640_REG_MIPI_CTRL00 (0x4800) and
not on OV5640_REG_IO_MIPI_CTRL00 (0x300e) as mine does. So the lane
configuration mode was set to 0x45 and never changed later.
>
> So it's not clear to me why it is still working, and the datasheet does
> not help a lot on this (BIT(6) is for debug modes).
> FYI I tried with BIT(5) only but it does not work (though I did not
> investigate a lot).

I'll keep BIT(6) set, point out the discrepancy with the datasheet,
and point out it has been tested with 2 lanes, until someone can
confirm it works with 1 lane too.

Thanks
   j

>
> > Anyway, it works for me with 2 lanes (and I assume Steve), you have tested
> > too, with how many lanes are you working with?
> >
> > Anyway, a comment there might be nice to have... Will add in next
> > version
>
> Definitely.
>
> Regards,
> Loic

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]