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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect pin index mapping for slew rate configuration modifies wrong hardware registers and rejects valid pins.
- [High] Setting a pin to input direction fails to explicitly enable the receiver, leaving the GPIO permanently unreadable if previously disabled.
- [Medium] Incorrect initialization of `diff_old` restricts maximum matchable clock frequency on 64-bit systems.
- [Medium] Speculative: Missing delay between hardware reset assertion and deassertion for the reference generator.
--

commit 29038ca9d5127738bdf88351f0cbd48295171826
Author: Cosmin Tanislav <demonsingur@gmail.com>
Subject: media: i2c: maxim-serdes: add MAX96717 driver

Add a new MAX96717 driver that also supports MAX9295A, MAX96717F and
MAX96793. Integrates it with the common serializer framework.

> 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
[ ... ]
> +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;

[Severity: High]
Does this logic improperly treat the pin offset as a sequential register
index?

When offset is 4, it is mapped to MAX96717_PIO_SLEW_1_PIO06_SLEW instead
of its correct register. Additionally, valid offsets like 5 and 6 return
-EINVAL.

This issue surfaces when max96717_mux_set_rclkout() is called with group
set to 4 (MAX96717_RCLK_MFP):

> +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);

This will corrupt pin 6's slew rate and leave pin 4 unconfigured. Should the
offsets be explicitly mapped to their corresponding physical pins?

[ ... ]
> +static int max96717_conf_pin_config_set_one(struct max96717_priv *priv,
> +					    unsigned int offset,
> +					    unsigned long config)
> +{
[ ... ]
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		arg = arg >= MAX96717_BIAS_PULL_STRENGTH_1000000_OHM;
> +		config = pinconf_to_config_packed(MAX96717_PINCTRL_PULL_STRENGTH_HIGH, arg);
> +		return max96717_conf_pin_config_set_one(priv, offset, config);
> +	case PIN_CONFIG_LEVEL:
> +		config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT_ENABLE, 1);
> +		return max96717_conf_pin_config_set_one(priv, offset, config);
> +	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);
> +	case MAX96717_PINCTRL_TX_ID:

[Severity: High]
Does PIN_CONFIG_INPUT_ENABLE need to be explicitly handled here to re-enable
the receiver?

When max96717_gpio_direction_output() cascades into
PIN_CONFIG_OUTPUT_ENABLE, it sets MAX96717_PINCTRL_RX_EN to 0 to disable
the receiver. If the pin is subsequently reconfigured as an input via
max96717_gpio_direction_input(), PIN_CONFIG_INPUT_ENABLE merely disables
the output driver but lacks a cascade to set MAX96717_PINCTRL_RX_EN back to 1.

This could leave standard GPIO inputs permanently unreadable if they were
previously disabled.

[ ... ]
> +static unsigned int max96717_clk_find_best_index(struct max96717_priv *priv,
> +						 unsigned long rate)
> +{
> +	unsigned int i, idx = 0;
> +	unsigned long diff_new, diff_old = U32_MAX;
> +
> +	for (i = 0; i < ARRAY_SIZE(max96717_predef_freqs); i++) {
> +		diff_new = abs_diff(rate, max96717_predef_freqs[i].freq);
> +		if (diff_new < diff_old) {

[Severity: Medium]
Will U32_MAX restrict the maximum matchable clock frequency on 64-bit systems?

Since unsigned long is 64-bit, if a client requests a very high rate
(e.g., ULONG_MAX) to query the maximum supported rate, the calculated
diff_new (ULONG_MAX - freq) will be far greater than U32_MAX.

The condition diff_new < diff_old will be false for all iterations, causing
the loop to exit without updating idx and falling back to the lowest
possible frequency (6.25 MHz). Should diff_old be initialized to ULONG_MAX
instead?

[ ... ]
> +static int max96717_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +				 unsigned long parent_rate)
> +{
[ ... ]
> +	val |= MAX96717_REF_VTG0_REFGEN_RST;
> +	val |= MAX96717_REF_VTG0_REFGEN_PREDEF_EN;
> +
> +	ret = regmap_write(priv->regmap, MAX96717_REF_VTG0, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_clear_bits(priv->regmap, MAX96717_REF_VTG0,
> +				MAX96717_REF_VTG0_REFGEN_RST);

[Severity: Medium]
Is it possible the hardware reference generator needs a delay to cleanly reset?

This asserts the reset flag and immediately clears it in the next I2C
transaction via regmap_clear_bits(). If the I2C bus is fast or commands are
batched, the hardware might not register a sufficient pulse width. Could this
cause the PLL state machine to fail to lock?

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