devicetree: format files in boards

[kylebonnici]

Diff for boards folder as per discussion in #92334

[JarmouniA]

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

[mathieuchopstm]

(non-blocking remarks up to ST boards)

[mathieuchopstm]

IMO both comments should remain aligned.

[kartben]

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 */
			};
		};
	};

[kylebonnici]

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

[mathieuchopstm]

Two blockers on ST boards + one nit.

[kylebonnici]

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.

[pillo79]

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

[pillo79]

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?

[kylebonnici]

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!

[mathieuchopstm]

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:

zephyr/include/zephyr/dt-bindings/memory-attr/memory-attr-arm.h

Line 24 in 7a8bd3d

#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)

[mathieuchopstm]

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?)

[kylebonnici]

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.

[mathieuchopstm]

yes, all macros should expand to (...) (or a naked value, but it's fine to treat that as (...))

[kylebonnici]

@kartben @keith-zephyr @fabiobaltieri so should LPS always clean out extra () when we have expressions with only 1 macro in it?

hence prop1 = <(MACRO(2,3))> -> prop1 = <MACRO(2,3)>
but prop1 = <(MACRO(2,3) + 4) -> prop1 = <(MACRO(2,3) + 4)

[fabiobaltieri]

sounds like a good idea to me if you can do it, redundant parenthesis makes expressions harder to understand in my opinion, I always ask to remove them on the code and other reviewers do it too

[kylebonnici]

@fabiobaltieri @mathieuchopstm

I have one concern if we have

#DEFINE  SUB(a,b) (a - b)
...
prop=<(SUB(1-10))> // -> resolves to prop=<(-9)>

and -9 has to be in (..) so I think for the formatting stage I cannot do this assumption to remove the ()

[mathieuchopstm]

#DEFINE  SUB(a,b) (a - b)
...
prop=<(SUB(1-10))> // -> resolves to prop=<(-9)>

I assume that's a typo and you meant SUB(1,10), in which case...

No: SUB(1-10) expands to (1 - 10) and thus prop = <((1 - 10))> => no need for parentheses

(+ on paper, DTS does not support negative values)

[keith-zephyr]

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

[mathieuchopstm]

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]

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.

Dismissed as per reviewers's request

[kylebonnici]

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

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

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

FYI @mathieuchopstm

[pillo79]

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.

[kylebonnici]

Push commit to address this feedback + rebased on main again

[fabiobaltieri]

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

[kylebonnici]

Will check tonight

[fabiobaltieri]

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]

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

[kylebonnici]

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

Hope this helps

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud