Re: [PATCH v3 3/4] riscv: dts: sophgo: add clock generator for Sophgo CV1800 series SoC

[Conor Dooley]

On Thu, Dec 07, 2023 at 01:52:16PM +0100, Krzysztof Kozlowski wrote:
> On 07/12/2023 10:42, Inochi Amaoto wrote:
> >>> +&clk {
> >>> +	compatible = "sophgo,cv1810-clk";
> >>> +};
> >>> diff --git a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
> >>> index 2d6f4a4b1e58..6ea1b2784db9 100644
> >>> --- a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
> >>> +++ b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
> >>> @@ -53,6 +53,12 @@ soc {
> >>>  		dma-noncoherent;
> >>>  		ranges;
> >>>
> >>> +		clk: clock-controller@3002000 {
> >>> +			reg = <0x03002000 0x1000>;
> >>> +			clocks = <&osc>;
> >>> +			#clock-cells = <1>;
> >>
> >> I don't find such layout readable and maintainable. I did some parts
> >> like this long, long time ago for one of my SoCs (Exynos54xx), but I
> >> find it over time unmaintainable approach. I strongly suggest to have
> >> compatible and other properties in one place, so cv1800 and cv1812, even
> >> if it duplicates the code.
> >>
> > 
> > Hi Krzysztof:
> > 
> > Thanks for your advice, but I have a question about this: when I should
> > use the DT override? The memory mapping of the CV1800 and CV1810 are
> > almost the same (the CV1810 have more peripheral and the future SG200X
> > have the same layout). IIRC, this is why conor suggested using DT override
> > to make modification easier. But duplicating node seems to break thiS, so
> > I's pretty confused.
> 
> Go with whatever your subarchitecture and architecture maintainers
> prefer, I just shared my opinion that I find such code difficult to read
> and maintain.
> 
> Extending node with supplies, pinctrl or even clocks would be readable.
> But the compatible: no. The same applies when you need to delete
> property or subnode: not readable/maintainable IMHO.

There are apparently 3 or 4 of these SoCs that are basically identical,
which is why the approach was taken. I do agree that it looks somewhat
messy because I was looking for device-specific compatibles for these
SoCs.

Attachment: signature.asc
Description: PGP signature