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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] Orphan kernel-doc-style block for `sit9531x_input_get_regs()` in
  drivers/dpll/sit9531x/core.c (lines 147-155) documents a function that…
- [Low] The Kconfig help for SIT9531X_DPLL advertises DPLL device
  registration, DPLL pin exposure and Generic Netlink configuration, and…

--

dpll: add basic SiTime SiT9531x support

This adds the initial SiTime SiT9531x I2C clock generator driver: a paged
regmap configuration for the 32x256 register space selected through offset
0xFF, page/offset register access helpers, and a probe that fetches the
xtal rate, takes the optional reset line and identifies the chip variant.

Two small observations below, both cosmetic.

> diff --git a/drivers/dpll/sit9531x/Kconfig b/drivers/dpll/sit9531x/Kconfig
> new file mode 100644
> index 000000000000..47aea8674327
> --- /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 only arrives
later in the series.  At this commit sit9531x_dev_probe() stops after
identifying the variant:

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

	return 0;

There is no dpll_device_register() or dpll_pin_register() call yet, and
select DPLL has no consumer, so enabling CONFIG_SIT9531X_DPLL at this point
in the series binds the I2C node without exposing any DPLL object.

The later patches in this series do add the DPLL device and pin
registration, so the help text matches the final state of the series;
should the Kconfig entry land with the patch that implements it instead?

[ ... ]

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> new file mode 100644
> index 000000000000..f355b08aebca
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> +/*
> + * sit9531x_input_get_regs - get force mask and state register addresses
> + * @index:	logical input index
> + * @force_reg:	output force mask register address
> + * @state_reg:	output state register address
> + *
> + * Selects the correct Page 0x02 register pair based on the pair's
> + * signal mode and the lane (P/N) the index refers to.
> + */
> +
> +static int sit9531x_read_variant_id(struct sit9531x_dev *sitdev, u8 *id)
> +{
> +	return sit9531x_read_u8(sitdev, SIT9531X_REG_VARIANT_ID, id);
> +}

[Severity: Low]
This isn't a bug, but the comment block for sit9531x_input_get_regs()
describes a function that does not exist in this patch, and it sits
directly above the unrelated sit9531x_read_variant_id().  A reader here
would attribute the Page 0x02 register-pair semantics to the variant-ID
helper.

The function it documents is added later in the series, by "dpll:
sit9531x: implement input pin state on a DPLL", where it appears right
under this same comment.  Could the comment move to that patch?

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