Re: [PATCH v16 17/22] 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:
- [Medium] Incorrect GPIO pin offset mapping in PIN_CONFIG_SLEW_RATE corrupts slew rate configuration.
- [Medium] Subtracting unsigned values causes underflow in clock rate difference calculation, triggering abs() macro failure.
--

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

media: i2c: maxim-serdes: add MAX96717 driver

This commit adds a new MAX96717 driver that supports MAX9295A, MAX96717F, 
and MAX96793 serializers. It integrates with the common serializer framework
while enabling features across multiple 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..2764a24fb3a93
> --- /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;
> +		} 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;

[Severity: Medium]
Does this logic map the pin offsets correctly? When offset is 3 or 4, this
configures the slew rate for PIO05 and PIO06, while leaving offsets 5 and
6 unhandled and returning -EINVAL. 

This causes max96717_mux_set_rclkout() to modify pin 6's hardware register
when it attempts to configure the RCLK out slew rate for pin 4. Similarly,
offsets 7 and 8 appear to map to PIO10 and PIO11.

[ ... ]
> +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);
> +		if (diff_new < diff_old) {
> +			diff_old = diff_new;
> +			idx = i;
> +		}
> +	}
> +
> +	return idx;
> +}

[Severity: Medium]
Since both rate and freq are unsigned long, can this subtraction underflow
when rate is less than freq? 

Additionally, using the abs() macro on unsigned types can lead to build
failures or unexpected behavior due to type checking in the macro. Would
using abs_diff() be a safer approach here to calculate the difference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-gmsl2-3_serdes-v16-0-4fa6e08bb514@analog.com?part=17