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

[johannes . goede]

Hi Jason,

On 11-Aug-26 14:32, 楊智成 wrote:
> 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?

Yes please, since the FIXME comment was present in both places it
seems consistent to me to also actually fix the issue in both places.

Regards,

Hans






> <johannes.goede@xxxxxxxxxxxxxxxx> 於 2026年8月11日週二 下午7:10寫道：
>>
>> Hi,
>>
>> On 11-Aug-26 11:17, Jason Yang via B4 Relay wrote:
>>> From: Jason Yang <jason98166@xxxxxxxxx>
>>>
>>> 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: stable@xxxxxxxxxxxxxxx
>>> Signed-off-by: Jason Yang <jason98166@xxxxxxxxx>
>>> 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/20260811-ov5640-1lane-v1-v3-1-ae476eaf5024@xxxxxxxxx
>>>
>>> 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/20260811-ov5640-1lane-v1-v2-1-65205ae86feb@xxxxxxxxx
>>>
>>> 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/20260811-ov5640-1lane-v1-v1-1-79699457ce13@xxxxxxxxx
>>> ---
>>>  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
>>
>>