On 2025-03-04 9:05 PM, Yixun Lan wrote: >>>> + clocks = <&ccu 176>, <&ccu 90>; >>>> + clock-names = "apb", "twsi"; >>> 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST) >>> https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594 >>> from above docs, there are two clocks >>> bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable >>> bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable >>> >>> I'd suggest to name it according to the functionality, thus 'func', 'bus' >>> clock, not its source.. which would make it more system wide consistent >> >> Also in that same register is: >> >> 2 RST RW 0x1 TWSI0 Reset Generation >> This field resets both the APB and functional domain. >> - 0: No Reset >> - 1: Reset >> >> Which means you need a 'resets' property in the binding as well. >> > right, there is reset needed > > I'd suggest to add it as an incremental patch later, when we > implement real reset driver, and also complete the calling reset > consumer API in i2c driver > > but, let me know if this is not the right way to go If you add the resets property later, that's a breaking change to the DT, because existing devicetrees will not have that property. So you would have to make the reset consumer in the driver optional, even if it's not really optional, to work with older DTs. So it is _possible_ to add incrementally, but not recommended because it adds "legacy" code that never really goes away. It's okay to define the binding as requiring the resets property now, even before the reset controller driver is merged. You just won't be able to add the I2C controller to the DTS until the reset controller binding is merged. But since the reset controller is the same IP block as the clock controller, its binding should be available soon anyway. Regards, Samuel
