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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Incorrect array mapping logic for Slew Rate PIN configuration
- [High] Unconditional hardcoding of rclkout pin mux bypasses pinctrl framework
- [High] Race condition exposing GPIOs and Clock before hardware initialization
- [High] gpio_get_direction returns an error for input pins
- [Medium] Kconfig dependency propagation issue for VIDEO_MAX96717
- [Medium] Log spam in clock determine_rate callback
--

commit e3bc10c21430d0bc98ab74141ac3217a5001c9ab
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, 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/Kconfig b/drivers/media/i2c/maxim-serdes/Kconfig
> index f5a4ca80a263b..ddbb5791e9349 100644
> --- a/drivers/media/i2c/maxim-serdes/Kconfig
> +++ b/drivers/media/i2c/maxim-serdes/Kconfig
> @@ -15,3 +15,21 @@ config VIDEO_MAXIM_SERDES
[ ... ]
> +config VIDEO_MAX96717
> +	tristate "Maxim MAX96717 Serializer support"
> +	depends on COMMON_CLK
> +	depends on I2C
> +	depends on PINCTRL
> +	select VIDEO_MAXIM_SERDES

[Severity: Medium]
Does this symbol need to inherit the dependencies of VIDEO_MAXIM_SERDES? If
VIDEO_MAXIM_SERDES has dependencies that are not met, could selecting it here
cause unmet direct dependency warnings or silent build failures on platforms
lacking the required infrastructure?

> diff --git a/drivers/media/i2c/maxim-serdes/max96717.c b/drivers/media/i2c/maxim-serdes/max96717.c
> new file mode 100644
> index 0000000000000..afca6ba51851f
> --- /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]
Since the offset parameter represents the absolute hardware GPIO pin number
(0 to 10), does this logic incorrectly treat it as a contiguous index? 

For example, if offset is 3 (GPIO 3), will it write to PIO05_SLEW instead of
returning -EINVAL?

[ ... ]
> +static int max96717_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +	unsigned long config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT_ENABLE, 0);
> +	struct max96717_priv *priv = gpiochip_get_data(gc);
> +	int ret;
> +
> +	ret = max96717_conf_pin_config_get(priv->pctldev, offset, &config);
> +	if (ret)
> +		return ret;
> +
> +	return pinconf_to_config_argument(config) ? GPIO_LINE_DIRECTION_OUT
> +						  : GPIO_LINE_DIRECTION_IN;
> +}

[Severity: High]
Because max96717_conf_pin_config_get() correctly returns -EINVAL when output
enable is false, does returning the error here break direction querying for
all input pins instead of interpreting it as GPIO_LINE_DIRECTION_IN?

[ ... ]
> +static int max96717_clk_determine_rate(struct clk_hw *hw,
> +				       struct clk_rate_request *req)
> +{
> +	struct max96717_priv *priv = clk_hw_to_priv(hw);
> +	struct device *dev = &priv->client->dev;
> +	unsigned int idx;
> +
> +	idx = max96717_clk_find_best_index(priv, req->rate);
> +
> +	if (req->rate != max96717_predef_freqs[idx].freq) {
> +		dev_warn(dev, "Request CLK freq:%lu, found CLK freq:%lu\n",
> +			 req->rate, max96717_predef_freqs[idx].freq);
> +	}

[Severity: Medium]
Since the clock framework routinely queries determine_rate to find the
closest available frequency, will this dev_warn() flood the kernel log during
normal clock rate negotiations?

[ ... ]
> +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: High]
Does hardcoding MAX96717_RCLK_MFP (pin 4) here bypass the pinctrl framework
and override device tree configurations if a board uses the alternate
supported pin (MFP 2) for rclkout?

[ ... ]
> +static int max96717_probe(struct i2c_client *client)
> +{
[ ... ]
> +	ret = max96717_wait_for_device(priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = max96717_gpiochip_probe(priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = max96717_register_clkout(priv);
> +	if (ret)
> +		return ret;
> +
> +	return max_ser_probe(client, &priv->ser);
> +}

[Severity: High]
Can exposing the GPIOs and clock to userspace and other subsystems before
max_ser_probe() completes lead to erratic hardware behavior? 

Mandatory hardware initialization (like setting CMU2 PFDDIV to 1.1V for
correct functionality) happens later inside max_ser_init() which is triggered
by max_ser_probe().

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