All of lore.kernel.org
 help / color / mirror / Atom feed
From: Yao Zi <ziyao@disroot.org>
To: Troy Mitchell <troy.mitchell@linux.spacemit.com>,
	Yixun Lan <dlan@gentoo.org>
Cc: Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>, Alex Elder <elder@riscstar.com>,
	Haylen Chu <heylenay@4d2.org>,
	Inochi Amaoto <inochiama@outlook.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	linux-clk@vger.kernel.org, linux-riscv@lists.infradead.org,
	spacemit@lists.linux.dev, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v2 2/2] dt-bindings: clock: spacemit: CLK_SSPA_I2S_BCLK for SSPA
Date: Tue, 22 Jul 2025 10:09:23 +0000	[thread overview]
Message-ID: <aH9jUyzGb53B-aOu@pie> (raw)
In-Reply-To: <B79CAF56E128A7B3+aH9Zb9zNm6y_ovvW@LT-Guozexi>

On Tue, Jul 22, 2025 at 05:27:11PM +0800, Troy Mitchell wrote:
> On Tue, Jul 22, 2025 at 05:21:01PM +0800, Yixun Lan wrote:
> > Hi Troy,
> > 
> > On 15:36 Tue 22 Jul     , Troy Mitchell wrote:
> > > This patch adds macro definitions: SSPAx_I2S_BCLK,
> > this is obvious, so no need to repeat, please add something useful
> > 
> > > to introduce a dummy gate for i2s_bclk.
> >                 ~~~~~~ 'virtual'? if it isn't a real gate clock,
> > 	but I'm not sure it's a good approach to introduce such
> > 	virtual clock if underlying hw doesn't comply with this
> I'll leave this question to others.
> 
> > 
> > > 
> > > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> > > ---
> > >  include/dt-bindings/clock/spacemit,k1-syscon.h | 114 +++++++++++++------------
> > >  1 file changed, 58 insertions(+), 56 deletions(-)
> > dt-binding patch should always go first
> get it
> 
> > 
> > > 
> > > diff --git a/include/dt-bindings/clock/spacemit,k1-syscon.h b/include/dt-bindings/clock/spacemit,k1-syscon.h
> > > index 35968ae98246609c889eb4a7d08b4ff7360de53b..6914ccf5be45a1071d5b6eac354cacb67888e00c 100644
> > > --- a/include/dt-bindings/clock/spacemit,k1-syscon.h
> > > +++ b/include/dt-bindings/clock/spacemit,k1-syscon.h
> > > @@ -123,62 +123,64 @@
> > >  #define CLK_TIMERS2		41
> > >  #define CLK_AIB			42
> > >  #define CLK_ONEWIRE		43
> > > -#define CLK_SSPA0		44
> > > -#define CLK_SSPA1		45
> > > -#define CLK_DRO			46
> > > -#define CLK_IR			47
> > > -#define CLK_TSEN		48
> > > -#define CLK_IPC_AP2AUD		49
> > > -#define CLK_CAN0		50
> > > -#define CLK_CAN0_BUS		51
> > > -#define CLK_UART0_BUS		52
> > > -#define CLK_UART2_BUS		53
> > > -#define CLK_UART3_BUS		54
> > > -#define CLK_UART4_BUS		55
> > > -#define CLK_UART5_BUS		56
> > > -#define CLK_UART6_BUS		57
> > > -#define CLK_UART7_BUS		58
> > > -#define CLK_UART8_BUS		59
> > > -#define CLK_UART9_BUS		60
> > > -#define CLK_GPIO_BUS		61
> > > -#define CLK_PWM0_BUS		62
> > > -#define CLK_PWM1_BUS		63
> > > -#define CLK_PWM2_BUS		64
> > > -#define CLK_PWM3_BUS		65
> > > -#define CLK_PWM4_BUS		66
> > > -#define CLK_PWM5_BUS		67
> > > -#define CLK_PWM6_BUS		68
> > > -#define CLK_PWM7_BUS		69
> > > -#define CLK_PWM8_BUS		70
> > > -#define CLK_PWM9_BUS		71
> > > -#define CLK_PWM10_BUS		72
> > > -#define CLK_PWM11_BUS		73
> > > -#define CLK_PWM12_BUS		74
> > > -#define CLK_PWM13_BUS		75
> > > -#define CLK_PWM14_BUS		76
> > > -#define CLK_PWM15_BUS		77
> > > -#define CLK_PWM16_BUS		78
> > > -#define CLK_PWM17_BUS		79
> > > -#define CLK_PWM18_BUS		80
> > > -#define CLK_PWM19_BUS		81
> > > -#define CLK_SSP3_BUS		82
> > > -#define CLK_RTC_BUS		83
> > > -#define CLK_TWSI0_BUS		84
> > > -#define CLK_TWSI1_BUS		85
> > > -#define CLK_TWSI2_BUS		86
> > > -#define CLK_TWSI4_BUS		87
> > > -#define CLK_TWSI5_BUS		88
> > > -#define CLK_TWSI6_BUS		89
> > > -#define CLK_TWSI7_BUS		90
> > > -#define CLK_TWSI8_BUS		91
> > > -#define CLK_TIMERS1_BUS		92
> > > -#define CLK_TIMERS2_BUS		93
> > > -#define CLK_AIB_BUS		94
> > > -#define CLK_ONEWIRE_BUS		95
> > > -#define CLK_SSPA0_BUS		96
> > > -#define CLK_SSPA1_BUS		97
> > > -#define CLK_TSEN_BUS		98
> > > -#define CLK_IPC_AP2AUD_BUS	99
> > > +#define CLK_SSPA0_I2S_BCLK	44
> > > +#define CLK_SSPA1_I2S_BCLK	45
> > just append the clock at the end, instead of doing massive renaming
> The c file is written in the same order as the h file,
> and I don't want to disrupt this order..
> 
> So should I keep this style or just append at the end?

