[PATCH v3 0/2] Add support for Si549 programmable clock

[PATCH v3 0/2] Add support for Si549 programmable clock

[Pavel Löbl]

This adds support for Si549 programmable oscillator. It's almost the
same as already supported Si544, except it uses different internal
oscillator frequency.

So new compatible strings are added, and driver data is extended
to carry both maximum output frequency and internal xtal frequency.

Changes in v3:
  Added missing xo_freq member description as found by kernel test robot:

  https://lore.kernel.org/oe-kbuild-all/202607291708.qFSxYL2s-lkp@intel.com/

Changes in v2:
  Fixed obvious stuff found by Sashiko (extra unused struct member),
  removing unneeded data argument to calculation functions, xo_freq
  is now passed in settings struct.

Pavel Löbl (2):
  dt-bindings: clock: si544: add si549 compatible
  clk: si544: add support for si549

 .../bindings/clock/silabs,si544.yaml          | 10 ++-
 drivers/clk/Kconfig                           |  6 +-
 drivers/clk/clk-si544.c                       | 83 +++++++++++++++----
 3 files changed, 76 insertions(+), 23 deletions(-)

-- 
2.53.0

[PATCH v3 1/2] dt-bindings: clock: si544: add si549 compatible

[Pavel Löbl]

Adding Si549 compatible, which uses different internal oscillator
frequency. Like in Si544 case, there are three types ending with
latters a,b and c. Each with different maximum output frequency.

Also fix datasheet links after Silicon Labs acquisition by
Skyworks Solutions.

Signed-off-by: Pavel Löbl <pavel@loebl.cz>
---
 .../devicetree/bindings/clock/silabs,si544.yaml        | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/silabs,si544.yaml b/Documentation/devicetree/bindings/clock/silabs,si544.yaml
index f87e71867108..c58171f1bc5b 100644
--- a/Documentation/devicetree/bindings/clock/silabs,si544.yaml
+++ b/Documentation/devicetree/bindings/clock/silabs,si544.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/clock/silabs,si544.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Silicon Labs SI514/SI544 clock generator
+title: Silicon Labs SI514/SI544/SI549 clock generator
 
 maintainers:
   - Mike Looijmans <mike.looijmans@topic.nl>
@@ -13,8 +13,9 @@ description: >
   Silicon Labs 514/544 programmable I2C clock generator. Details about the device
   can be found in the datasheet:
 
-    https://www.silabs.com/Support%20Documents/TechnicalDocs/si514.pdf
-    https://www.silabs.com/documents/public/data-sheets/si544-datasheet.pdf
+    https://www.skyworksinc.com/-/media/Skyworks/SL/documents/public/data-sheets/Si514.pdf
+    https://www.skyworksinc.com/-/media/SkyWorks/SL/documents/public/data-sheets/si544-datasheet.pdf
+    https://www.skyworksinc.com/-/media/Skyworks/SL/documents/public/data-sheets/si549-datasheet.pdf
 
 properties:
   compatible:
@@ -23,6 +24,9 @@ properties:
       - silabs,si544a
       - silabs,si544b
       - silabs,si544c
+      - silabs,si549a
+      - silabs,si549b
+      - silabs,si549c
 
   reg:
     maxItems: 1
-- 
2.53.0

Re: [PATCH v3 1/2] dt-bindings: clock: si544: add si549 compatible

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The updated datasheet URL for the si544 datasheet contains a capitalization typo in the path (`SkyWorks` instead of `Skyworks`), which could result in a 404 Not Found error on case-sensitive web servers.
- [Low] The binding description text was not updated to mention the new Si549 devices, leaving it inconsistent with the updated title and compatible string list.
--

--- Patch [1]: [PATCH v3 1/2] dt-bindings: clock: si544: add si549 compatible ---
commit 529dcd41cafc82a36f39e95eb442dbe9230735cf
Author: Pavel Löbl <pavel@loebl.cz>

dt-bindings: clock: si544: add si549 compatible

This commit adds the Si549 compatible strings to the si544 clock generator
binding. It also updates the datasheet links to point to Skyworks Solutions
after their acquisition of Silicon Labs.

