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

Jakub Kostiw
2025-05-06T18:34:00Z

Hi Cosmin

Awesome work. The initiative to establish a common framework for GMSL
devices is a great idea.

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)

The same goes for MAX96724 equivalent macro:

> +#define MAX96724_BACKTOP22(x) (0x415 * (x) * 0x3)

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;

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.

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