Am Mittwoch, 14. Oktober 2026, 09:20:44 Mitteleuropäische Sommerzeit schrieb Navid Ghahremani: > Add Device Tree binding schemas for the ZTE ZX279128S clock controllers: > topcrm, lspcrpm, and crm2. Define fixed clock lists and simple-mfd > for child nodes inside topcrm. This would be easier to review if it were split in 3 patches, one yaml per patch. I'll try to comment on the details anyway. > +properties: > + compatible: > + oneOf: > + - const: zte,zx279128s-crm2 > + - items: > + - const: zte,zx279128s-crm2 > + - const: syscon Afaiu this makes "syscon" optional. Why do you need syscon? Why might you not need syscon? The goal should be to avoid "syscon" as much as possible. The meaning of the syscon compatible is that a different driver will access these registers directly instead of going through a properly exported interface like #clock- cells. Is there anything in crm2 that is not a clock or reset? > a/Documentation/devicetree/bindings/clock/zte,zx279128s-lspcrpm.yaml > b/Documentation/devicetree/bindings/clock/zte,zx279128s-lspcrpm.yaml new > +maintainers: > + - Stefan Dösinger > + - Navid Ghahremani I think you should be the maintainer alone here, I don't have the hardware to do proper testing of patches. > +description: | > + Each of the two groups of low speed peripherals (LSP) has a peripheral > clock + module (LSP CRPM) of its own, fed by the top CRM. > + > +properties: > + compatible: > + enum: > + - zte,zx279128s-lsp0crpm > + - zte,zx279128s-lsp1crpm I think splitting those into different bindings is still a viable choice. Yeah, ZTE gave them very similar names, but from what I can see the two controllers have nothing in common. > +patternProperties: > + "^phy@[01]$": > + type: object > + description: PCIe PHY child nodes You can drop the phy child node and just add "#phy-cells=1". See https:// docs.kernel.org/devicetree/bindings/writing-bindings.html : "DON’T create nodes just for the sake of instantiating drivers. Multi-function devices only need child nodes when the child nodes have their own DT resources. A single node can be multiple providers (e.g. clocks and resets)." In practical terms, putting a child node into the DT makes this a public ABI contract. The alternative is to split the device with an MFD driver or using the aux bus. In the latter case, what components a device consists of is a driver implementation detail that can easily be changed later. The PHY subsystem has a quirk that devm_of_phy_provider_register wants the device node to have a DT node, which is not the case if the device is an MFD child. device_set_of_node_from_dev(dev, dev->parent); helps. The other purpose of having a DT child node is if a sub-component is likely to be used in a different piece of hardware. syscon-reboot is a pretty generic example of that. > --- /dev/null > +++ b/include/dt-bindings/clock/zte,zx279128s-crm.h I'd suggest to rename clock/zte,zx279128s-crm.h to clock/zte,zx279128s-clk.h. "CRM" implies clock-reset-manager, but the -reset.h header has the reset handles, so the "crm.h" header won't have them. > +#define ZX279128S_TOP_PLL_A9 0 > +#define ZX279128S_TOP_PLL_LSP 1 > +#define ZX279128S_TOP_CPU 2 Do they all need to be exported? You only need those that will be used for a clock = <&topcrm ZX279128S_TOP_SOMETHING> handle. That could e.g. be the CPU wclk and pclk, but the parent PLL itself can remain a driver detail. Most likely all your device clocks have a gate somewhere. In my zx29 case I only export gates from the clock driver, with the exception of the Cortex M0 mux - the Cortex M0 can't be turned off, so it has no gate. I realize that a lot of devices may be missing from the DTSI in patch 23. > +/* Low speed peripherals 0 */ > +#define ZX279128S_LSP0_UART0_WCLK 0 > +#define ZX279128S_LSP0_UART0_PCLK 1 > +#define ZX279128S_LSP0_UART1_WCLK 2 > +#define ZX279128S_LSP0_UART1_PCLK 3 > +#define ZX279128S_LSP0_SPI_WCLK 4 > +#define ZX279128S_LSP0_SPI_PCLK 5 > +#define ZX279128S_LSP0_GPIO_PCLK 6 E.g. exporting those intuitively makes sense.