Re: [PATCH 19/20] dt-bindings: arm: berlin: Add Synaptics SL261X SoC and RDK board

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] pinctrl: register corruption due to wrong offset for GPIO54 and SM_GPIO20
- [Medium] reset: soc reset driver fails to probe due to compatible string mismatch
- [Medium] clk: missing clock provider registration in sl261x_aio_clk_setup()
--

commit efe9a4b32a9836b222023aba07e846238a729d6a
Author: Jisheng Zhang <jszhang@kernel.org>

dt-bindings: arm: berlin: Add Synaptics SL261X SoC and RDK board
    
Add devicetree binding for the Synaptics SL261X SoCs and corresponding
RDK boards.

> diff --git a/drivers/clk/berlin/sl261x-clks.c b/drivers/clk/berlin/sl261x-clks.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/berlin/sl261x-clks.c
[ ... ]
> +static int sl261x_aio_clk_setup(struct platform_device *pdev)
> +{
[ ... ]
> +	/* aplls */
> +	ret = sl261x_pll_register(dev, base + APLL0_OFFSET,
> +				  clk_names[APLL0_OUT], clk_names[APLL0_OUTF]);
> +	if (ret)
> +		return ret;
> +	ret = sl261x_pll_register(dev, base + APLL1_OFFSET,
> +				  clk_names[APLL1_OUT], clk_names[APLL1_OUTF]);
> +	if (ret)
> +		return ret;
> +
> +	/* TODO aio clks */
> +
> +	return 0;
> +}

[Severity: Medium]
Since the device tree node for this clock defines #clock-cells = <1>, doesn't
the clock framework expect a provider to be registered here?

It appears sl261x_aio_clk_setup() returns 0 without calling
devm_of_clk_add_hw_provider(), which could cause any consumer requesting a
clock from the syna,sl261x-aio-clk provider to fail with a probe deferral
error.

> diff --git a/drivers/pinctrl/berlin/pinctrl-sl261x.c b/drivers/pinctrl/berlin/pinctrl-sl261x.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pinctrl/berlin/pinctrl-sl261x.c
[ ... ]
> +static const struct berlin_desc_group sl261x_soc_pinctrl_groups[] = {
[ ... ]
> +	BERLIN_PINCTRLCONF_GROUP("GPIO54", 0x800c, 0x3, 0x03,
> +			0x87c, 10,

[Severity: High]
Is the configuration offset for GPIO54 correct in sl261x_soc_pinctrl_groups?

Surrounding pins like GPIO53 use 0x8878 and GPIO55 uses 0x8880. If this was
meant to be 0x887c, wouldn't writing to 0x87c during pin configuration corrupt
unrelated system registers instead?

[ ... ]
> +static const struct berlin_desc_group sl261x_sysmgr_pinctrl_groups[] = {
[ ... ]
> +	BERLIN_PINCTRLCONF_GROUP("SM_GPIO20", 0x8, 0x3, 0x0f,
> +			0xc6c, 10,

[Severity: High]
Could the multiplexing offset for SM_GPIO20 in sl261x_sysmgr_pinctrl_groups
be a typo?

All surrounding pins use a multiplexing offset of 0xb08. Executing pinmux
requests with an offset of 0x8 might lead to system register corruption.

> diff --git a/drivers/reset/reset-sl261x.c b/drivers/reset/reset-sl261x.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/reset/reset-sl261x.c
[ ... ]
> +static const struct of_device_id sl261x_reset_dt_match[] = {
> +	{ .compatible = "syna,sl261x-reset", .data = &sl261x_reset_data },
> +	{ .compatible = "syna,sl261x-system-reset", .data = &sl261x_system_reset_data },
> +	{ },
> +};

[Severity: Medium]
Will this driver successfully bind to the SoC reset node during boot?

The sl261x_reset_dt_match table matches against syna,sl261x-reset, but the
devicetree bindings and device tree sources define the compatible string as
syna,sl261x-soc-reset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929061417.20733-1-jszhang@kernel.org?part=19