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

[Cosmin Tanislav <demonsingur@gmail.com>]

On 5/7/25 10:28 AM, Jakub Kostiw wrote:
>> Can you revert the change you made to polarity_on_physical_lanes, and
>> try the following?
>>
>> diff --git a/drivers/media/i2c/maxim-serdes/max9296a.c b/drivers/ 
>> media/i2c/maxim-serdes/max9296a.c
>> index f48f5b68a750..dea0518fd790 100644
>> --- a/drivers/media/i2c/maxim-serdes/max9296a.c
>> +++ b/drivers/media/i2c/maxim-serdes/max9296a.c
>> @@ -474,7 +474,7 @@ static int max9296a_init_phy(struct max_des *des, 
>> struct max_des_phy *phy)
>>                  */
>>
>>                 if (priv->info->polarity_on_physical_lanes)
>> -                       map = phy->mipi.data_lanes[i];
>> +                       map = phy->mipi.data_lanes[i] - 1;
>>                 else
>>                         map = i;
>>
>> data_lanes is 1-based (since 0 is the clock lane), but the bits
>> in register 0x335 start from 0. That means we should adjust the
>> values in data_lanes to be 0-based. 
> 
> I have applied your patch and polarity settings seems to be correct now 
> (based on register contents).
> However, I have came across another issue.
> When I was debugging the driver for MAX96714, before I found out that 
> the issue was with polarity settings, I have commented out calls to 
> MAX9296A_DPLL_0. Probably because I thought it was there by mistake. I 
> totally forgot about that change.
> Before applying your patch I reverted any changes to the driver, so 
> MAX9296A_DPLL_0 writes were back again. Sadly, video stream did not 
> work. So I began to wonder, and just for sake of testing, commented 
> these calls again (added some logs for quick tracing purposes):
> 
> diff --git a/drivers/media/i2c/maxim-serdes/max9296a.c b/drivers/media/ 
> i2c/maxim-serdes/max9296a.c
> index f48f5b68a..b24a8e2d6 100644
> --- a/drivers/media/i2c/maxim-serdes/max9296a.c
> +++ b/drivers/media/i2c/maxim-serdes/max9296a.c
> @@ -391,6 +391,8 @@ static int max9296a_init_phy(struct max_des *des, 
> struct max_des_phy *phy)
>           * PHY1 Lane 1 = D3
>           */
> 
> +       dev_info(priv->dev, "Using  %d lanes", num_data_lanes);
> +
>          /* Configure a lane count. */
>          ret = regmap_update_bits(priv->regmap, 
> MAX9296A_MIPI_TX10(hw_index),
>                                   MAX9296A_MIPI_TX10_CSI2_LANE_CNT,
> @@ -474,7 +476,7 @@ static int max9296a_init_phy(struct max_des *des, 
> struct max_des_phy *phy)
>                   */
> 
>                  if (priv->info->polarity_on_physical_lanes)
> -                       map = phy->mipi.data_lanes[i];
> +                       map = phy->mipi.data_lanes[i] - 1;
>                  else
>                          map = i;
> 
> @@ -484,6 +486,9 @@ static int max9296a_init_phy(struct max_des *des, 
> struct max_des_phy *phy)
>          if (phy->index == 0 && priv->info->phy0_lanes_0_1_on_second_phy)
>                  val = ((val & 0x3) << 2) | ((val >> 2) & 0x3);
> 
> +       dev_info(priv->dev, "Val for MIPI_PHY5 (0_1): %lx", 
> FIELD_PREP(MAX9296A_MIPI_PHY5_PHY_POL_MAP_0_1, val));
> +       dev_info(priv->dev, "Val for MIPI_PHY5 (2_3): %lx", 
> FIELD_PREP(MAX9296A_MIPI_PHY5_PHY_POL_MAP_2_3, val >> 2));
> +
>          ret = regmap_update_bits(priv->regmap, MAX9296A_MIPI_PHY5(index),
> MAX9296A_MIPI_PHY5_PHY_POL_MAP_0_1 |
> MAX9296A_MIPI_PHY5_PHY_POL_MAP_2_3,
> @@ -499,10 +504,10 @@ static int max9296a_init_phy(struct max_des *des, 
> struct max_des_phy *phy)
>                  return ret;
> 
>          /* Put DPLL block into reset. */
> -       ret = regmap_clear_bits(priv->regmap, MAX9296A_DPLL_0(hw_index),
> - MAX9296A_DPLL_0_CONFIG_SOFT_RST_N);
> -       if (ret)
> -               return ret;
> +       //ret = regmap_clear_bits(priv->regmap, MAX9296A_DPLL_0(hw_index),
> +       // MAX9296A_DPLL_0_CONFIG_SOFT_RST_N);
> +       //if (ret)
> +       //      return ret;
> 
>          /* Set DPLL frequency. */
>          ret = regmap_update_bits(priv->regmap, MAX9296A_BACKTOP22(index),
> @@ -519,10 +524,10 @@ static int max9296a_init_phy(struct max_des *des, 
> struct max_des_phy *phy)
>                  return ret;
> 
>          /* Pull DPLL block out of reset. */
> -       ret = regmap_set_bits(priv->regmap, MAX9296A_DPLL_0(index),
> -                             MAX9296A_DPLL_0_CONFIG_SOFT_RST_N);
> -       if (ret)
> -               return ret;
> +       //ret = regmap_set_bits(priv->regmap, MAX9296A_DPLL_0(index),
> +       //                    MAX9296A_DPLL_0_CONFIG_SOFT_RST_N);
> +       //if (ret)
> +       //      return ret;
> 
>          if (dpll_freq > 1500000000ull) {
>                  /* Enable initial deskew with 2 x 32k UI. */
> 
> To my surprise it works this way. I tested this 2 times back and forth. 
> Can these calls really cause some issues?


Can you revert your changes and try this?

diff --git a/drivers/media/i2c/maxim-serdes/max9296a.c 
b/drivers/media/i2c/maxim-serdes/max9296a.c
index dea0518fd790..3bb80fe42a22 100644
--- a/drivers/media/i2c/maxim-serdes/max9296a.c
+++ b/drivers/media/i2c/maxim-serdes/max9296a.c
@@ -519,7 +519,7 @@ static int max9296a_init_phy(struct max_des *des, 
struct max_des_phy *phy)
  		return ret;

  	/* Pull DPLL block out of reset. */
-	ret = regmap_set_bits(priv->regmap, MAX9296A_DPLL_0(index),
+	ret = regmap_set_bits(priv->regmap, MAX9296A_DPLL_0(hw_index),
  			      MAX9296A_DPLL_0_CONFIG_SOFT_RST_N);
  	if (ret)
  		return ret;