Skip to content

Conversation

kylebonnici
Copy link
Contributor

@kylebonnici kylebonnici commented Jul 7, 2025

Diff for boards folder as per discussion in #92334

@JarmouniA
Copy link
Contributor

Can you split this into a commit for each vendor, otherwise it takes ages to load.

@kylebonnici kylebonnici force-pushed the dtc/formating-boards branch 2 times, most recently from 0d7d462 to 99d1999 Compare July 8, 2025 10:21
Copy link
Contributor

@mathieuchopstm mathieuchopstm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(non-blocking remarks up to ST boards)

Comment on lines 215 to 216
spi-one-frame = <0xf0>; /* 625 ns high and 625 ns low */
spi-zero-frame = <0xc0>; /* 312.5 ns high and 937.5 ns low */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO both comments should remain aligned.

Copy link
Contributor

@kartben kartben Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this was marked as resolved - maybe it was fixed at some point but currently it's off. Not sure if you're just systematically adding one tab character before the comment, but it's not as simple a that and you should either just make it a simple whitespace and not care about trying to have comments from different lines be flush, or you need something like what we have in the new-ish feature that augments build/zephyr/zephyr.dts with useful comments
afdb62d

Or maybe just don't do anything with comments beside basic whitespace sanitization (i.e clean up any bad mixup of whitespaces and tabs), and stop there?

	/* node '/cpus' defined in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:22 */
	cpus {
		#address-cells = < 0x1 >; /* in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:23 */
		#size-cells = < 0x0 >;    /* in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:24 */

		/* node '/cpus/cpu@0' defined in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:26 */
		cpuapp: cpu: cpu@0 {
			compatible = "arm,cortex-m33f"; /* in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:27 */
			reg = < 0x0 >;                  /* in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:28 */
			device_type = "cpu";            /* in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:29 */
			clocks = < &hfpll >;            /* in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:30 */
			#address-cells = < 0x1 >;       /* in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:31 */
			#size-cells = < 0x1 >;          /* in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:32 */

			/* node '/cpus/cpu@0/itm@e0000000' defined in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:34 */
			itm: itm@e0000000 {
				compatible = "arm,armv8m-itm";     /* in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:35 */
				reg = < 0xe0000000 0x1000 >;       /* in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:36 */
				swo-ref-frequency = < 0x7a12000 >; /* in zephyr/dts/vendor/nordic/nrf54l_05_10_15.dtsi:37 */
			};
		};
	};

Copy link
Contributor Author

@kylebonnici kylebonnici Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had marked it as resolved as it is the same many another's.there was one where I asked what I should do with comments i tagged you in that thread ... apologies if marking as resolved was not the right thing to do. If can you reply where I tagged you it would help me follow what to do next.

I am open to any change I just need to know which ones to do

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think going with a single whitespace is better than adding a hard tab in the middle of the line.
While aligning comments is nice, the proposal of using a hard tab doesn't always achieve alignment, so keep it simple by just requiring a single space before comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have rebased and changed the rule to as space as per the above suggestions.

Copy link
Contributor

@mathieuchopstm mathieuchopstm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two blockers on ST boards + one nit.

@mathieuchopstm mathieuchopstm added area: Boards area: Shields Shields (add-on boards) labels Jul 9, 2025
@kylebonnici
Copy link
Contributor Author

kylebonnici commented Jul 10, 2025

I have seen the feedback and aligning comment over different lines is not trivial and IMO not practical.

Currently the formatter will make sure that there is a tab between the comment first token and prev token when prev token is on the same line. If prev token is not on the same line than comment is will be aligned the same way the value would be

node{
	prop = <10
	       /* align as if it is a value like 1o and 20 */
	       20
	       >,
	       /* align as if it as a value like <10 20> */
	       <10 20>;
}

Keep in mine user can have multiple values on same line like <10 20> and also <10 20>, <10 20>

hence consider the below:

node{
	prop = <10 20 30	/* foo */
	        /* align as if it is a value like 1o and 20 */
	        40	/* bar */  50 	/* foo */
	       >,
	       /* align as if it as a value like <10 20> */
	       <10 20>,	/* foo */  <20 30>, 	/* bar */;
}

Not to mention what if comment is before the value like

node{
	prop = <10
	        /* align as if it is a value like 1o and 20 */
	        /* foo */ 20
	       >,
	       /* align as if it as a value like <10 20> */
	       <10 20>;
}

If we are to consider the suggested changes by @mathieuchopstm I will need contribution in the form of concrete rules on what to for every use case possible.

Copy link
Contributor

@pillo79 pillo79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this effort and the linter work, really appreciated, thanks! 👍 🙇
Minor questions/feedback on the new formatting on the Arduino DTS files.

Comment on lines 101 to 102
pinctrl-0 = <
&eth_ref_clk_pa1
&eth_crs_dv_pa7
&eth_rxd0_pc4
&eth_rxd1_pc5
&eth_tx_en_pg11
&eth_txd1_pg12
&eth_txd0_pg13
>;
&eth_ref_clk_pa1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to detect this situation and enforce that the first entry be included with the opening <, or is there some other place where this formatting kind is really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are useless AFAICS: devicetree_generated.h is unchanged with or without them.
Not sure the linter can complain about those, however, since there may be other situations where they are actually useful.