> diff --git a/Documentation/devicetree/bindings/clock/silabs,si544.yaml b/Documentation/devicetree/bindings/clock/silabs,si544.yaml
> index f87e718671080..c58171f1bc5be 100644
> --- a/Documentation/devicetree/bindings/clock/silabs,si544.yaml
> +++ b/Documentation/devicetree/bindings/clock/silabs,si544.yaml

[ ... ]

> @@ -13,8 +13,9 @@ description: >
>    Silicon Labs 514/544 programmable I2C clock generator. Details about the device
>    can be found in the datasheet:

[Severity: Low]
Should the description block also be updated to mention the 549 series? The
title and compatible properties were updated to include 549, but this text
remains limited to 514/544.

> -    https://www.silabs.com/Support%20Documents/TechnicalDocs/si514.pdf
> -    https://www.silabs.com/documents/public/data-sheets/si544-datasheet.pdf
> +    https://www.skyworksinc.com/-/media/Skyworks/SL/documents/public/data-sheets/Si514.pdf
> +    https://www.skyworksinc.com/-/media/SkyWorks/SL/documents/public/data-sheets/si544-datasheet.pdf

[Severity: Low]
Is the capitalization in this URL path correct? The other updated links use
"Skyworks" in the path, but this one uses "SkyWorks". This might result in a
broken link if the web server relies on strict case-sensitivity.

> +    https://www.skyworksinc.com/-/media/Skyworks/SL/documents/public/data-sheets/si549-datasheet.pdf
>  
>  properties:

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729115310.3490589-1-pavel@loebl.cz?part=1

[PATCH v3 2/2] clk: si544: add support for si549

[Pavel Löbl]

Add support for Si549, which only differs from Si544 in internal
oscilator frequency. Si549 also comes in three types A,B and C.
Each having different maximum output frequency.

This patch also fixes maximum frequency of Si544 C. Which should
be 325MHz according to datasheet.

Signed-off-by: Pavel Löbl <pavel@loebl.cz>
---
 drivers/clk/Kconfig     |  6 +--
 drivers/clk/clk-si544.c | 83 ++++++++++++++++++++++++++++++++---------
 2 files changed, 69 insertions(+), 20 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 1717ce75a907..159f546a48f0 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -155,12 +155,12 @@ config COMMON_CLK_SI514
 	  generator.
 
 config COMMON_CLK_SI544
-	tristate "Clock driver for SiLabs 544 devices"
+	tristate "Clock driver for SiLabs 544 and compatible devices"
 	depends on I2C
 	select REGMAP_I2C
 	help
-	  This driver supports the Silicon Labs 544 programmable clock
-	  generator.
+	  This driver supports the Silicon Labs 544/549 programmable clock
+	  generators.
 
 config COMMON_CLK_SI570
 	tristate "Clock driver for SiLabs 570 and compatible devices"
diff --git a/drivers/clk/clk-si544.c b/drivers/clk/clk-si544.c
index 09c06ecec1a5..8e6c2487b79b 100644
--- a/drivers/clk/clk-si544.c
+++ b/drivers/clk/clk-si544.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Driver for Silicon Labs Si544 Programmable Oscillator
+ * Driver for Silicon Labs Si544/Si549 Programmable Oscillator
  * Copyright (C) 2018 Topic Embedded Products
  * Author: Mike Looijmans <mike.looijmans@topic.nl>
  */
@@ -40,7 +40,9 @@
 #define SI544_MIN_FREQ	    200000U
 
 /* Si544 Internal oscillator runs at 55.05 MHz */
-#define FXO		  55050000U
+#define SI544_XO_FREQ	55050000U
+/* Si549 Internal oscillator runs at 152.60 MHz */
+#define SI549_XO_FREQ	152600000U
 
 /* VCO range is 10.8 .. 12.1 GHz, max depends on speed grade */
 #define FVCO_MIN       10800000000ULL
@@ -56,11 +58,16 @@
 #define DELTA_M_FRAC_NUM	19
 #define DELTA_M_FRAC_DEN	20000
 
