Re: [PATCH v15 18/22] media: i2c: maxim-serdes: add MAX9296A driver

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect logic for MAX9296A PHY0 hardware quirk breaks lane swapping and polarity inversion.
--

--- Patch [18]: [PATCH v15 18/22] media: i2c: maxim-serdes: add MAX9296A driver ---
commit 8d5f521bc04765a9e0396458e2f5ac75686e72fa
Author: Cosmin Tanislav <[email protected]>

media: i2c: maxim-serdes: add MAX9296A driver

Add a new MAX9296A driver that also supports MAX96714, MAX96714F,
MAX96714R, MAX96716A and MAX96792A.

> diff --git a/drivers/media/i2c/maxim-serdes/max9296a.c b/drivers/media/i2c/maxim-serdes/max9296a.c
> new file mode 100644
> index 0000000000000..3dcdb63dbb9ee
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max9296a.c

[ ... ]

> +	/* Configure lane mapping. */
> +	/*
> +	 * The lane of each PHY can be mapped to physical lanes 0, 1, 2, and 3.
> +	 * This mapping is exclusive, multiple lanes, even if unused cannot be
> +	 * mapped to the same physical lane.
> +	 * Each lane mapping is represented as two bits.
> +	 */
> +	val = 0;
> +	for (i = 0; i < num_hw_data_lanes ; i++) {
> +		unsigned int map;
> +
> +		if (i < num_data_lanes)
> +			map = phy->mipi.data_lanes[i] - 1;
> +		else
> +			map = ffz(used_data_lanes);
> +
> +		val |= map << (i * 2);
> +		used_data_lanes |= BIT(map);
> +	}
> +
> +	if (phy->index == 0 && priv->info->phy0_lanes_0_1_on_second_phy)
> +		val = ((val & 0xf) << 4) | ((val >> 4) & 0xf);

[Severity: High]
Does this logic correctly handle physical lane routing for the PHY0 hardware
quirk? 

Since val stores the physical lane targets per logical lane, swapping the
4-bit nibbles swaps the logical lane assignments rather than the physical
targets. For example, if a custom mapping like data-lanes = <4 3> is
requested, this shift moves the mapping to the unused logical lanes 2 and 3,
while logical lanes 0 and 1 receive the default ffz mapping. 

Should this instead transform the physical targets, for example by XORing
them with 2?

> +
> +	ret = regmap_update_bits(priv->regmap, MAX9296A_MIPI_PHY3(index),
> +				 MAX9296A_MIPI_PHY3_PHY_LANE_MAP_4,
> +				 FIELD_PREP(MAX9296A_MIPI_PHY3_PHY_LANE_MAP_4, val));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Configure lane polarity.
> +	 *
> +	 * PHY 0 and 1 are on register 0x335.
> +	 * PHY 2 and 3 are on register 0x336.
> +	 *
> +	 * Each PHY has 3 bits of polarity configuration.
> +	 *
> +	 * On MAX9296A, each bit represents the lane polarity of logical lanes.
> +	 * Each of these lanes can be mapped to any physical lane.
> +	 * 0th bit is for lane 0.
> +	 * 1st bit is for lane 1.
> +	 * 2nd bit is for clock lane.
> +	 *
> +	 * On MAX96714, each bit represents the lane polarity of physical lanes.
> +	 * 0th bit for physical lane 0.
> +	 * 1st bit for physical lane 1.
> +	 * 2nd bit for clock lane of PHY 0, the slave PHY, which is unused.
> +	 *
> +	 * 3rd bit for physical lane 2.
> +	 * 4th bit for physical lane 3.
> +	 * 5th bit for clock lane of PHY 1, the master PHY.
> +	 */
> +
> +	for (i = 0, val = 0; i < num_data_lanes; i++) {
> +		unsigned int map;
> +
> +		if (!phy->mipi.lane_polarities[i + 1])
> +			continue;
> +
> +		/*
> +		 * The numbers inside the data_lanes array specify the hardware
> +		 * lane each logical lane maps to.
> +		 * If polarity is set for the physical lanes, retrieve the
> +		 * physical lane matching the logical lane from data_lanes.
> +		 * Otherwise, when polarity is set for the logical lanes
> +		 * the index of the polarity can be used.
> +		 */
> +
> +		if (priv->info->polarity_on_physical_lanes)
> +			map = phy->mipi.data_lanes[i] - 1;
> +		else
> +			map = i;
> +
> +		val |= BIT(map);
> +	}
> +
> +	if (phy->index == 0 && priv->info->phy0_lanes_0_1_on_second_phy)
> +		val = ((val & 0x3) << 2) | ((val >> 2) & 0x3);

[Severity: High]
Does this bitwise shift break polarity inversion for the active lanes?

Since the active logical lanes remain 0 and 1, shifting by 2 moves their
polarity configuration to the unused logical lanes 2 and 3. 

As the polarity val here applies to logical lanes, does it need any quirk
adjustment at all?

> +
> +	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,
> +				 FIELD_PREP(MAX9296A_MIPI_PHY5_PHY_POL_MAP_0_1, val) |
> +				 FIELD_PREP(MAX9296A_MIPI_PHY5_PHY_POL_MAP_2_3, val >> 2));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=18