Re: [PATCH v13 16/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] GPIO input direction fails to enable the input receiver buffer (`RX_EN`).
- [Medium] Incorrect mapping of GPIO offsets to slew rate control registers.
- [Medium] Hardcoded pin multiplexing unconditionally overrides device tree pinmux settings.
- [Low] Unsigned arithmetic underflow in clock rate difference calculation causes incorrect clock rate selection.
--

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

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.

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

[Severity: Medium]
Is the mapping of GPIO offsets to register masks correct here?

It looks like the pinctrl subsystem passes the physical GPIO pin number
as the offset, but the switch logic appears to mix up a contiguous
index array with the actual GPIO numbers. For example, passing offset 3
configures the mask for PIO05 instead of PIO03. Could this cause valid
pins like offset 5 to fail entirely by erroneously hitting the offset < 7
branch later in the code?

[ ... ]
> +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 configuring a pin as an input properly re-enable the receiver buffer?

When a pin is configured as an output, this logic explicitly sets RX_EN
to 0. If that pin is later switched back to an input via
PIN_CONFIG_INPUT_ENABLE, there doesn't seem to be matching logic to
restore MAX96717_PINCTRL_RX_EN to 1. Will the pin's input buffer remain
disabled and always read as 0?

[ ... ]
> +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(rate - max96717_predef_freqs[i].freq);

[Severity: Low]
Can this calculation underflow and fail to compile?

Because both rate and predef_freqs[i].freq are unsigned long, the
subtraction will underflow rather than become negative. Additionally,
the kernel's abs() macro strictly checks for signed types and evaluates
to a void expression when passed an unsigned type. Would using abs_diff()
work better here?

[ ... ]
> +static int max96717_register_clkout(struct max96717_priv *priv)
> +{
> +	struct device *dev = &priv->client->dev;
> +	struct clk_init_data init = { .ops = &max96717_clk_ops };
> +	int ret;
> +
> +	ret = max96717_mux_set_rclkout(priv, MAX96717_RCLK_MFP);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Does this unconditionally overwrite device tree pinmux configurations?

During initialization, the pinctrl core applies device tree hogs (for
example, routing rclkout to MFP2) via pinctrl_enable() in
max96717_gpiochip_probe(). Calling max96717_mux_set_rclkout() here
immediately afterward appears to silently override the DT settings,
forcefully routing rclkout to MFP4 instead.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-gmsl2-3_serdes-v13-0-9d8a4919983b@analog.com?part=16