+struct si544_clk_desc {
+	unsigned long max_freq;
+	unsigned long xo_freq;
+};
+
 struct clk_si544 {
 	struct clk_hw hw;
 	struct regmap *regmap;
 	struct i2c_client *i2c_client;
-	unsigned long  max_freq;
+	const struct si544_clk_desc *chip_info;
 };
 #define to_clk_si544(_hw)	container_of(_hw, struct clk_si544, hw)
 
@@ -72,6 +79,7 @@ struct clk_si544 {
  * @ls_div_bits:	2nd divider, as 2^x, range 0..5
  *                      If ls_div_bits is non-zero, hs_div must be even
  * @delta_m:		Frequency shift for small -950..+950 ppm changes, 24 bit
+ * @xo_freq:		Internal oscillator frequency
  */
 struct clk_si544_muldiv {
 	u32 fb_div_frac;
@@ -79,6 +87,7 @@ struct clk_si544_muldiv {
 	u16 hs_div;
 	u8 ls_div_bits;
 	s32 delta_m;
+	u32 xo_freq;
 };
 
 /* Enables or disables the output driver */
@@ -145,6 +154,8 @@ static int si544_get_muldiv(struct clk_si544 *data,
 	settings->delta_m = reg[0] << 8 | reg[1] << 16 | reg[2] << 24;
 	settings->delta_m >>= 8;
 
+	settings->xo_freq = data->chip_info->xo_freq;
+
 	return 0;
 }
 
@@ -193,7 +204,7 @@ static bool is_valid_frequency(const struct clk_si544 *data,
 	if (frequency < SI544_MIN_FREQ)
 		return false;
 
-	return frequency <= data->max_freq;
+	return frequency <= data->chip_info->max_freq;
 }
 
 /* Calculate divider settings for a given frequency */
@@ -201,6 +212,7 @@ static int si544_calc_muldiv(struct clk_si544_muldiv *settings,
 	unsigned long frequency)
 {
 	u64 vco;
+	u32 fxo = settings->xo_freq;
 	u32 ls_freq;
 	u32 tmp;
 	u8 res;
@@ -238,13 +250,13 @@ static int si544_calc_muldiv(struct clk_si544_muldiv *settings,
 	vco = (u64)ls_freq * settings->hs_div;
 
 	/* Calculate the integer part of the feedback divider */
-	tmp = do_div(vco, FXO);
+	tmp = do_div(vco, fxo);
 	settings->fb_div_int = vco;
 
 	/* And the fractional bits using the remainder */
 	vco = (u64)tmp << 32;
-	vco += FXO / 2; /* Round to nearest multiple */
-	do_div(vco, FXO);
+	vco += fxo / 2; /* Round to nearest multiple */
+	do_div(vco, fxo);
 	settings->fb_div_frac = vco;
 
 	/* Reset the frequency adjustment */
@@ -258,15 +270,16 @@ static unsigned long si544_calc_center_rate(
 		const struct clk_si544_muldiv *settings)
 {
 	u32 d = settings->hs_div * BIT(settings->ls_div_bits);
+	u32 fxo = settings->xo_freq;
 	u64 vco;
 
 	/* Calculate VCO from the fractional part */
-	vco = (u64)settings->fb_div_frac * FXO;
-	vco += (FXO / 2);
+	vco = (u64)settings->fb_div_frac * fxo;
+	vco += (fxo / 2);
 	vco >>= 32;
 
 	/* Add the integer part of the VCO frequency */
-	vco += (u64)settings->fb_div_int * FXO;
+	vco += (u64)settings->fb_div_int * fxo;
 
 	/* Apply divider to obtain the generated frequency */
 	do_div(vco, d);
@@ -446,7 +459,7 @@ static int si544_probe(struct i2c_client *client)
 	init.num_parents = 0;
 	data->hw.init = &init;
 	data->i2c_client = client;
-	data->max_freq = (uintptr_t)i2c_get_match_data(client);
+	data->chip_info = i2c_get_match_data(client);
 
 	if (of_property_read_string(client->dev.of_node, "clock-output-names",
 			&init.name))
@@ -478,18 +491,54 @@ static int si544_probe(struct i2c_client *client)
 	return 0;
 }
 
+static const struct si544_clk_desc clk_si544a_info = {
+	.xo_freq = SI544_XO_FREQ,
+	.max_freq = 1500000000,
+};
+
+static const struct si544_clk_desc clk_si544b_info = {
+	.xo_freq = SI544_XO_FREQ,
+	.max_freq = 800000000,
+};
+
+static const struct si544_clk_desc clk_si544c_info = {
+	.xo_freq = SI544_XO_FREQ,
+	.max_freq = 325000000,
+};
+
+static const struct si544_clk_desc clk_si549a_info = {
+	.xo_freq = SI549_XO_FREQ,
+	.max_freq = 1500000000,
+};
+
+static const struct si544_clk_desc clk_si549b_info = {
+	.xo_freq = SI549_XO_FREQ,
+	.max_freq = 800000000,
+};
+
+static const struct si544_clk_desc clk_si549c_info = {
+	.xo_freq = SI549_XO_FREQ,
+	.max_freq = 325000000,
+};
+
 static const struct i2c_device_id si544_id[] = {
-	{ "si544a", 1500000000 },
-	{ "si544b", 800000000 },
-	{ "si544c", 350000000 },
+	{ "si544a", (kernel_ulong_t)&clk_si544a_info },
+	{ "si544b", (kernel_ulong_t)&clk_si544b_info },
+	{ "si544c", (kernel_ulong_t)&clk_si544c_info },
+	{ "si549a", (kernel_ulong_t)&clk_si549a_info },
+	{ "si549b", (kernel_ulong_t)&clk_si549b_info },
+	{ "si549c", (kernel_ulong_t)&clk_si549c_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, si544_id);
 
 static const struct of_device_id clk_si544_of_match[] = {
-	{ .compatible = "silabs,si544a", .data = (void *)1500000000 },
-	{ .compatible = "silabs,si544b", .data = (void *)800000000 },
-	{ .compatible = "silabs,si544c", .data = (void *)350000000 },
+	{ .compatible = "silabs,si544a", .data = &clk_si544a_info },
+	{ .compatible = "silabs,si544b", .data = &clk_si544b_info },
+	{ .compatible = "silabs,si544c", .data = &clk_si544c_info },
+	{ .compatible = "silabs,si549a", .data = &clk_si549a_info },
+	{ .compatible = "silabs,si549b", .data = &clk_si549b_info },
+	{ .compatible = "silabs,si549c", .data = &clk_si549c_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, clk_si544_of_match);
-- 
2.53.0

Re: [PATCH v3 2/2] clk: si544: add support for si549

[sashiko-bot]

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

New issues:
- [High] Missing NULL check for `i2c_get_match_data()` leads to a NULL pointer dereference.
- [Low] The `si544_id` array uses positional initializers instead of named initializers, violating the I2C subsystem rules.

Pre-existing issues:
- [High] Unconditional dereference of `client->dev.of_node` causes a kernel panic on non-DT systems.
- [High] Division by zero occurs in `si544_calc_center_rate()` if the hardware returns 0 for the `HS_DIV` register.
--

--- Patch [2]: [PATCH v3 2/2] clk: si544: add support for si549 ---
commit 12920363f4810dc248ea69506597ff1f3d421e35
Author: Pavel Löbl <pavel@loebl.cz>

clk: si544: add support for si549

This patch adds support for the Si549 programmable clock generator, which
differs from the Si544 primarily in internal oscillator frequency. It
introduces three types (A, B, and C) with different maximum output
frequencies and fixes the maximum frequency for the Si544 C variant.

> diff --git a/drivers/clk/clk-si544.c b/drivers/clk/clk-si544.c
> index 09c06ecec1a52..8e6c2487b79b8 100644
> --- a/drivers/clk/clk-si544.c
> +++ b/drivers/clk/clk-si544.c

[ ... ]

> @@ -258,15 +270,16 @@ static unsigned long si544_calc_center_rate(
>  		const struct clk_si544_muldiv *settings)
>  {
>  	u32 d = settings->hs_div * BIT(settings->ls_div_bits);
> +	u32 fxo = settings->xo_freq;
>  	u64 vco;
>  
>  	/* Calculate VCO from the fractional part */
> -	vco = (u64)settings->fb_div_frac * FXO;
> -	vco += (FXO / 2);
> +	vco = (u64)settings->fb_div_frac * fxo;
> +	vco += (fxo / 2);
>  	vco >>= 32;
>  
>  	/* Add the integer part of the VCO frequency */
> -	vco += (u64)settings->fb_div_int * FXO;
> +	vco += (u64)settings->fb_div_int * fxo;
>  
>  	/* Apply divider to obtain the generated frequency */
>  	do_div(vco, d);

[Severity: High]
This is a pre-existing issue, but can this result in a division by zero if
the hardware returns 0 for the HS_DIV register?

During driver probe, si544_get_muldiv() reads the HS_DIV register from the
I2C device. If the hardware is uninitialized or malfunctioning, settings->hs_div
evaluates to 0:

si544_get_muldiv() {
    ...
    settings->hs_div = (reg[1] & 0x07) << 8 | reg[0];
    ...
}

This would cause the divisor d to be 0 here in si544_calc_center_rate(),
leading to a division by zero when executing do_div(vco, d).

[ ... ]

> @@ -446,7 +459,7 @@ static int si544_probe(struct i2c_client *client)
>  	init.num_parents = 0;
>  	data->hw.init = &init;
>  	data->i2c_client = client;
> -	data->max_freq = (uintptr_t)i2c_get_match_data(client);
> +	data->chip_info = i2c_get_match_data(client);

[Severity: High]
Is it possible to hit a NULL pointer dereference here if the driver is bound
manually via sysfs?

If the driver is bound without a matching id_table entry (for example, via
driver_override), i2c_get_match_data() can return NULL. This NULL pointer is
then unconditionally assigned to data->chip_info.

Later in si544_get_muldiv(), the unvalidated pointer is dereferenced:

si544_get_muldiv() {
    ...
    settings->xo_freq = data->chip_info->xo_freq;
    ...
}

Could a NULL check be added for data->chip_info before continuing the probe?

>  
>  	if (of_property_read_string(client->dev.of_node, "clock-output-names",
>  			&init.name))
>  		init.name = client->dev.of_node->name;

[Severity: High]
This isn't a bug introduced by this patch, but could this fallback path
trigger a NULL pointer dereference on non-Device Tree systems?

On platforms like ACPI or board files where client->dev.of_node is NULL,
of_property_read_string() safely handles the NULL pointer and returns -ENOSYS.
This causes the if condition to evaluate to true.

The fallback path then unconditionally dereferences client->dev.of_node to
access its name, which will cause a kernel panic if it is NULL.

[ ... ]

> @@ -478,18 +491,54 @@ static int si544_probe(struct i2c_client *client)
>  	return 0;
>  }

[ ... ]

>  static const struct i2c_device_id si544_id[] = {
> -	{ "si544a", 1500000000 },
> -	{ "si544b", 800000000 },
> -	{ "si544c", 350000000 },
> +	{ "si544a", (kernel_ulong_t)&clk_si544a_info },
> +	{ "si544b", (kernel_ulong_t)&clk_si544b_info },
> +	{ "si544c", (kernel_ulong_t)&clk_si544c_info },
> +	{ "si549a", (kernel_ulong_t)&clk_si549a_info },
> +	{ "si549b", (kernel_ulong_t)&clk_si549b_info },
> +	{ "si549c", (kernel_ulong_t)&clk_si549c_info },
>  	{ }
>  };

[Severity: Low]
Are these array initializations compliant with the I2C subsystem requirements?

The I2C subsystem guidelines state that initialized arrays of type
struct i2c_device_id must use named initializers, but these new entries use
positional initializers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729115310.3490589-1-pavel@loebl.cz?part=2