Re: [PATCH v4 2/7] clk: qcom: Add generic clkref_en support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unbalanced regulator disable during late init for clocks left enabled by the bootloader.
- [Medium] Hardcoded resource mapping and OF provider registration prevent API reuse in multi-functional drivers.
- [Medium] `qcom_clk_ref_is_enabled` returns a negative error code on failure, which CCF interprets as true.
--

commit 958034c0adafc3f72b4d7e46df81935d74404f7a
Author: Qiang Yu <qiang.yu@oss.qualcomm.com>

clk: qcom: Add generic clkref_en support

This commit introduces a new clk_ref clock type with custom clk_ops for
QREF blocks. It provides a common registration and probe API intended to
be reused across different placements like TCSR and TLMM.

> diff --git a/drivers/clk/qcom/clk-ref.c b/drivers/clk/qcom/clk-ref.c
> new file mode 100644
> index 0000000000000..213c0f58bb36e
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-ref.c

[ ... ]

> +static void qcom_clk_ref_unprepare(struct clk_hw *hw)
> +{
> +	struct qcom_clk_ref *rclk = to_qcom_clk_ref(hw);
> +
> +	if (rclk->desc.num_regulators)
> +		regulator_bulk_disable(rclk->desc.num_regulators, rclk->regulators);
> +}

[Severity: High]
If the bootloader leaves this clock enabled, the common clock framework
will call unprepare() during late init to turn off unused clocks. Since
prepare() was never called by the kernel, won't this
regulator_bulk_disable() call trigger an unbalanced disable warning and
permanently leak power?

Does the driver need to sync the hardware state during probe by enabling
the regulators if the clock is already on?

[ ... ]

> +static int qcom_clk_ref_is_enabled(struct clk_hw *hw)
> +{
> +	struct qcom_clk_ref *rclk = to_qcom_clk_ref(hw);
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(rclk->regmap, rclk->desc.offset, &val);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If regmap_read() fails, this returns a negative error code (e.g., -EINVAL).
Since the common clock framework evaluates the is_enabled callback as a
boolean, won't it treat this negative error code as true and incorrectly
assume the clock is enabled?

> +
> +	return !!(val & QCOM_CLK_REF_EN_MASK);
> +}

[ ... ]

> +int qcom_clk_ref_probe(struct platform_device *pdev,
> +		       const struct regmap_config *config,
> +		       const struct qcom_clk_ref_desc *descs,
> +		       size_t num_clk_refs)
> +{
> +	struct qcom_clk_ref_provider *provider;
> +	struct device *dev = &pdev->dev;
> +	struct regmap *regmap;
> +	void __iomem *base;
> +	int ret;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);

[Severity: Medium]
The commit message mentions reusing this model for blocks like TLMM on SM8750.
If a multi-functional driver like TLMM tries to use this helper, won't
devm_platform_ioremap_resource() fail with -EBUSY because the parent driver
already maps the register region?

Since qcom_clk_ref_register() is static and callers are forced to use
qcom_clk_ref_probe(), does this hardcoded resource mapping prevent the
intended integration with multi-functional drivers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527-tcsr_qref_0527-v4-0-ded83866c9d9@oss.qualcomm.com?part=2