Re: [PATCH v9 6/8] riscv: dts: spacemit: enable the i2c8 adapter

[Alex Elder]

On 7/24/25 4:11 PM, Yixun Lan wrote:
> Hi Alex,
>
> On 15:25 Thu 24 Jul     , Alex Elder wrote:
> > Define properties for the I2C adapter that provides access to the
> > SpacemiT P1 PMIC.  Enable this adapter on the Banana Pi BPI-F3.
> >
> > Signed-off-by: Alex Elder <elder@xxxxxxxxxxxx>
> > ---
> >   arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts | 15 +++++++++++++++
> >   arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi    |  7 +++++++
> >   arch/riscv/boot/dts/spacemit/k1.dtsi            | 11 +++++++++++
> >   3 files changed, 33 insertions(+)
> >
> > diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> > index fe22c747c5012..7c9f91c88e01a 100644
> > --- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> > +++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> > @@ -40,6 +40,21 @@ &emmc {
> >   	status = "okay";
> >   };
> >   
> > +&i2c8 {
> > +	pinctrl-0 = <&i2c8_cfg>;
> > +	pinctrl-names = "default";
> ..
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> I think these two can be moved into dtsi, as they are
> common and fixed properties for the i2c controller

You're right.  I will update this in v10, which I will
send later today.  I'd like to hear from Lee before I
prepare that though.

> > +	status = "okay";
> > +
> > +	pmic@41 {
> > +		compatible = "spacemit,p1";
> > +		reg = <0x41>;
> > +		interrupts = <64>;
> ..
> > +		status = "okay";
> status property here can be dropped as enabled by default

OK.

> > +	};
> > +};
> > +
> >   &uart0 {
> >   	pinctrl-names = "default";
> >   	pinctrl-0 = <&uart0_2_cfg>;
> > diff --git a/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi b/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
> > index 3810557374228..96d7a46d4bf77 100644
> > --- a/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
> > +++ b/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
> > @@ -11,6 +11,13 @@
> >   #define K1_GPIO(x)	(x / 32) (x % 32)
> >   
> >   &pinctrl {
> > +	i2c8_cfg: i2c8-cfg {
> > +		i2c8-0-pins {
> > +			pinmux = <K1_PADCONF(93, 0)>,	/* PWR_SCL */
> > +				 <K1_PADCONF(94, 0)>;	/* PWR_SDA */
> > +		};
> > +	};
> > +
> >   	uart0_2_cfg: uart0-2-cfg {
> >   		uart0-2-pins {
> >   			pinmux = <K1_PADCONF(68, 2)>,
> > diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
> > index abde8bb07c95c..2a5a132d5a774 100644
> > --- a/arch/riscv/boot/dts/spacemit/k1.dtsi
> > +++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
> > @@ -459,6 +459,17 @@ pwm7: pwm@d401bc00 {
> >   			status = "disabled";
> >   		};
> >   
> > +		i2c8: i2c@d401d800 {
> > +			compatible = "spacemit,k1-i2c";
> > +			reg = <0x0 0xd401d800 0x0 0x38>;
> ..
> > +			interrupts = <19>;
> I'd suggest to move interrupts property after clock, see my similar
> comment
> https://lore.kernel.org/all/20250724121916-GYA748228@gentoo/

OK.

					-Alex

> > +			clocks = <&syscon_apbc CLK_TWSI8>,
> > +				 <&syscon_apbc CLK_TWSI8_BUS>;
> > +			clock-names = "func", "bus";
> > +			clock-frequency = <400000>;
> > +			status = "disabled";
> > +		};
> > +
> >   		pinctrl: pinctrl@d401e000 {
> >   			compatible = "spacemit,k1-pinctrl";
> >   			reg = <0x0 0xd401e000 0x0 0x400>;
> > --
> > 2.43.0