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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] 64-bit integer division using the '/' operator causes a linker error on 32-bit architectures.
- [Medium] The ACPI fallback path reads the generic 'clocks' property as a single 32-bit integer frequency, violating standard property semantics.

Pre-existing issues:
- [High] Hardware interrupts are enabled before the IRQ handler is registered.
- [High] The driver lacks a remove function, failing to disable hardware interrupts during driver unbind.
--

--- Patch [2]: [PATCH v9 2/2] i2c: ls2x: Add clocks property parsing and adjust bus speed ---
commit d881529f1ceaa7d91f234c648749c3641859d689
Author: Hongliang Wang <wanghongliang@loongson.cn>

i2c: ls2x: Add clocks property parsing and adjust bus speed

This commit improves I2C bus speed calculation across platforms by parsing
the real I2C bus reference clock (PCLK) and div from DTS or ACPI, instead
of using fixed values.

> diff --git a/drivers/i2c/busses/i2c-ls2x.c b/drivers/i2c/busses/i2c-ls2x.c
> index b475dd27b7af9..65451fc9145b7 100644
> --- a/drivers/i2c/busses/i2c-ls2x.c
> +++ b/drivers/i2c/busses/i2c-ls2x.c
> @@ -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 = ((u64)priv->pclk * 10) / ((u64)priv->div * t->bus_freq_hz) - 1;

[Severity: High]
Will this 64-bit integer division using the direct '/' operator cause a
linker error on 32-bit architectures?

Since CONFIG_COMPILE_TEST=y is possible, compiling this driver on 32-bit
architectures emits a call to __udivdi3, which is intentionally omitted in
the kernel. 

Could we use div64_u64() or do_div() from <linux/math64.h> here instead?

> +
>  	/*
>  	 * According to the chip manual, we can only access the registers as bytes,
>  	 * otherwise the high bits will be truncated.

[ ... ]

> @@ -304,6 +315,32 @@ static int ls2x_i2c_probe(struct platform_device *pdev)
>  	if (irq < 0)
>  		return irq;
>  
> +	if (dev_of_node(dev)) {

[ ... ]

> +	} else {
> +		/* clocks and clock-div properties are used only in ACPI path. */
> +		ret = device_property_read_u32(dev, "clocks", &priv->pclk);

[Severity: Medium]
Does reading the ACPI "clocks" property as a single 32-bit integer frequency
violate standard property semantics?

The driver expects ACPI tables to provide the input clock frequency as an
integer under the "clocks" property. 

However, "clocks" is standardly defined as an array of references (phandles)
to clock providers. 

Should a standard property like "clock-frequency" be used instead to pass a
scalar integer?

> +		if (ret || !priv->pclk)
> +			priv->pclk = LS2X_I2C_PCLK_FREQ;

[ ... ]

[Severity: High]
This is a pre-existing issue, but does initializing the hardware in
ls2x_i2c_probe() enable interrupts before the IRQ handler is registered?

drivers/i2c/busses/i2c-ls2x.c:ls2x_i2c_probe() {
    ...
    ls2x_i2c_init(priv);
    
    ret = devm_request_irq(dev, irq, ls2x_i2c_isr, IRQF_SHARED, "ls2x-i2c",
                           priv);
    ...
}

In ls2x_i2c_init(), the CTR_READY_MASK (which includes the interrupt enable
bit LS2X_CTR_IEN) is written to the control register before the handler is
registered by devm_request_irq().

If a hardware interrupt is asserted in this window, could it lead to an
unhandled interrupt storm and permanently disable the shared IRQ line?

[Severity: High]
This is a pre-existing issue, but is the driver missing a remove function
to disable hardware interrupts during driver unbind?

drivers/i2c/busses/i2c-ls2x.c:
static struct platform_driver ls2x_i2c_driver = {
    .probe      = ls2x_i2c_probe,
    .driver     = {
        ...
    },
};

Because the driver relies on devres for cleanup and lacks a .remove
callback in ls2x_i2c_driver, devres will free the IRQ handler on unbind
while the hardware is never quiesced. 

If an interrupt asserts after the handler is freed, would this cause an
unhandled interrupt?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810074927.25284-1-wanghongliang@loongson.cn?part=2