Re: [PATCH v15 16/22] media: i2c: maxim-serdes: add MAX96717 driver

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect mapping of absolute pin offsets to sparse hardware slew rate registers.
- [High] Changing a GPIO's direction to input fails to enable the receiver, causing silent 0 reads.
- [Medium] Boolean DT property `maxim,jitter-compensation` defaults to disabling the feature.
- [Medium] Missing upper bounds check on `num_data_lanes` allows silent truncation and link corruption.
--

--- Patch [16]: [PATCH v15 16/22] media: i2c: maxim-serdes: add MAX96717 driver ---
commit 1c2ed4219d13c6784d889f29a381e8396c67efec
Author: Cosmin Tanislav <[email protected]>

media: i2c: maxim-serdes: add MAX96717 driver

Add a new MAX96717 driver that also supports MAX9295A, MAX96717F and
MAX96793. Integrate it with the common serializer 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/max96717.c b/drivers/media/i2c/maxim-serdes/max96717.c
> new file mode 100644
> index 0000000000000..6cc4060e10f34
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max96717.c

[ ... ]

> +static const struct pinconf_generic_params max96717_cfg_params[] = {
> +	{ "maxim,jitter-compensation", MAX96717_PINCTRL_JITTER_COMPENSATION_EN, 0 },

[Severity: Medium]
When specifying a boolean property like maxim,jitter-compensation in the
device tree without a value, the generic pinctrl parsing applies this
default value of 0. Does this cause max96717_conf_pin_config_set_one() to
receive arg = 0 and explicitly clear the TX_COMP_EN bit instead of enabling
it? 

Should the default value be 1 to ensure the feature is enabled when
requested?

[ ... ]

> +static int max96717_get_pin_config_reg(unsigned int offset, u32 param,
> +				       unsigned int *reg, unsigned int *mask,
> +				       unsigned int *val)
> +{
> +	*reg = MAX96717_GPIO_A(offset);
> +
> +	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;

[Severity: High]
Does this logic incorrectly map absolute GPIO pin indices to the sparse
hardware slew rate fields?

If a caller passes offset == 4 (which maps to MAX96717_RCLK_MFP), it looks
like this falls into the offset < 5 branch and sets the mask to
MAX96717_PIO_SLEW_1_PIO06_SLEW. Would this inadvertently modify the slew
rate of GPIO6 instead of configuring the intended clock signal?

[ ... ]

> +static int max96717_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
> +	unsigned long config = pinconf_to_config_packed(PIN_CONFIG_INPUT_ENABLE, 1);
> +	struct max96717_priv *priv = gpiochip_get_data(gc);
> +
> +	return max96717_conf_pin_config_set_one(priv, offset, config);
> +}

[Severity: High]
When changing a pin's direction to input, does PIN_CONFIG_INPUT_ENABLE
correctly enable the receiver?

Looking at max96717_conf_pin_config_set_one(), PIN_CONFIG_OUTPUT_ENABLE
explicitly disables MAX96717_PINCTRL_RX_EN:

	case PIN_CONFIG_OUTPUT_ENABLE:
		config = pinconf_to_config_packed(MAX96717_PINCTRL_RX_EN, 0);
		return max96717_conf_pin_config_set_one(priv, offset, config);

Since max96717_gpio_direction_input() does not re-enable RX_EN, would
switching a pin from output back to input leave its receiver permanently
disabled, causing subsequent reads to silently return 0?

[ ... ]

> +static int max96717_init_phy(struct max_ser *ser,
> +			     struct max_ser_phy *phy)
> +{
> +	struct max96717_priv *priv = ser_to_priv(ser);
> +	unsigned int num_data_lanes = phy->mipi.num_data_lanes;
> +	unsigned int used_data_lanes = 0;
> +	unsigned int val;
> +	unsigned int i;
> +	int ret;
> +
> +	if (num_data_lanes == 3 && !priv->info->supports_3_data_lanes) {
> +		dev_err(priv->dev, "Unsupported 3 data lane mode\n");
> +		return -EINVAL;
> +	}
> +
> +	if (phy->mipi.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK &&
> +	    !priv->info->supports_noncontinuous_clock) {
> +		dev_err(priv->dev, "Unsupported non-continuous mode\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Configure a lane count. */
> +	ret = regmap_update_bits(priv->regmap, MAX96717_MIPI_RX1,
> +				 MAX96717_MIPI_RX1_CTRL_NUM_LANES,
> +				 FIELD_PREP(MAX96717_MIPI_RX1_CTRL_NUM_LANES,
> +					    num_data_lanes - 1));

[Severity: Medium]
Is there a missing upper bound check on num_data_lanes here?

While the V4L2 core allows up to 8 data lanes, this hardware appears to
support a maximum of 4. If the device tree mistakenly provides 5 lanes,
num_data_lanes - 1 becomes 4, which would overflow the 2-bit field mask
GENMASK(5, 4) and truncate to 0 via FIELD_PREP.

Additionally, a value larger than 4 could cause the following polarity loop
to iterate out of bounds and corrupt unrelated hardware fields:

	for (i = 0, val = 0; i < num_data_lanes; i++)
		if (phy->mipi.lane_polarities[i + 1])
			val |= BIT(i);

Should this function reject configurations where num_data_lanes exceeds
the hardware limits?

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