Re: [PATCH RESEND v5 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Yixun Lan <dlan@gentoo.org>]

Hi Samuel Holland:

On 20:11 Tue 04 Mar     , Samuel Holland wrote:
> Hi Troy,
> 
> On 2025-03-03 3:35 AM, Yixun Lan wrote:
> > On 13:30 Mon 03 Mar     , Troy Mitchell wrote:
> >> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> >> and supports FIFO transmission.
> >>
> >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> >> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
> >> ---
> >>  .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml   | 59 ++++++++++++++++++++++
> >>  1 file changed, 59 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> >> new file mode 100644
> >> index 0000000000000000000000000000000000000000..db49f1f473e6f166f534b276c86b3951d86341c3
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> >> @@ -0,0 +1,59 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: I2C controller embedded in SpacemiT's K1 SoC
> >> +
> >> +maintainers:
> >> +  - Troy Mitchell <troymitchell988@gmail.com>
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: spacemit,k1-i2c
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  interrupts:
> >> +    maxItems: 1
> >> +
> > ..
> >> +  clocks:
> >> +    minItems: 2
> >> +    maxItems: 2
> >> +
> >> +  clock-names:
> >> +    minItems: 2
> >> +    maxItems: 2
> > I'd suggest to give a brief description and explicit clock name here,
> > you can consult marvell,mv64xxx-i2c.yaml for example
> > 
> >> +
> >> +  clock-frequency:
> >> +    description: |
> >> +      K1 support three different modes which running different frequencies
> >> +      standard speed mode: up to 100000 (100Hz)
> >> +      fast speed mode    : up to 400000 (400Hz)
> >> +      high speed mode    : up to 3300000 (3.3Mhz)
> >> +    default: 400000
> >> +    maximum: 3300000
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +  - interrupts
> >> +  - clocks
> >> +
> >> +unevaluatedProperties: false
> >> +
> >> +examples:
> >> +  - |
> >> +    i2c@d4010800 {
> >> +        compatible = "spacemit,k1-i2c";
> >> +        reg = <0xd4010800 0x38>;
> >> +        interrupt-parent = <&plic>;
> >> +        interrupts = <36>;
> >> +        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

> Regards,
> Samuel
> 
> >> +        clock-frequency = <100000>;
> >> +    };
> >> +
> >> +...
> >>
> >> -- 
> >> 2.34.1
> >>
> > 
> 
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55