Re: [PATCH v2 09/13] i3c: dw-i3c-master: Add a quirk to skip clock and reset

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] Missing reset control assertion on pclk acquisition failure.
- [High] Type confusion when parsing match data for ACPI devices using PRP0001.
- [Medium] Semantic misuse of the `clock-frequency` device property.
--

commit e97fa5a2aa57bfea8bd1aabcc2dc629a2e47c19c
Author: Akhil R <akhilrajeev@nvidia.com>

    i3c: dw-i3c-master: Add a quirk to skip clock and reset

This commit adds a quirk to skip clock and reset acquisition for
ACPI-enumerated devices like Tegra410. It moves match data parsing
earlier in the probe sequence and falls back to reading the
"clock-frequency" property if the quirk is present.

[ ... ]

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 05ccdf177b6dd..a62eec6d2ac0a 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c

[ ... ]

> @@ -560,13 +561,26 @@ static void dw_i3c_master_set_intr_regs(struct dw_i3c_master *master)
>  	writel(IBI_REQ_REJECT_ALL, master->regs + IBI_MR_REQ_REJECT);
>  }
>  
> +static unsigned long dw_i3c_master_get_core_rate(struct dw_i3c_master *master)
> +{
> +	unsigned int core_rate_prop;
> +
> +	if (!(master->quirks & DW_I3C_ACPI_SKIP_CLK_RST))
> +		return clk_get_rate(master->core_clk);
> +
> +	if (device_property_read_u32(master->dev, "clock-frequency", &core_rate_prop))
> +		return 0;
> +
> +	return core_rate_prop;

Does this usage conflict with standard device tree bindings? The
"clock-frequency" property in the I2C and I3C subsystems typically represents
the external bus frequency (e.g., 400kHz or 12.5MHz), not the internal IP core
clock rate.

Reusing this standard property for the internal core clock rate might cause
integer underflows and timing misconfigurations if standard bus frequencies are
provided in the device tree or ACPI tables.

[ ... ]

> @@ -1600,21 +1614,34 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>  	if (IS_ERR(master->regs))
>  		return PTR_ERR(master->regs);
>  
> -	master->core_clk = devm_clk_get_enabled(&pdev->dev, NULL);
> -	if (IS_ERR(master->core_clk))
> -		return PTR_ERR(master->core_clk);
> +	if (has_acpi_companion(&pdev->dev)) {
> +		quirks = (unsigned long)device_get_match_data(&pdev->dev);
> +	} else if (pdev->dev.of_node) {

Will this cause type confusion if an ACPI device uses the PRP0001 mechanism?

If PRP0001 delegates matching to the OF table, device_get_match_data()
returns a pointer to a struct dw_i3c_drvdata rather than an integer bitmask.
Casting this memory pointer directly to an unsigned long would assign
arbitrary bits based on the kernel's memory layout, potentially triggering
random quirks like DW_I3C_ACPI_SKIP_CLK_RST.

> +		drvdata = device_get_match_data(&pdev->dev);
> +		if (drvdata)
> +			quirks = drvdata->flags;
> +	}
> +	master->quirks = quirks;
> +
> +	if (master->quirks & DW_I3C_ACPI_SKIP_CLK_RST) {
> +		master->core_clk = NULL;
> +		master->core_rst = NULL;
> +	} else {
> +		master->core_clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +		if (IS_ERR(master->core_clk))
> +			return PTR_ERR(master->core_clk);
> +
> +		master->core_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
> +									     "core_rst");
> +		if (IS_ERR(master->core_rst))
> +			return PTR_ERR(master->core_rst);
> +		reset_control_deassert(master->core_rst);
> +	}
>  
>  	master->pclk = devm_clk_get_optional_enabled(&pdev->dev, "pclk");
>  	if (IS_ERR(master->pclk))
>  		return PTR_ERR(master->pclk);

If devm_clk_get_optional_enabled() fails here (e.g., returning -EPROBE_DEFER),
will this leave the hardware out of reset?

The code returns directly, but master->core_rst was already deasserted.
While devres frees the reset control reference, it won't automatically
re-assert the reset line. Should this path use the err_assert_rst label
to ensure proper cleanup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260409105747.48158-1-akhilrajeev@nvidia.com?part=9