Re: [RFC PATCH v2 12/16] media: i2c: add Maxim GMSL2/3 serializer and deserializer drivers

[Cosmin Tanislav <demonsingur@gmail.com>]

On 5/6/25 9:33 PM, Jakub Kostiw wrote:
> Hi Cosmin
> 
> Awesome work. The initiative to establish a common framework for GMSL 
> devices is a great idea.
> 

Hi! Thanks for the feedback.

> I believe that we have found few bugs:
> 
>> +#define MAX9296A_BACKTOP22(x)            (0x31d * (x) * 0x3)
> 
> The first multiplication is wrong and should be replaced with addition:
> 
> +#define MAX9296A_BACKTOP22(x)            (0x31d + (x) * 0x3)
> 

I'm aware of this issue and had it fixed locally, just haven't submitted
a new version yet.

> The same goes for MAX96724 equivalent macro:
> 
>> +#define MAX96724_BACKTOP22(x)            (0x415 * (x) * 0x3)
> 

Although I haven't noticed that it applied to MAX96724 too, thanks.

> In MAX96714 driver there is an issue with setting up lane-polarities.
> 
>> +static const struct max9296a_chip_info max96714_info = {
>> +    .max_register = 0x5011,
>> +    .set_pipe_stream_id = max96714_set_pipe_stream_id,
>> +    .set_pipe_enable = max96714_set_pipe_enable,
>> +    .set_pipe_tunnel_enable = max96714_set_pipe_tunnel_enable,
>> +    .phys_configs = {
>> +        .num_configs = ARRAY_SIZE(max96714_phys_configs),
>> +        .configs = max96714_phys_configs,
>> +    },
>> +    .polarity_on_physical_lanes = true,
>> +    .supports_phy_log = true,
>> +    .adjust_rlms = true,
>> +    .num_pipes = 1,
>> +    .pipe_hw_ids = { 1 },
>> +    .num_phys = 1,
>> +    .phy_hw_ids = { 1 },
>> +    .num_links = 1,
>> +};
> 
> In order to make thing work we had to set
> 
>> +    .polarity_on_physical_lanes = true,
> 
> To false. So this field is either improperly set for MAX96714, or 
> handling of this case is wrong:
> 
>> +        if (priv->info->polarity_on_physical_lanes)
>> +            map = phy->mipi.data_lanes[i];
>> +        else
>> +            map = i;
> 

MAX96714 has different value meanings for the polarity register
(compared to MAX9296A). On MAX96714, the bits represent the
polarity of the hardware lane (not taking mapping into account).
On MAX9296A, mapping is taken into account by hardware, so
the polarity can be retrieved directly. See the comments surrounding
that piece of code. It's entirely possible that I was wrong when
writing this code, but this was cross-checked with the datasheet
and the GMSL SerDes GUI. It's not out of the question that any of
these could be wrong.

Are you setting a specific polarity on the lanes? I've validated
MAX96714 (after the upstream submission) myself and it works.

> Upon mentioned changes we have successfully tested two GMSL2 
> combinations on Raspberry 5 platform:
> 
> 1. MAX96724 + MAX96717 (only 2 MIPI-CSI2 lanes with single camera due to 
> hardware limitations)
> 
> 2. MAX96714 + MAX96717
> 
> We have also been wondering about these registers:
> 
>> +#define MAX9296A_DPLL_0(x)            (0x1c00 + ((x) == 0 ? 1 : 2) * 
>> 0x100)
>> +#define MAX96724_DPLL_0(x)            (0x1c00 + (x) * 0x100)
> 
> There are writes to these addresses but we could not find any reference 
> to these in either MAX96714 or MAX96724 datasheets.
> 

These registers are not mentioned in the datasheet of MAX96714 or
MAX96724, as they're considered "hidden". You can look at MAX96716A
datasheet for a description.

> Are You willing to add support for mapping MIPI-CSI2 lanes? This is a 
> very convenient feature which comes in handy during HW design.
> 

This should already be implemented by using different numbers in
data-lanes property in devicetree.