The linter so far formats the document if it has no syntax errors and if it has syntax errors it will not format but rather report the syntax issues.

The item you mentioned is a diagnostic rule and I can certainly add this to the dts-lsp. FYI I am not an embedded or zephyr developer so more information and explanation on why

zephyr,memory-attr = <(DT_MEM_ARM(ATTR_MPU_RAM))>;

is not needed and how you reached this conclusion it will help me a lot.

With respect to the linter also doing diagnostic check, This is in my plan but for now I want the first baby step in once the linter is in we can expand and enable more. Also diagnostic can be a bit slower as it needs to parse not only the DTS files but also all the headers

Would it be possible to detect this situation and enforce that the first entry be included with the opening <, or is there some other place where this formatting kind is really needed?

So if I understand correctly you want the first values to be next to the < and I guess the last to be next to >;?
would you also want to extend this to byte string i.e [ and I guess the last to be next to ];?

With regards to changing the rule I have no issue as long as other Zephyr maintainer all agree this is the way to go!

Copy link
Contributor

@mathieuchopstm mathieuchopstm Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I am not an embedded or zephyr developer so more information and explanation on why

zephyr,memory-attr = <(DT_MEM_ARM(ATTR_MPU_RAM))>;

is not needed and how you reached this conclusion it will help me a lot.

DT_MEM_ARM is defined as:

#define DT_MEM_ARM(x) ((x) << DT_MEM_ARCH_ATTR_SHIFT)

i.e. DT_MEM_ARM(ATTR_MPU_RAM) expands to ((ATTR_MPU_RAM) << DT_MEM_ARCH_ATTR_SHIFT) - the whole expression is already within parenthesis, so the ones around the macro invocation itself are redundant.

Additionally, per the Linux kernel coding style guidelines which Zephyr follows:

macros defining constants using expressions must enclose the expression in parentheses. Beware of similar issues with macros using parameters.

#define CONSTANT 0x4000
#define CONSTEXP (CONSTANT | 3)

