Re: [PATCH 2/4] dt-bindings: clock: add binding header for sf21-topcrm

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The dt-bindings header file exposes software-specific `MAX` macros (`SF21_CLK_MAX` and `SF21_RESET_MAX`).
--

commit b39e2fdfa1953205f67f3e13ae23a09e43917c11
Author: Chuanhong Guo <gch981213@gmail.com>

dt-bindings: clock: add binding header for sf21-topcrm

Add the device tree binding header for Siflower SF21A6826/SF21H8898
toplevel clock and reset module. The header covers both clock and
reset IDs provided by the block.

> diff --git a/include/dt-bindings/clock/siflower,sf21-topcrm.h b/include/dt-bindings/clock/siflower,sf21-topcrm.h
> new file mode 100644
> index 0000000000000..3690b3452501e
> --- /dev/null
> +++ b/include/dt-bindings/clock/siflower,sf21-topcrm.h
[ ... ]
> +#define SF21_CLK_PCIE_REFN		29
> +#define SF21_CLK_PCIE_REFP		30
> +#define SF21_CLK_MAX			31

Should this MAX macro be exposed in the dt-bindings header?

Device tree binding headers define the hardware ABI, and the total number
of clocks is an internal software implementation detail. Changing this
value when new clocks are added in the future breaks ABI compatibility for
components compiled against the older header.

Could this macro be defined internally within the driver source code instead?

> +
> +#define SF21_RESET_GIC			0
[ ... ]
> +#define SF21_RESET_NPU2DDR_ASYNCBRIDGE	16
> +#define SF21_RESET_IROM			17
> +#define SF21_RESET_MAX			18

Similarly, should SF21_RESET_MAX be moved to the driver source code to
avoid breaking ABI compatibility if new resets are added?

> +
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-sf21-topcrm-v1-0-438f2e0513ff@gmail.com?part=2