Re: [PATCH v8 2/2] i2c: ls2x: Add clocks property parsing and adjust bus speed

On Tue, Jul 21, 2026 at 08:26:04PM +0800, Hongliang Wang wrote:
> The i2c-ls2x driver supports dts and acpi parameter passing.
> 
> In dts, uses clock framework, by parsing clocks property to
> get i2c bus reference clock, and define the div of reference
> clock by device data.
> 
> In acpi, by passing clocks property to describe i2c bus reference
> clock and clock-div property to describe the div of reference clock.
> 
> Based on i2c bus reference clock(clock_a), i2c bus speed(clock_s)
> and div, calculate the prcescale of i2c divider register. The
> calculation formula is
> 
> prcescale = (clock_a*10)/(div*clock_s)-1

This commit log is not understandable, what did you actually do
here? Please read carefully the submitting-patches documentation
(under the "Describe your changes" paragraph).

> Reviewed-by: Huacai Chen <[email protected]>

I haven't seen Huacai's review on patch 2, as far as I've seen he
has reviewed only patch 1. Have I missed anything?

> Cc: [email protected]
> Signed-off-by: Hongliang Wang <[email protected]>

Is this a Fix? I asked you this already. Please read carefully
the paragraph I suggested and add the necessary tag.

> ---
>  drivers/i2c/busses/i2c-ls2x.c | 40 ++++++++++++++++++++++++++++++++---
>  1 file changed, 37 insertions(+), 3 deletions(-)
> 

...

> @@ -107,12 +116,13 @@ static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
>  	else
>  		t->bus_freq_hz = LS2X_I2C_FREQ_STD;
>  
> +	val = (priv->pclk * 10) / (priv->div * t->bus_freq_hz) - 1;

Sashiko pointed out that this might overflow. While your replied
that the expected platform values may not overflow, the code does
not enforce those constraints.

The clock rate comes from firmware or the clock framework, so at
least a zero rate should be rejected. Using 64-bit arithmetic here
would also avoid relying on undocumented assumptions about the
input range at essentially no cost.

> +

...

> +	if (dev_of_node(dev)) {
> +		clk = devm_clk_get_optional_enabled(dev, NULL);
> +		if (IS_ERR(clk))
> +			return PTR_ERR(clk);
> +		if (clk)
> +			priv->pclk = clk_get_rate(clk);
> +		else
> +			priv->pclk = LS2X_I2C_PCLK_FREQ;
> +
> +		priv->div = (unsigned long)device_get_match_data(dev);
> +		if (!priv->div)
> +			priv->div = LS2X_I2C_2K_CLOCK_DIV;
> +	} else {
> +		/* clocks and clock-div are only ACPI properties. */

clocks is also DT property.

Andi

> +		ret = device_property_read_u32(dev, "clocks", &priv->pclk);
> +		if (ret)
> +			priv->pclk = LS2X_I2C_PCLK_FREQ;