So any expression resulting only from a macro expansion ought to already be wrapped in parentheses. (things like AAA | BBB need to be wrapped manually, but that's a given)

(As secondary remark, outside formatting considerations: this pattern should be replaced by zephyr,memory-attr = <DT_MEM_ARM_MPU_RAM>; instead)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to detect this situation and enforce that the first entry be included with the opening <, or is there some other place where this formatting kind is really needed?

So if I understand correctly you want the first values to be next to the < and I guess the last to be next to >;? would you also want to extend this to byte string i.e [ and I guess the last to be next to ];?

With regards to changing the rule I have no issue as long as other Zephyr maintainer all agree this is the way to go!

I'm out of the loop so my question may be silly, but where do the formatting rules used in this PR come from in the first place? I wasn't even aware that DTS files could be formatted by a tool (reading your comment, and vaguely remembering there was a tech talk about DTS-lsp, I'm assuming this is something new and recent, but I may be wrong?)

Copy link
Contributor Author

@kylebonnici kylebonnici Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand because the macro resolved with (...) then we do not need it in the prop = <()>. this can be a new feature in the LPS in the quick actions.

I can already evaluate all the macros But For LSP to do analysts on the outcome of the macro I need to parse the .h files and for the formatting stage this is not being done for performance reasons. Keep in mind that in overlays and dtsi files the macros available at compile/preprocessing time will change on the main board file and the order of the includes so to do this and format a dtsi/overlay file the main board dts files will be a requirement. I am not sure we want to this in the formatting check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I lost track of things a bit, what's the proposal here?

TL;DR linter should treat ANYTHING and ANYTHING(...) as "something already in parentheses after expansion" and warn patterns such as prop = <(MACRO(x))>;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so if it finds anything like <SOMETHING (-10)> or <SOMETHING(-10)> it won't try to change it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do change and do commit separately so we can see the diff

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabiobaltieri @mathieuchopstm see last commit for changes with respect to the above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kylebonnici kylebonnici force-pushed the dtc/formating-boards branch from e58046c to f0d9999 Compare July 13, 2025 17:38
@keith-zephyr
Copy link
Contributor

I created PR #93123 to explicitly make the bracket usage part of the Zephyr devicetree style guide.

&fmc_a2_pf2 &fmc_a3_pf3 &fmc_a4_pf4 &fmc_a5_pf5
&fmc_sdnras_pf11 &fmc_a6_pf12 &fmc_a7_pf13 &fmc_a8_pf14
&fmc_a9_pf15 &fmc_a10_pg0 &fmc_a11_pg1 &fmc_a12_pg2
&fmc_a14_pg4 /* FMC_BA0 */ &fmc_a15_pg5 /* FMC_BA1 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the comments could be moved on top of the property to solve the issue here:

/* fmc_a14_pg4 is FMC_BA0 and fmc_a15_pg5 is FMC_BA1 */

@mathieuchopstm
Copy link
Contributor

mathieuchopstm commented Jul 22, 2025

I would like to dismiss my -1 since current proposal is OK for me, but GitHub is very helpfully hiding where I would usually do it because of merge conflicts...

If someone knows another way, please let me know 🙂 - in any case, feel free to dismiss it later if I forget to.

@kartben kartben dismissed mathieuchopstm’s stale review July 22, 2025 10:05

Dismissed as per reviewers's request

@kylebonnici
Copy link
Contributor Author

kylebonnici commented Jul 22, 2025

After looking at other tools formatting after the /* and after the * and before */ is not normal done as this will hinder comments like the below

image

I will revert the changes and remove the NIT pick request to

- /* Not a GPIO*/
+ /* Not a GPIO */

FYI @mathieuchopstm

@kylebonnici kylebonnici force-pushed the dtc/formating-boards branch from f6c062b to 61cb0d3 Compare July 22, 2025 18:57
Comment on lines 12 to 13
pinmux =
<LPUART0_RX_PTA1>,
<LPUART0_TX_PTA2>;
<LPUART0_RX_PTA1>,
<LPUART0_TX_PTA2>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we allow this wrap after =? It's really rare and gains nothing in terms of indentation; the style guide also says "use a single space on both sides of = in property definitions", which may be interpreted as disallowing this form.
IMO this should be joined with previous line, like the opening brace scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Push commit to address this feedback + rebased on main again

@kylebonnici kylebonnici force-pushed the dtc/formating-boards branch from bb664f4 to 2906c3f Compare July 23, 2025 08:05
Comment on lines 112 to 108
cs-gpios = <&sdp_k1_120_hdr SDP_120_SPI_SEL_A_N (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
cs-gpios = <&sdp_k1_120_hdr SDP_120_SPI_SEL_A_N(GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has the same issue that has been discussed in the other thread, the speace is meaningful, this is a tuple with three fields not a macro with argument

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will check tonight

@fabiobaltieri
Copy link
Member

Lot of good fixes in this, feeling like a good fraction of these or more could be merged and moved off the way right now so we have less delta to work with, tempted to do some cherry-pick and squash here, what do other folks think?

@mathieuchopstm
Copy link
Contributor

Opened #93603 for the STM32WB0 boards that caused a fuss.

@kylebonnici kylebonnici force-pushed the dtc/formating-boards branch from 2906c3f to d86b293 Compare July 23, 2025 18:22
@kylebonnici
Copy link
Contributor Author

kylebonnici commented Jul 23, 2025

@fabiobaltieri I have started over from main and applied the linter. Split commits by vendor + changed formatting to allow {} from { }

Hope this helps

@kylebonnici kylebonnici force-pushed the dtc/formating-boards branch from 0286cda to ac97e81 Compare August 1, 2025 21:41
@kylebonnici
Copy link
Contributor Author

@keith-zephyr @fabiobaltieri @kartben I have rebase one more time and now the changes we discus are all included in the individual commit per vendor i.e.

...<(MACRO)>.. -> ...<MACRO>..

and

Space before each comment instead of tab when not first token on line

I guess @fabiobaltieri we should not stop making changes and stated getting some of these changes In?

@kylebonnici
Copy link
Contributor Author

Is there any action I can take to start getting this in?

Applying dts-linter results for format files in

boards/seeed

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/segger

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/sensry

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/shields

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/sifive

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/silabs

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/sipeed

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/snps

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/sparkfun

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/st

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/tdk

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/technexion

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/telink

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/ti

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/toradex

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/u-blox

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/udoo

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/variscite

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/vngiotlab

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/waveshare

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/we

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/weact

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/wemos

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/witte

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Applying dts-linter results for format files in

boards/wiznet

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Fix files that needed line splitting to be less then 100 with
Fix files that needed to use node referance to force propes to fit in
100 width

Signed-off-by: Kyle Micallef Bonnici <[email protected]>
Copy link

sonarqubecloud bot commented Sep 2, 2025

<15 0 &pioa 14 0>; /* SPI(SCK) EXTx */
/* GND */
/* +3.3V */
gpio-map = <0 0 &piob 2 0>, /* AFE AD0 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments still not correct. Align the 3 boards and disable formatting for these files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will have to be fixed manually.

linter did not touch the comment as we agreed, it fixed issues on same line comment is on which in turn made comment move.

As per my previous comment linter will not touch comments with respect to the previous token unless the comments first token is the first in that line!. Hence it will also not attempt to realign!

To avoid this then linter will not be allowed to touch lines with comments! which will defeat the purpose! of this automated process!

We also need to be reasonable as we are fixing a lot of pat mistakes in formatting, Once formatting is in place users will start off with a formatted file and the alignment they chose for there comments will be preserved.

Please note this is an automated process. Instead of asking each contributor with files with bad formatting to fix manually, the tool is automatically fixing these... if some extra work remains this can IMO be done by each vendor after formation is in place!

Copy link
Contributor Author

@kylebonnici kylebonnici Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments still not correct. Align the 3 boards and disable formatting for these files.

@nandojve please provide me lists (in addition to boards/atmel/sam/sam4e_xpro/sam4e_xpro.dts)
of file you want me to skip formatting on!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Boards area: Shields Shields (add-on boards)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants