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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] MIPI TX controller registers (e.g. MIPI_TX10, MIPI_TX3, MIPI_TX51) are incorrectly configured using PHY IDs instead of Pipe IDs, leaving video pipes 0 and 3 unconfigured.
--

commit 66bd782f7bdf1b5e7ef00c3c965bc6161148af83
Author: Cosmin Tanislav <demonsingur@gmail.com>

media: i2c: maxim-serdes: add MAX9296A driver

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

Integrate it with the common Deserializer framework, while keeping
compatibility with existing usecases, avoiding code duplication, and
also enabling more features across all chips.

> diff --git a/drivers/media/i2c/maxim-serdes/max9296a.c b/drivers/media/i2c/maxim-serdes/max9296a.c
> new file mode 100644
> index 0000000000000..7b25b54e63deb
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max9296a.c
> @@ -0,0 +1,1461 @@
[ ... ]
> +static int max9296a_init_phy(struct max_des *des, struct max_des_phy *phy)
> +{
> +	struct max9296a_priv *priv = des_to_priv(des);
> +	bool is_cphy = phy->bus_type == V4L2_MBUS_CSI2_CPHY;
> +	unsigned int num_data_lanes = phy->mipi.num_data_lanes;
> +	unsigned int dpll_freq = is_cphy ? phy->link_frequency
> +					 : phy->link_frequency * 2;
> +	unsigned int num_hw_data_lanes;
> +	unsigned int hw_index = max9296a_phy_id(priv, phy);
> +	unsigned int index = phy->index;
> +	unsigned int used_data_lanes = 0;
> +	unsigned int val;
> +	unsigned int i;
> +	int ret;
[ ... ]
> +	/* Configure a lane count. */
> +	ret = regmap_update_bits(priv->regmap, MAX9296A_MIPI_TX10(hw_index),
> +				 MAX9296A_MIPI_TX10_CSI2_LANE_CNT,
> +				 FIELD_PREP(MAX9296A_MIPI_TX10_CSI2_LANE_CNT,
> +					    num_data_lanes - 1));

[Severity: High]
Are the MIPI_TX registers correctly indexed using the PHY ID here?

The MIPI_TX register block (base 0x400) appears to be per-pipe as seen in
max9296a_set_pipe_remap(), meaning its index should likely be the pipe ID
(0, 1, 2, 3), not the PHY ID. If indexed by PHY ID, video pipes 0 and 3 will
remain unconfigured, while pipes 1 and 2 will be misconfigured.

> +	if (ret)
> +		return ret;
[ ... ]
> +static int max9296a_set_phy_mode(struct max_des *des, struct max_des_phy *phy,
> +				 struct max_des_phy_mode *mode)
> +{
> +	struct max9296a_priv *priv = des_to_priv(des);
> +	unsigned int phy_id = max9296a_phy_id(priv, phy);
> +	int ret;
> +
> +	/* Set alternate memory map modes. */
> +	ret = regmap_assign_bits(priv->regmap, MAX9296A_MIPI_TX51(phy_id),
> +				 MAX9296A_MIPI_TX51_ALT_MEM_MAP_12,
> +				 mode->alt_mem_map12);

[Severity: High]
Does this code also incorrectly use the PHY ID to configure the alternate memory
map on the MIPI TX block instead of the pipe ID?

> +	if (ret)
> +		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-gmsl2-3_serdes-v17-0-002499e534e8@analog.com?part=22