[PATCH v2] clk: si521xx: correct the SI52147 OF output enable map

[PATCH v2] clk: si521xx: correct the SI52147 OF output enable map

[Pengpeng Hou]

The SI52147 data sheet assigns DIFF0 through DIFF3 output-enable bits to
0x17 in control register 1. The OF match data uses 0x15, leaving the
DIFF2 bit out of the map, while the I2C ID table already uses the
documented value.

Use 0x17 for the SI52147 OF entry so both enumeration paths expose the
documented outputs.

Fixes: edc12763a3a29 ("clk: si521xx: Clock driver for Skyworks Si521xx I2C PCIe clock generators")

Assisted-by: Codex:gpt-5
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since the RFC: https://lore.kernel.org/all/20260624055810.49310-1-pengpeng@iscas.ac.cn/
- replace the table-consistency assumption with the SI52147 data sheet
- send as a normal patch now that the register value is documented
- disclose the use of Codex

Data sheet: https://www.skyworksinc.com/Application-Pages/-/media/SkyWorks/SL/documents/public/data-sheets/Si52147.pdf

The change was reviewed against the data sheet and source. It was not
tested on SI52147 hardware.

 drivers/clk/clk-si521xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-si521xx.c b/drivers/clk/clk-si521xx.c
index ceadc07bcb6d..d5be08e12685 100644
--- a/drivers/clk/clk-si521xx.c
+++ b/drivers/clk/clk-si521xx.c
@@ -374,7 +374,7 @@ MODULE_DEVICE_TABLE(i2c, si521xx_id);
 static const struct of_device_id clk_si521xx_of_match[] = {
 	{ .compatible = "skyworks,si52144", .data = (void *)SI521XX_OE_MAP(0x5, 0xc0) },
 	{ .compatible = "skyworks,si52146", .data = (void *)SI521XX_OE_MAP(0x15, 0xe0) },
-	{ .compatible = "skyworks,si52147", .data = (void *)SI521XX_OE_MAP(0x15, 0xf8) },
+	{ .compatible = "skyworks,si52147", .data = (void *)SI521XX_OE_MAP(0x17, 0xf8) },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, clk_si521xx_of_match);
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2] clk: si521xx: correct the SI52147 OF output enable map

[sashiko-bot]

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

Pre-existing issues:
- [High] Out-of-bounds array access in si521xx_of_clk_get() when parsing device tree clock indices.
- [High] `regcache_sync()` during resume overwrites the direct I2C initialization of the Byte Count register, breaking subsequent I2C reads.
--

--- Patch [1]: [PATCH v2] clk: si521xx: correct the SI52147 OF output enable map ---
commit 415da2f3277cfd67c9fa71b04185cd6d8978b923
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
Subject: clk: si521xx: correct the SI52147 OF output enable map

This patch corrects the SI52147 OF match data to use 0x17 for output-enable
bits in control register 1. This aligns the OF map with the data sheet and
I2C ID table, exposing the documented outputs.

> diff --git a/drivers/clk/clk-si521xx.c b/drivers/clk/clk-si521xx.c
> index ceadc07bcb6df..d5be08e126858 100644
> --- a/drivers/clk/clk-si521xx.c
> +++ b/drivers/clk/clk-si521xx.c
> @@ -374,7 +374,7 @@ MODULE_DEVICE_TABLE(i2c, si521xx_id);
>  static const struct of_device_id clk_si521xx_of_match[] = {
>  	{ .compatible = "skyworks,si52144", .data = (void *)SI521XX_OE_MAP(0x5, 0xc0) },
>  	{ .compatible = "skyworks,si52146", .data = (void *)SI521XX_OE_MAP(0x15, 0xe0) },
> -	{ .compatible = "skyworks,si52147", .data = (void *)SI521XX_OE_MAP(0x15, 0xf8) },
> +	{ .compatible = "skyworks,si52147", .data = (void *)SI521XX_OE_MAP(0x17, 0xf8) },

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to an
out-of-bounds array access in si521xx_of_clk_get()?

When parsing device tree clock indices, the user-provided index from the
device tree is directly used as an array index without bounds checking:

drivers/clk/clk-si521xx.c:si521xx_of_clk_get() {
	...
	unsigned int idx = clkspec->args[0];

	return &si->clk_dif[idx].hw;
}

If a device tree specifies a clock index beyond the initialized clocks for
the specific chip, this returns an arbitrary pointer or an uninitialized
struct, causing memory corruption or a NULL pointer dereference in the
clock core.

Should the index be validated against a maximum value (like the number of
outputs for the chip) before accessing si->clk_dif[]?

[Severity: High]
This is a pre-existing issue, but does the resume path overwrite the Byte
Count register and break subsequent I2C reads?

In si521xx_probe(), the Byte Count register (SI521XX_REG_BC) is directly
written to 1 via i2c_master_send(), bypassing the regmap:

drivers/clk/clk-si521xx.c:si521xx_probe() {
	...
	const u8 data[3] = { SI521XX_REG_BC, 1, 1 };
	...
	ret = i2c_master_send(client, data, ARRAY_SIZE(data));
	...
}

Because this register is never updated via the regmap and no reg_defaults
are provided, its cached value remains uninitialized (0).

During si521xx_resume(), regcache_sync() writes all cached values for
writable registers back to the hardware:

drivers/clk/clk-si521xx.c:si521xx_resume() {
	...
	ret = regcache_sync(si->regmap);
	...
}

This overwrites SI521XX_REG_BC with 0. The custom read function
si521xx_regmap_i2c_read() relies on the hardware BC being 1 to correctly
receive 1 byte of data.

Does this cause I2C reads to fail or return corrupt data after a suspend
and resume cycle?

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, clk_si521xx_of_match);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813153650.44793-1-pengpeng@iscas.ac.cn?part=1