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

Hi Andi,

On 2026/7/25 上午7:39, Andi Shyti wrote:
> 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).
I'm sorry for the unclear commit message in this version.
In the next revision I will rewrite the commit description properly,
following the Describe your changes guidelines, to clearly explain
what this patch does:

The original driver uses a fixed PCLK frequency(LS2X_I2C_PCLK_FREQ) and
fixed div(5) for I2C bus speed calculation. As PCLK frequency and div
vary on different SoCs and ACPI platforms, this leads to inaccurate I2C
bus speed calculation and poor adaptability across platforms.

Improve the accuracy of I2C bus speed calculation across different
platforms by parsing the real I2C bus reference clock (PCLK) and div.
Support both DTS and ACPI.

For DTS:
- Retrieve I2C bus reference clock via the common clock framework
- Fetch div from match data for LS2K/LS7A series
- Fallback to default values if clock lookup fails or the obtained
   pclk or div is zero

For ACPI:
- Parse "clocks" property to get I2C bus reference clock
- Parse "clock-div" property to get div
- Fallback to the default values if property parsing fails or the
   obtained pclk or div is zero

Calculate the I2C clock prescaler based on reference clock, div and
target bus frequency with the formula:
prescale = (pclk * 10) / (div * bus_freq_hz) - 1

Dynamically acquiring PCLK and div per platform ensures accurate I2C bus
speed calculation and reliable operation across different platforms.
>> 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?
>
Huacai has already confirmed on the mailing list during the v6 discussion
that he agrees with this patch and the tag can be retained. So this 
Reviewed-by
is valid.

Link: https://lore.kernel.org/r/[email protected]/

>> 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.
>
This is a correctness improvement, not a fix for a bug introduced by
a specific commit. Therefore no Fixes tag is applicable here.

The Cc stable was added on suggestion in v4 from Huacai Chen, in order 
to keep
clock configuration consistent and accurate across stable kernel 
releases for
Loongson platforms. If this improvement is not suitable for stable, I will
remove it in v9.

Link: 
https://lore.kernel.org/all/CAAhV-H71ZiakZaLVKYg2Qvp8ZJiT7hr9P9bAmXCrjWkHg+-vGg@mail.gmail.com/
>> ---
>>   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.
>
I will fix these issues in the next version:
- Use 64-bit arithmetic to avoid integer overflow during the clock 
calculation
- Add checks to reject zero clock rates and ensure valid clock inputs

The code is modified as follows:

--- a/drivers/i2c/busses/i2c-ls2x.c
+++ b/drivers/i2c/busses/i2c-ls2x.c
@@ -116,7 +116,7 @@ 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;
+       val = ((u64)priv->pclk * 10) / ((u64)priv->div * t->bus_freq_hz) 
- 1;

         /*
          * According to the chip manual, we can only access the 
registers as bytes,
@@ -319,10 +319,13 @@ static int ls2x_i2c_probe(struct platform_device 
*pdev)
                 clk = devm_clk_get_optional_enabled(dev, NULL);
                 if (IS_ERR(clk))
                         return PTR_ERR(clk);
-               if (clk)
+               if (clk) {
                         priv->pclk = clk_get_rate(clk);
-               else
+                       if (!priv->pclk)
+                               priv->pclk = LS2X_I2C_PCLK_FREQ;
+               } else {
                         priv->pclk = LS2X_I2C_PCLK_FREQ;
+               }

                 priv->div = (unsigned long)device_get_match_data(dev);
                 if (!priv->div)
@@ -330,7 +333,7 @@ static int ls2x_i2c_probe(struct platform_device *pdev)
         } else {
                 /* clocks and clock-div are only ACPI properties. */
                 ret = device_property_read_u32(dev, "clocks", &priv->pclk);
-               if (ret)
+               if (ret || !priv->pclk)
                         priv->pclk = LS2X_I2C_PCLK_FREQ;

                 ret = device_property_read_u32(dev, "clock-div", 
&priv->div);

>> +
> ...
>
>> +	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.
The comment is intended to indicate that the clocks and clock-div properties
are used only in the ACPI path, as requested by Krzysztof in the v4 review.
For DT, clocks are handled through the common clock framework instead.

Link: 
https://lore.kernel.org/all/20260526-pompous-gopher-of-serendipity-d72f1f@quoll/

I will update the comment to
"clocks and clock-div properties are used only in ACPI path" to make the
distinction clearer in the next version.
> Andi
>
>> +		ret = device_property_read_u32(dev, "clocks", &priv->pclk);
>> +		if (ret)
>> +			priv->pclk = LS2X_I2C_PCLK_FREQ;
Best regards,
Hongliang Wang