Re: [PATCH v7 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] The driver ignores error pointers returned by `devm_clk_get_optional_enabled()`, failing to defer the probe when the clock is not yet available.
- [High] Missing validation for `device_get_match_data()` in the Device Tree path causes a fatal kernel panic due to division by zero.
- [Medium] Misuse of the standardized "clocks" property to store a raw integer frequency.

Pre-existing issues:
- [High] The driver uses `DEFINE_RUNTIME_DEV_PM_OPS` without enabling runtime PM, causing system suspend/resume callbacks to be ignored.
--

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

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

The i2c-ls2x driver supports dts and acpi parameter passing. In dts,
it uses the clock framework to get the i2c bus reference clock and
define the divider by device data. In acpi, it passes properties to
describe the bus reference clock and calculate the prescale value.

> --- a/drivers/i2c/busses/i2c-ls2x.c
> +++ b/drivers/i2c/busses/i2c-ls2x.c

[ ... ]

> @@ -304,6 +315,25 @@ static int ls2x_i2c_probe(struct platform_device *pdev)
>  	if (irq < 0)
>  		return irq;
>  
> +	if (dev_of_node(dev)) {
> +		clk = devm_clk_get_optional_enabled(dev, NULL);
> +		if (!IS_ERR_OR_NULL(clk))

[Severity: High]
If the clock provider is not yet ready, devm_clk_get_optional_enabled()
returns -EPROBE_DEFER. Because !IS_ERR_OR_NULL(clk) evaluates to false for
error pointers, does this skip returning the error and instead apply the
fallback frequency, preventing the probe from correctly deferring?

> +			priv->pclk = clk_get_rate(clk);
> +		else
> +			priv->pclk = LS2X_I2C_PCLK_FREQ;
> +
> +		priv->div = (unsigned long)device_get_match_data(dev);

[Severity: High]
If the driver is bound via sysfs to a platform device with an of_node but
an unmatched compatible string, device_get_match_data() will return NULL.

Does assigning this directly to priv->div without a fallback cause a
division by zero later in ls2x_i2c_adjust_bus_speed() when calculating the
prescale value?

ls2x_i2c_adjust_bus_speed()
    val = (priv->pclk * 10) / (priv->div * t->bus_freq_hz) - 1;

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

[Severity: Medium]
Across standard device bindings, the "clocks" property is strictly defined
as an array of phandles or references. Could reading it as a raw 32-bit
integer frequency cause naming conflicts or failures in generic firmware
parsing tools? Should "clock-frequency" be used instead?

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

[ ... ]

> @@ -349,8 +379,8 @@ static DEFINE_RUNTIME_DEV_PM_OPS(ls2x_i2c_pm_ops,
>  				 ls2x_i2c_suspend, ls2x_i2c_resume, NULL);
>  

[Severity: High]
This is a pre-existing issue, but the driver uses DEFINE_RUNTIME_DEV_PM_OPS
without enabling runtime pm via pm_runtime_enable().

Because the device pm state defaults to RPM_SUSPENDED, do system suspend
and resume operations using pm_runtime_force_suspend() see the device as
already suspended and skip calling ls2x_i2c_suspend() and ls2x_i2c_resume()?

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