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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] max96792a_ops is missing the .set_pipe_link callback, preventing proper link routing.
- [High] max9296a_set_pipe_stream_id ignores the new stream_id argument and programs the old state.
- [High] Unvalidated data lane values of 0 can trigger an integer underflow and undefined bitwise shift behavior.
- [Medium] The device remains powered on (resource state leak) if probe fails after toggling the powerdown GPIO.
- [Low] The Kconfig symbol VIDEO_MAX9296A selects VIDEO_MAXIM_SERDES but does not inherit its dependency on VIDEO_DEV.
--

commit dd3188793bbc20f9b99b6950a6fcec1fa7e23c5d
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.

> diff --git a/drivers/media/i2c/maxim-serdes/Kconfig b/drivers/media/i2c/maxim-serdes/Kconfig
> --- a/drivers/media/i2c/maxim-serdes/Kconfig
> +++ b/drivers/media/i2c/maxim-serdes/Kconfig
[ ... ]
> @@ -45,3 +45,16 @@ config VIDEO_MAX96724
>  
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called max96724.
> +
> +config VIDEO_MAX9296A
> +	tristate "Maxim MAX9296A Dual Deserializer support"
> +	depends on I2C
> +	select VIDEO_MAXIM_SERDES

[Severity: Low]
Is there a missing dependency here? VIDEO_MAXIM_SERDES depends on VIDEO_DEV,
but this config only depends on I2C. Selecting a symbol without meeting its
dependencies can lead to unmet direct dependency warnings and silent build
failures.

> diff --git a/drivers/media/i2c/maxim-serdes/max9296a.c b/drivers/media/i2c/maxim-serdes/max9296a.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max9296a.c
[ ... ]
> +static int max9296a_init_phy(struct max_des *des, struct max_des_phy *phy)
> +{
[ ... ]
> +	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);

[Severity: High]
What happens if a malformed Device Tree provides a lane value of 0?

It appears the core framework only validates if the value exceeds
num_data_lanes. If the data lane is 0, the subtraction will underflow to
UINT_MAX, which could trigger undefined behavior when shifting bits in the
BIT(map) macro.

[ ... ]
> +static int max9296a_set_pipe_stream_id(struct max_des *des, struct max_des_pipe *pipe,
> +				       unsigned int stream_id)
> +{
> +	struct max9296a_priv *priv = des_to_priv(des);
> +	unsigned int index = max9296a_pipe_id(priv, pipe);
> +
> +	return regmap_update_bits(priv->regmap, MAX9296A_RX50(index), MAX9296A_RX50_STR_SEL,
> +				  FIELD_PREP(MAX9296A_RX50_STR_SEL, pipe->stream_id));

[Severity: High]
Should this use the passed stream_id argument instead of pipe->stream_id?

If the framework hasn't updated the pipe state yet, this might program
the hardware with the old stream ID, potentially breaking video stream
routing.

[ ... ]
> +static int max9296a_probe(struct i2c_client *client)
> +{
[ ... ]
> +	if (priv->gpiod_pwdn) {
> +		/* PWDN must be held for 1us for reset */
> +		udelay(1);
> +
> +		gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> +		/* Maximum power-up time (tLOCK) 4ms */
> +		usleep_range(4000, 5000);
> +	}
> +
> +	priv->des.info = priv->info->info;
> +	priv->des.ops = priv->info->ops;
> +
> +	ret = max9296a_reset(priv);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If max9296a_reset() or max_des_probe() fails below, does the device remain
powered on?

The powerdown GPIO is driven to 0 here to wake up the device, but it
doesn't appear to be reverted to 1 on the error paths, potentially leaving
the hardware drawing power.

> +
> +	return max_des_probe(client, &priv->des);
> +}
[ ... ]
> +static const struct max_des_ops max96792a_ops = {
> +	MAX9296A_COMMON_OPS,
> +	.set_pipe_stream_id = max96714_set_pipe_stream_id,
> +	.set_pipe_enable = max96714_set_pipe_enable,
> +	.set_pipe_tunnel_phy = max96716a_set_pipe_tunnel_phy,
> +	.set_pipe_tunnel_enable = max96714_set_pipe_tunnel_enable,
> +};

[Severity: High]
Should this struct include the .set_pipe_link callback?

The MAX96792A chip supports 2 pipes and 2 links, and it looks like the closely
related max96716a_ops uses max96716a_set_pipe_link for proper link routing.
Without it, could the core framework skip configuring the pipe-to-link mapping,
leaving the hardware with an incomplete configuration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-gmsl2-3_serdes-v12-0-b26d92931196@analog.com?part=18