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

["Artur Andrzejczak" <andrzejczak.artur@gmail.com>]

I only went through the clock, pinctrl and CSI-2 lane code, not the
whole patch.

On Fri Aug 7, 2026 at 3:02 PM CEST, Dumitru Ceclan wrote:
> [...]
> +	{ "maxim,jitter-compensation", MAX96717_PINCTRL_JITTER_COMPENSATION_EN, 0 },

The automated review already raised this: maxim,jitter-compensation is a
boolean in the binding, so the property is present but zero length.
Reading a zero-length property as a u32 gives -EOVERFLOW.
pinconf-generic treats only -EINVAL as absent and substitutes the
default above on any other error, so 0 lands.
max96717_conf_pin_config_set_one() then takes the arg ? en_val : ~en_val
branch with arg == 0 and clears the bit. That means the property for
enabling jitter compensation never sets this bit, and nothing else in
the driver sets it either. Giving it a value does not help, since
maxim,jitter-compensation = <1> is rejected by the schema.
pinctrl-k210.c uses 1 for its boolean params. Should the default value
here be 1 as well?

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

max96717_init_phy() calculates num_data_lanes - 1 without
checking for zero. The binding requires data-lanes, so this needs a
non-conforming DT, but the driver removed in patch 21 still rejected it
with "Invalid data lanes must be 1 to 4". Here num_data_lanes remains 0
and the write sets 0b11, so the count becomes four, silently.
max_ser_find_phys_config() compares num_data_lanes against the single
{ 4 } entry in max96717_phys_configs, so zero passes there too. The
automated review asked about the upper bound on this line, this is the
lower one. Shall the range check come back?

> [...]
> +	val = FIELD_PREP(MAX96717_REF_VTG0_REFGEN_PREDEF_FREQ,
> +			 predef_freq->val);
> +
> +	if (predef_freq->is_alt)
> +		val |= MAX96717_REF_VTG0_REFGEN_PREDEF_FREQ_ALT;
> +	if (!predef_freq->is_rclk)
> +		val |= MAX96717_REF_VTG0_REFGEN_EN;
> +
> +	val |= MAX96717_REF_VTG0_REFGEN_RST;
> +
> +	ret = regmap_write(priv->regmap, MAX96717_REF_VTG0, val);

max96717_clk_set_rate() sets REFGEN_PREDEF_FREQ and its ALT bit, but not
the predefined frequency enable bit. The REF_VTG0 defines stop at
PREDEF_FREQ. Bit 6 is not part of the composed value, and this is a full
regmap_write(), so the write clears it. The driver removed in patch 21
sets REFGEN_PREDEF_EN (BIT(6)) in its own write of that register, with
the same encodings for the six rates the two tables share. Currently,
that bit is left 0 on every rate that goes through REFGEN, including the
24 MHz default programmed at probe. The datasheet has REFGEN_PREDEF_EN
reset to 1, and describes REF_VTG4/5 as the feedback divider fraction
used when predefined mode is disabled, which this driver never writes.
Was that on purpose, or should it be set here too?

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

max96717_register_clkout() routes RCLKOUT to mfp4 and sets that pin to
the fastest slew rate. It runs after max96717_gpiochip_probe(), which
calls pinctrl_enable(), so at that time the pin setup from the max96717
node itself has already been applied. The binding in patch 03 allows
function = "rclkout" on mfp2, so a config that asks for the RCLK there
gets it, but loses it a few lines later: max96717_mux_set_rclkout()
clears RCLK_ALT for any group other than mfp2. A slew rate set on mfp4
is overwritten the same way. Should this leave alone a mux state that
pinctrl has already selected?

Moreover, the driver names seven slew fields for eleven pins. The
binding allows slew-rate on every pin, but max96717_get_pin_config_reg()
returns -EINVAL for it on mfp5, mfp6, mfp9 and mfp10, so a slew-rate
that passes the schema on one of those four makes pinctrl_select_state()
fail. Another device pointing its pinctrl-0 at such a node fails to
probe, since pinctrl_bind_pins() returns -EINVAL. On the max96717 node
itself it does not: pinctrl_claim_hogs() logs the error and returns 0.
Either way pinctrl_commit_state() stops at the failing setting, so the
pin configs after it are never applied. Table 13 in the datasheet lists
no pin slew for mfp5, mfp6, mfp9 and mfp10, so the driver looks right
here. Should the binding restrict slew-rate to the seven pins that have
the field?

Kind Regards,
Artur Andrzejczak