Re: [PATCH net-next v9 03/15] dpll: add basic SiTime SiT9531x support

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] The user-visible Kconfig help for SIT9531X_DPLL states the driver
  "registers each on-chip PLL as a DPLL device and exposes input/output…

--

dpll: add basic SiTime SiT9531x support

Adds the first piece of a SiTime SiT9531x driver: an I2C driver with a
paged regmap configuration, page/offset register helpers, XO rate
lookup, an optional reset line and variant detection from the ID byte
at page 0 offset 0x02.  No DPLL objects are registered yet.

> diff --git a/drivers/dpll/sit9531x/Kconfig b/drivers/dpll/sit9531x/Kconfig
> new file mode 100644
> index 0000000000000..47aea8674327c
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/Kconfig
> @@ -0,0 +1,17 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config SIT9531X_DPLL
> +	tristate "SiTime SiT9531x DPLL driver"
> +	depends on I2C && NET
> +	select DPLL
> +	select REGMAP_I2C
> +	help
> +	  Driver for SiTime SiT9531x family clock generators
> +	  (SiT95317, SiT95316).
> +
> +	  This driver registers each on-chip PLL as a DPLL device
> +	  and exposes input/output clocks as DPLL pins, providing
> +	  runtime configuration via Generic Netlink.

[Severity: Low]
This isn't a bug, but the help text describes behaviour that does not
exist yet at this commit.  Nothing in this patch includes a DPLL header
or calls a dpll_*() function, and select DPLL pulls the DPLL core in for
a module that does not use it.

Would it be worth moving the "registers each on-chip PLL as a DPLL
device ... via Generic Netlink" paragraph and select DPLL to the patch
that actually performs the registration, so the visible config option
matches the code at every step of the series?

For the record, this resolves later in the series: the final tree adds
dpll.c and prop.c, sit9531x_dpll_device_register() calls
dpll_device_register(), and sit9531x_dev_probe() ends with
sit9531x_devm_dpll_init(), which makes the help text accurate again.

> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sit9531x.
> diff --git a/drivers/dpll/sit9531x/Makefile b/drivers/dpll/sit9531x/Makefile
> new file mode 100644
> index 0000000000000..a221fe55386a9
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_SIT9531X_DPLL) += sit9531x.o
> +sit9531x-y := core.o
> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> new file mode 100644
> index 0000000000000..b0920f76362a1
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -0,0 +1,272 @@

[ ... ]

> +	dev_info(sitdev->dev, "%s detected, %u inputs, %u outputs\n",
> +		 sitdev->info->name, sitdev->info->num_inputs,
> +		 sitdev->info->num_outputs);
> +
> +	return 0;
> +}

Related to the Kconfig comment above: sit9531x_dev_probe() returns right
after the dev_info(), so a device tree node describing dpll-types,
input-pins and output-pins binds successfully with no DPLL device and no
netlink interface, and without any message saying so.  Is a note in the
help text or the commit log about this intermediate state wanted?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915000015.80480-1-arouhi%40sitime.com