This isn't only a style issue. This patch breaks devicetree ABI, old
devicetree won't work with the change. Please don't modify existing
definitions but only add new ones.

>                 - Troy
> > 
> > > +#define CLK_SSPA0		46
> > > +#define CLK_SSPA1		47
> > > +#define CLK_DRO			48

Regards,
Yao Zi

WARNING: multiple messages have this Message-ID (diff)
From: Yao Zi <ziyao@disroot.org>
To: Troy Mitchell <troy.mitchell@linux.spacemit.com>,
	Yixun Lan <dlan@gentoo.org>
Cc: Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>, Alex Elder <elder@riscstar.com>,
	Haylen Chu <heylenay@4d2.org>,
	Inochi Amaoto <inochiama@outlook.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	linux-clk@vger.kernel.org, linux-riscv@lists.infradead.org,
	spacemit@lists.linux.dev, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v2 2/2] dt-bindings: clock: spacemit: CLK_SSPA_I2S_BCLK for SSPA
Date: Tue, 22 Jul 2025 10:09:23 +0000	[thread overview]
Message-ID: <aH9jUyzGb53B-aOu@pie> (raw)
In-Reply-To: <B79CAF56E128A7B3+aH9Zb9zNm6y_ovvW@LT-Guozexi>

On Tue, Jul 22, 2025 at 05:27:11PM +0800, Troy Mitchell wrote:
> On Tue, Jul 22, 2025 at 05:21:01PM +0800, Yixun Lan wrote:
> > Hi Troy,
> > 
> > On 15:36 Tue 22 Jul     , Troy Mitchell wrote:
> > > This patch adds macro definitions: SSPAx_I2S_BCLK,
> > this is obvious, so no need to repeat, please add something useful
> > 
> > > to introduce a dummy gate for i2s_bclk.
> >                 ~~~~~~ 'virtual'? if it isn't a real gate clock,
> > 	but I'm not sure it's a good approach to introduce such
> > 	virtual clock if underlying hw doesn't comply with this
> I'll leave this question to others.
> 
> > 
> > > 
> > > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> > > ---
> > >  include/dt-bindings/clock/spacemit,k1-syscon.h | 114 +++++++++++++------------
> > >  1 file changed, 58 insertions(+), 56 deletions(-)
> > dt-binding patch should always go first
> get it
> 
> > 
> > > 
> > > diff --git a/include/dt-bindings/clock/spacemit,k1-syscon.h b/include/dt-bindings/clock/spacemit,k1-syscon.h
> > > index 35968ae98246609c889eb4a7d08b4ff7360de53b..6914ccf5be45a1071d5b6eac354cacb67888e00c 100644
> > > --- a/include/dt-bindings/clock/spacemit,k1-syscon.h
> > > +++ b/include/dt-bindings/clock/spacemit,k1-syscon.h
> > > @@ -123,62 +123,64 @@
> > >  #define CLK_TIMERS2		41
> > >  #define CLK_AIB			42
> > >  #define CLK_ONEWIRE		43
> > > -#define CLK_SSPA0		44
> > > -#define CLK_SSPA1		45
> > > -#define CLK_DRO			46
> > > -#define CLK_IR			47
> > > -#define CLK_TSEN		48
> > > -#define CLK_IPC_AP2AUD		49
> > > -#define CLK_CAN0		50
> > > -#define CLK_CAN0_BUS		51
> > > -#define CLK_UART0_BUS		52
> > > -#define CLK_UART2_BUS		53
> > > -#define CLK_UART3_BUS		54
> > > -#define CLK_UART4_BUS		55
> > > -#define CLK_UART5_BUS		56
> > > -#define CLK_UART6_BUS		57
> > > -#define CLK_UART7_BUS		58
> > > -#define CLK_UART8_BUS		59
> > > -#define CLK_UART9_BUS		60
> > > -#define CLK_GPIO_BUS		61
> > > -#define CLK_PWM0_BUS		62
> > > -#define CLK_PWM1_BUS		63
> > > -#define CLK_PWM2_BUS		64
> > > -#define CLK_PWM3_BUS		65
> > > -#define CLK_PWM4_BUS		66
> > > -#define CLK_PWM5_BUS		67
> > > -#define CLK_PWM6_BUS		68
> > > -#define CLK_PWM7_BUS		69
> > > -#define CLK_PWM8_BUS		70
> > > -#define CLK_PWM9_BUS		71
> > > -#define CLK_PWM10_BUS		72
> > > -#define CLK_PWM11_BUS		73
> > > -#define CLK_PWM12_BUS		74
> > > -#define CLK_PWM13_BUS		75
> > > -#define CLK_PWM14_BUS		76
> > > -#define CLK_PWM15_BUS		77
> > > -#define CLK_PWM16_BUS		78
> > > -#define CLK_PWM17_BUS		79
> > > -#define CLK_PWM18_BUS		80
> > > -#define CLK_PWM19_BUS		81
> > > -#define CLK_SSP3_BUS		82
> > > -#define CLK_RTC_BUS		83
> > > -#define CLK_TWSI0_BUS		84
> > > -#define CLK_TWSI1_BUS		85
> > > -#define CLK_TWSI2_BUS		86
> > > -#define CLK_TWSI4_BUS		87
> > > -#define CLK_TWSI5_BUS		88
> > > -#define CLK_TWSI6_BUS		89
> > > -#define CLK_TWSI7_BUS		90
> > > -#define CLK_TWSI8_BUS		91
> > > -#define CLK_TIMERS1_BUS		92
> > > -#define CLK_TIMERS2_BUS		93
> > > -#define CLK_AIB_BUS		94
> > > -#define CLK_ONEWIRE_BUS		95
> > > -#define CLK_SSPA0_BUS		96
> > > -#define CLK_SSPA1_BUS		97
> > > -#define CLK_TSEN_BUS		98
> > > -#define CLK_IPC_AP2AUD_BUS	99
> > > +#define CLK_SSPA0_I2S_BCLK	44
> > > +#define CLK_SSPA1_I2S_BCLK	45
> > just append the clock at the end, instead of doing massive renaming
> The c file is written in the same order as the h file,
> and I don't want to disrupt this order..
> 
> So should I keep this style or just append at the end?

