Re: [PATCH v4] media: ov5640: select the MIPI lane mode from the endpoint lane count

Hi Hans,

Sorry about the name mix-up. I'll make sure to credit this to Hans de

Goede in the next version.

> Sorry that I missed this previously, but should this not also

> write the correct lane-count right away, rather then waiting

> with fixing the line-count for 1 lane configs till we hit

> ov5640_set_stream_mipi() ?

I intentionally kept the fix to the streaming path. The value written

during power-up is overwritten when streaming starts, before any frames

are produced, so it should not affect the actual stream. I also wanted

to keep the Fixes: patch as minimal as possible to make backporting

easier.

Would you prefer me to extend the patch to program the correct lane

count during power-up as well?

Regards,

Jason


<[email protected]> 於 2026年8月11日週二 下午7:10寫道：
>
> Hi,
>
> On 11-Aug-26 11:17, Jason Yang via B4 Relay wrote:
> > From: Jason Yang <[email protected]>
> >
> > ov5640_set_stream_mipi() always programs IO_MIPI_CTRL00 with 0x45,
> > which selects the two data lane mode: the number of data lanes
> > described in the devicetree endpoint only feeds the sensor's clock
> > tree computations, so a module wired with one data lane starts
> > streaming in two lane mode and the receiver never assembles a
> > frame.
> >
> > Take the lane mode from the endpoint instead. The field encodes
> > the lane count directly - 001 for one lane, 010 for two - per the
> > current sensor manual (version 2.33). The 2.03 manual documented
> > 000/001 for one/two lanes; OmniVision corrected the table in
> > version 2.1, which is why the long-standing comment here found 001
> > unusable for two lanes and validated 010 instead.
> >
> > The power-up path also programs a two data lane mode, but that
> > value is overwritten when streaming starts, so it is left alone.
> >
> > Tested with a single data lane module on an i.MX8MP board
> > (imx-mipi-csis receiver), where the unpatched value produces no
> > frames at all, and on an RK3588 board.
> >
> > Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver")
> > Cc: [email protected]
> > Signed-off-by: Jason Yang <[email protected]>
> > Assisted-by: Claude:claude-opus-5
> > ---
> > Changes in v4:
> > - Keep the lane count programmed also while the interface is
> >   disabled, toggling only the enable bits (Hans Verkuil); with one
> >   data lane the disable value becomes 0x20 instead of 0x40.
> > - Link to v3: https://lore.kernel.org/r/[email protected]
> >
> > Changes in v3:
> > - Inline the lane count in the write instead of going through a
> >   local variable (Sakari Ailus).
> > - Link to v2: https://lore.kernel.org/r/[email protected]
> >
> > Changes in v2:
> > - Compute the register value from the lane count in the single
> >   write instead of branching on it (Sakari Ailus), with the count
> >   in a local variable to stay within 80 columns; the programmed
> >   values are unchanged, 0x25 for one lane and 0x45 for two.
> > - Request a normal stable backport rather than opting out of
> >   AUTOSEL (Sakari Ailus).
> > - Drop the quotes around the function name in the reference from
> >   ov5640_set_power_mipi() (Sakari Ailus).
> > - Link to v1: https://lore.kernel.org/r/[email protected]
> > ---
> >  drivers/media/i2c/ov5640.c | 18 ++++++------------
> >  1 file changed, 6 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 8deb5f5501fa..a99e4edb6a75 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -1831,22 +1831,16 @@ static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> >       /*
> >        * Enable/disable the MIPI interface
> >        *
> > -      * 0x300e = on ? 0x45 : 0x40
> > -      *
> > -      * FIXME: the sensor manual (version 2.03) reports
> > -      * [7:5] = 000  : 1 data lane mode
> > -      * [7:5] = 001  : 2 data lanes mode
> > -      * But this settings do not work, while the following ones
> > -      * have been validated for 2 data lanes mode.
> > -      *
> > -      * [7:5] = 010  : 2 data lanes mode
> > +      * [7:5]        : data lane count, 001 for one lane and 010 for two,
> > +      *                per version 2.33 of the sensor manual
> >        * [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);
> > +                            sensor->ep.bus.mipi_csi2.num_data_lanes << 5 |
> > +                            (on ? 0x05 : 0x0));
> >       if (ret)
> >               return ret;
> >
> > @@ -2535,8 +2529,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
> >        * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> >        *
> >        * 0x300e = 0x40
> > -      * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > -      *                "ov5640_set_stream_mipi()")
> > +      * [7:5] = 010  : 2 data lanes mode (see the note in
> > +      *                ov5640_set_stream_mipi())
> >        * [4] = 0      : Power up MIPI HS Tx
> >        * [3] = 0      : Power up MIPI LS Rx
> >        * [2] = 1      : MIPI interface enabled
>
> Sorry that I missed this previously, but should this not also
> write the correct lane-count right away, rather then waiting
> with fixing the line-count for 1 lane configs till we hit
> ov5640_set_stream_mipi() ?
>
> Regards,
>
> Hans
>
>