Re: [PATCH v18 18/23] media: i2c: maxim-serdes: add MAX96717 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: max96717: MIPI RX lane map and polarity are swapped and use logical indices
- [Medium] media: max96717: incorrect pinctrl offset mapping rejects valid slew-rate pins and configures wrong pins
--

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

media: i2c: maxim-serdes: add MAX96717 driver

This patch introduces a new driver for the Maxim MAX96717 and related
serializers. It integrates with the common serializer framework while adding
support for features across multiple chips, receiving video over MIPI CSI-2
and outputting via GMSL2/3.

> diff --git a/drivers/media/i2c/maxim-serdes/max96717.c b/drivers/media/i2c/maxim-serdes/max96717.c
> new file mode 100644
> index 0000000000000..c87ea9e82c1ca
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max96717.c
> @@ -0,0 +1,1703 @@
[ ... ]
> +static int max96717_get_pin_config_reg(unsigned int offset, u32 param,
> +				       unsigned int *reg, unsigned int *mask,
> +				       unsigned int *val)
> +{
[ ... ]
> +	switch (param) {
> +	case PIN_CONFIG_SLEW_RATE:
> +		if (offset < 3) {
> +			*reg = MAX96717_PIO_SLEW_0;
> +			if (offset == 0)
> +				*mask = MAX96717_PIO_SLEW_0_PIO00_SLEW;
> +			else if (offset == 1)
> +				*mask = MAX96717_PIO_SLEW_0_PIO01_SLEW;
> +			else
> +				*mask = MAX96717_PIO_SLEW_0_PIO02_SLEW;
> +		} else if (offset < 5) {
> +			*reg = MAX96717_PIO_SLEW_1;
> +			if (offset == 3)
> +				*mask = MAX96717_PIO_SLEW_1_PIO05_SLEW;
> +			else
> +				*mask = MAX96717_PIO_SLEW_1_PIO06_SLEW;
> +		} else if (offset < 7) {
> +			return -EINVAL;
> +		} else if (offset < 9) {
> +			*reg  = MAX96717_PIO_SLEW_2;
> +			if (offset == 7)
> +				*mask = MAX96717_PIO_SLEW_2_PIO010_SLEW;
> +			else
> +				*mask = MAX96717_PIO_SLEW_2_PIO011_SLEW;
> +		} else {
> +			return -EINVAL;
> +		}

[Severity: Medium]
Does this logic map the correct pins for slew-rate configuration?

When offset represents the actual pin index, this sequentially maps offset 3
to PIO05_SLEW, offset 4 to PIO06_SLEW, and offsets 7 and 8 to PIO010_SLEW and
PIO011_SLEW.

This mistakenly configures PIO05 and PIO06 when pins 3 and 4 are requested.
When valid slew-capable pins like 5, 6, or 10 are passed, they fall into the
else if branches and are incorrectly rejected with -EINVAL.

[ ... ]
> +static int max96717_mux_set_rclkout(struct max96717_priv *priv, unsigned int group)
> +{
> +	unsigned long config;
> +	int ret;
> +
> +	config = pinconf_to_config_packed(PIN_CONFIG_SLEW_RATE,
> +					  MAX96717_PIO_SLEW_FASTEST);
> +	ret = max96717_conf_pin_config_set_one(priv, group, config);

[Severity: Medium]
When max96717_mux_set_rclkout() requests the fastest slew rate for
MAX96717_RCLK_MFP (pin 4), does the mapping issue mentioned above cause
it to erroneously configure PIO06's slew rate instead of the RCLK pin?

[ ... ]
> +static int max96717_init_phy(struct max_ser *ser,
> +			     struct max_ser_phy *phy)
> +{
[ ... ]
> +	/* Configure lane mapping. */
> +	val = 0;
> +	for (i = 0; i < 4; 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);
> +	}
> +
> +	ret = regmap_update_bits(priv->regmap, MAX96717_MIPI_RX3,
> +				 MAX96717_MIPI_RX3_PHY2_LANE_MAP,
> +				 FIELD_PREP(MAX96717_MIPI_RX3_PHY2_LANE_MAP, val));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, MAX96717_MIPI_RX2,
> +				 MAX96717_MIPI_RX2_PHY1_LANE_MAP,
> +				 FIELD_PREP(MAX96717_MIPI_RX2_PHY1_LANE_MAP, val >> 4));

[Severity: High]
Are the lane assignments for PHY1 and PHY2 swapped here?

The driver constructs val with physical lane mappings for logical lanes 0-3,
where bits 0-3 correspond to lanes 0-1 and bits 4-7 correspond to lanes 2-3.

However, val (lanes 0-1) is written to MAX96717_MIPI_RX3_PHY2_LANE_MAP, and
val >> 4 (lanes 2-3) is written to MAX96717_MIPI_RX2_PHY1_LANE_MAP. PHY1
normally controls lanes 0-1 and PHY2 controls lanes 2-3.

> +	if (ret)
> +		return ret;
> +
> +	/* Configure lane polarity. */
> +	for (i = 0, val = 0; i < num_data_lanes; i++)
> +		if (phy->mipi.lane_polarities[i + 1])
> +			val |= BIT(i);

[Severity: High]
Does lane polarity configuration use the correct index?

Setting polarity using BIT(i) applies the inversion based on the logical lane
index. If the lanes are remapped, this could apply the inversion to the wrong
physical lane. Should this use the physical lane index
BIT(phy->mipi.data_lanes[i] - 1) instead?

Additionally, the same PHY1/PHY2 swap from the lane map appears to occur for
the polarity map just below this loop.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-gmsl2-3_serdes-v18-0-43117818f499@analog.com?part=18