This isn't only a style issue. This patch breaks devicetree ABI, old
devicetree won't work with the change. Please don't modify existing
definitions but only add new ones.

>                 - Troy
> > 
> > > +#define CLK_SSPA0		46
> > > +#define CLK_SSPA1		47
> > > +#define CLK_DRO			48

Regards,
Yao Zi

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply	other threads:[~2025-07-22 10:09 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-07-22  7:36 [PATCH v2 0/2] clk: spacemit: fix sspax_clk Troy Mitchell
2025-07-22  7:36 ` Troy Mitchell
2025-07-22  7:36 ` [PATCH v2 1/2] " Troy Mitchell
2025-07-22  7:36   ` Troy Mitchell
2025-07-22  7:36 ` [PATCH v2 2/2] dt-bindings: clock: spacemit: CLK_SSPA_I2S_BCLK for SSPA Troy Mitchell
2025-07-22  7:36   ` Troy Mitchell
2025-07-22  9:21   ` Yixun Lan
2025-07-22  9:21     ` Yixun Lan
2025-07-22  9:27     ` Troy Mitchell
2025-07-22  9:27       ` Troy Mitchell
2025-07-22 10:09       ` Yao Zi [this message]
2025-07-22 10:09         ` Yao Zi
2025-07-23  2:31         ` Troy Mitchell
2025-07-23  2:31           ` Troy Mitchell
2025-07-23  4:41   ` Rob Herring
2025-07-23  4:41     ` Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aH9jUyzGb53B-aOu@pie \
    --to=ziyao@disroot.org \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dlan@gentoo.org \
    --cc=elder@riscstar.com \
    --cc=heylenay@4d2.org \
    --cc=inochiama@outlook.com \
    --cc=krzk+dt@kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=mturquette@baylibre.com \
    --cc=robh@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=spacemit@lists.linux.dev \
    --cc=troy.mitchell@linux.spacemit.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.