Skip to content

Conversation

@mbolivar-nordic
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic commented May 6, 2020

DTS changes for 2.3.

This PR is attempting to close out the remaining issues discussed in the last dev-review, namely being able to access data for all nodes regardless of their status.

This has knock-on effects in the API which cause various renames.

We're going to defer a few more of them until after code freeze to avoid breaking CI any more.

@zephyrbot
Copy link

zephyrbot commented May 6, 2020

All checks are passing now.

checkpatch (informational only, not a failure)

-:898: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#898: FILE: drivers/can/can_stm32.c:24:
+    DT_NODE_HAS_COMPAT_STATUS(DT_NODELABEL(can2), st_stm32_can, okay)$

-:2550: WARNING:TRAILING_SEMICOLON: macros should not use a trailing semicolon
#2550: FILE: drivers/sensor/nxp_kinetis_temp/temp_kinetis.c:186:
+#define TEMP_KINETIS_INIT(inst)						\
+	BUILD_ASSERT(DT_INST_IO_CHANNELS_INPUT_BY_NAME(inst, sensor) <	\
+		     DT_INST_IO_CHANNELS_INPUT_BY_NAME(inst, bandgap),	\
+		     "This driver assumes sensor ADC channel to come before "\
+		     "bandgap ADC channel");				\
+									\
+	static struct temp_kinetis_data temp_kinetis_data_0;		\
+									\
+	static const struct temp_kinetis_config temp_kinetis_config_0 = {\
+		.adc_dev_name =						\
+			DT_INST_IO_CHANNELS_LABEL_BY_IDX(inst, 0),	\
+		.sensor_adc_ch =					\
+			DT_INST_IO_CHANNELS_INPUT_BY_NAME(inst, sensor),\
+		.bandgap_adc_ch =					\
+			DT_INST_IO_CHANNELS_INPUT_BY_NAME(inst, bandgap),\
+		.bandgap_mv = DT_INST_PROP(0, bandgap_voltage) / 1000,	\
+		.vtemp25_mv = DT_INST_PROP(0, vtemp25) / 1000,		\
+		.slope_cold_uv = DT_INST_PROP(0, sensor_slope_cold),	\
+		.slope_hot_uv = DT_INST_PROP(0, sensor_slope_hot),	\
+		.adc_seq = {						\
+			.options = NULL,				\
+			.channels =					\
+		BIT(DT_INST_IO_CHANNELS_INPUT_BY_NAME(inst, sensor)) |	\
+		BIT(DT_INST_IO_CHANNELS_INPUT_BY_NAME(inst, bandgap)),	\
+			.buffer = &temp_kinetis_data_0.buffer,		\
+			.buffer_size = sizeof(temp_kinetis_data_0.buffer),\
+			.resolution = CONFIG_TEMP_KINETIS_RESOLUTION,	\
+			.oversampling = CONFIG_TEMP_KINETIS_OVERSAMPLING,\
+			.calibrate = false,				\
+		},							\
+	};								\
+									\
+	DEVICE_AND_API_INIT(temp_kinetis, DT_INST_LABEL(inst),		\
+			    temp_kinetis_init, &temp_kinetis_data_0,	\
+			    &temp_kinetis_config_0, POST_KERNEL,	\
+			    CONFIG_SENSOR_INIT_PRIORITY,		\
+			    &temp_kinetis_driver_api);

-:2957: WARNING:LONG_LINE: line over 80 characters
#2957: FILE: drivers/usb/device/usb_dc_stm32.c:60:
+#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otgfs) && DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs)

-:2992: WARNING:LONG_LINE: line over 80 characters
#2992: FILE: drivers/usb/device/usb_dc_stm32.c:138:
+#elif DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otgfs) || DT_HAS_COMPAT_STATUS_OKAY(st_stm32_usb)

-:3012: WARNING:LONG_LINE: line over 80 characters
#3012: FILE: drivers/usb/device/usb_dc_stm32.c:313:
+#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32_usbphyc) && DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs)

-:3021: WARNING:LONG_LINE: line over 80 characters
#3021: FILE: drivers/usb/device/usb_dc_stm32.c:324:
+#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32_usbphyc) && DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs)

-:3038: WARNING:LONG_LINE: line over 80 characters
#3038: FILE: drivers/usb/device/usb_dc_stm32.c:360:
+#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32_usbphyc) && DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs)

-:3187: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#3187: FILE: include/devicetree.h:1013:
+#define DT_NODE_HAS_COMPAT_STATUS(node_id, compat, status) \
+	DT_NODE_HAS_COMPAT(node_id, compat) && DT_NODE_HAS_STATUS(node_id, status)

-:3188: WARNING:LONG_LINE: line over 80 characters
#3188: FILE: include/devicetree.h:1014:
+	DT_NODE_HAS_COMPAT(node_id, compat) && DT_NODE_HAS_STATUS(node_id, status)

-:3680: WARNING:LONG_LINE: line over 80 characters
#3680: FILE: tests/lib/devicetree/src/main.c:215:
+	zassert_equal(DT_NODE_HAS_STATUS(DT_NODELABEL(test_no_status), disabled),

-:3705: WARNING:LONG_LINE_STRING: line over 80 characters
#3705: FILE: tests/lib/devicetree/src/main.c:299:
+	zassert_equal(DT_ANY_INST_ON_BUS_STATUS_OKAY(spi), 1, "no spi is on spi");

-:3706: WARNING:LONG_LINE_STRING: line over 80 characters
#3706: FILE: tests/lib/devicetree/src/main.c:300:
+	zassert_equal(DT_ANY_INST_ON_BUS_STATUS_OKAY(i2c), 0, "a spi is on i2c");

-:3723: WARNING:LONG_LINE_STRING: line over 80 characters
#3723: FILE: tests/lib/devicetree/src/main.c:315:
+	zassert_equal(DT_ANY_INST_ON_BUS_STATUS_OKAY(i2c), 1, "no i2c is on i2c");

-:3724: WARNING:LONG_LINE_STRING: line over 80 characters
#3724: FILE: tests/lib/devicetree/src/main.c:316:
+	zassert_equal(DT_ANY_INST_ON_BUS_STATUS_OKAY(spi), 0, "an i2c is on spi");

-:3878: WARNING:SINGLE_STATEMENT_DO_WHILE_MACRO: Single statement macros should not use a do {} while (0) loop
#3878: FILE: tests/lib/devicetree/src/main.c:1268:
+#define INC(inst_ignored) do { val++; } while (0);

-:3878: WARNING:DO_WHILE_MACRO_WITH_TRAILING_SEMICOLON: do {} while (0) macros should not be semicolon terminated
#3878: FILE: tests/lib/devicetree/src/main.c:1268:
+#define INC(inst_ignored) do { val++; } while (0);

-:3884: WARNING:LONG_LINE_COMMENT: line over 80 characters
#3884: FILE: tests/lib/devicetree/src/main.c:1273:
+	 * Make sure DT_INST_FOREACH_STATUS_OKAY works with 0 instances, and does

- total: 0 errors, 17 warnings, 2947 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPDX_LICENSE_TAG SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@mbolivar-nordic mbolivar-nordic force-pushed the DTS-API branch 2 times, most recently from 6d960d4 to 9a4dc52 Compare May 6, 2020 14:13
@zephyrbot zephyrbot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test area: Build System labels May 6, 2020
Copy link
Contributor

@galak galak left a comment

Choose a reason for hiding this comment

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

Need decide if we want to phase out compat2enabled or tweak its semantics. I think changing the semantics is ok since the only status values we've really dealt with are (no status, status = "okay", status = "ok", and status = "disabled". I'm also good if we want to deprecate this and introduce something new.

@mbolivar-nordic
Copy link
Contributor Author

Need decide if we want to phase out compat2enabled or tweak its semantics. I think changing the semantics is ok since the only status values we've really dealt with are (no status, status = "okay", status = "ok", and status = "disabled". I'm also good if we want to deprecate this and introduce something new.

We agreed in dev-review that whatever we do, we can't break gen_legacy_defines.py. It uses compat2enabled, so we can't change its semantics without also making changes to gen_legacy_defines.py, which seems a bit reckless this late in the release cycle.

I'm adding new functionality specifically to be able to leave the legacy scripts alone.

@zephyrbot zephyrbot added area: PWM Pulse Width Modulation area: Watchdog Watchdog area: Sensors Sensors area: ADC Analog-to-Digital Converter (ADC) area: Counter area: I2C area: SPI SPI bus labels May 6, 2020
@galak
Copy link
Contributor

galak commented May 6, 2020

Need decide if we want to phase out compat2enabled or tweak its semantics. I think changing the semantics is ok since the only status values we've really dealt with are (no status, status = "okay", status = "ok", and status = "disabled". I'm also good if we want to deprecate this and introduce something new.

We agreed in dev-review that whatever we do, we can't break gen_legacy_defines.py. It uses compat2enabled, so we can't change its semantics without also making changes to gen_legacy_defines.py, which seems a bit reckless this late in the release cycle.

I'm adding new functionality specifically to be able to leave the legacy scripts alone.

We should probably add a comment in compat2enabled and node.enabled about these only existing for gen_legacy_defines.py. Do we want to call this compat2avail to match the linux API?

@mbolivar-nordic mbolivar-nordic force-pushed the DTS-API branch 3 times, most recently from 86df21d to 257d1d3 Compare May 6, 2020 18:46
@mbolivar-nordic mbolivar-nordic marked this pull request as ready for review May 6, 2020 18:46
@ioannisg ioannisg requested review from erwango and removed request for MaureenHelm and Sizurka May 8, 2020 08:06
@erwango erwango dismissed their stale review May 8, 2020 14:18

Reported CAN issue solved

Copy link
Contributor

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

Haven't looked closely at the documentation yet but figure that can be tuned if necessary.

@mbolivar-nordic
Copy link
Contributor Author

Haven't looked closely at the documentation yet but figure that can be tuned if necessary.

Yep, I promise to go over the documentation with a fine tooth comb before the release and correct any errors I find during the stabilization/documentation period. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

remove "is"

Copy link
Member

Choose a reason for hiding this comment

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

remove second either in the line

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this is correct. There are two optional subordinate clauses, one of which requires its own either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or rather, it's correct with two eithers, just the first, just the second, or neither. IMO. English is pretty flexible.

Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

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

Looks ok to me
Pls, address some minor doc-related comments.

galak and others added 7 commits May 8, 2020 15:19
Move to FOREACH to prepare for an upcoming patch.

Signed-off-by: Kumar Gala <[email protected]>
Even though it is about to be done for sound technical reasons, a
subsequent patch adding access to all device nodes at the last minute
in the 2.3 release is going to be playing a bit of a fast one on
the Zephyr community, especially users of DT_INST APIs.

In particular, instance numbers are currently allocated only to
enabled nodes, but that will not be true soon: *every* node of a
compatible will be allocated an instance number, even disabled ones.

This is especially unfortunate for drivers and applications that
expect singletons of their compatibles, and use DT_INST(0, ...) to
mean "the one enabled instance of my compatible".

To avoid gratuitous breakage, let's prepare for that by sorting each
edt.compat2nodes sub-list so that enabled instances always come before
disabled ones.

This doesn't break any API guarantees, because there basically *are*
no ordering guarantees, in part precisely to give us the flexibility
to do things like this. And it does help patterns that use instances 0
through N-1, including the important singleton case.

Signed-off-by: Martí Bolívar <[email protected]>
Usually, we want to operate only on "available" device
nodes ("available" means "status is okay and a matching binding is
found"), but that's not true in all cases.

Sometimes we want to operate on special nodes without matching
bindings, such as those describing memory.

To handle the distinction, change various additional devicetree APIs
making it clear that they operate only on available device nodes,
adjusting gen_defines and devicetree.h implementation details
accordingly:

- emit macros for all existing nodes in gen_defines.py, regardless
  of status or matching binding
- rename DT_NUM_INST to DT_NUM_INST_STATUS_OKAY
- rename DT_NODE_HAS_COMPAT to DT_NODE_HAS_COMPAT_STATUS_OKAY
- rename DT_INST_FOREACH to DT_INST_FOREACH_STATUS_OKAY
- rename DT_ANY_INST_ON_BUS to DT_ANY_INST_ON_BUS_STATUS_OKAY
- rewrite DT_HAS_NODE_STATUS_OKAY in terms of a new DT_NODE_HAS_STATUS
- resurrect DT_HAS_NODE in the form of DT_NODE_EXISTS
- remove DT_COMPAT_ON_BUS as a public API
- use the new default_prop_types edtlib parameter

Signed-off-by: Martí Bolívar <[email protected]>
These are redundantly checking a node's status twice.

Signed-off-by: Martí Bolívar <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
Add some tests for DT_NODE_HAS_STATUS

Signed-off-by: Kumar Gala <[email protected]>
Add more HOWTO information for the two current devicetree-based device
instantiation styles, and a bit more information on how to create
devices that depend on others.

Point to this from the Kconfig tips page, since it is meant as a
replacement for existing Kconfig practice.

Update macros.bnf.

Signed-off-by: Martí Bolívar <[email protected]>
Swap this out and make the status a parameter.
Leave a couple of cases of DT_NODE_HAS_COMPAT().

Signed-off-by: Kumar Gala <[email protected]>
Signed-off-by: Martí Bolívar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ADC Analog-to-Digital Converter (ADC) area: API Changes to public APIs area: Build System area: Counter area: Devicetree Tooling PR modifies or adds a Device Tree tooling area: Devicetree area: I2C area: PWM Pulse Width Modulation area: Sensors Sensors area: SPI SPI bus area: Tests Issues related to a particular existing or missing test area: Watchdog Watchdog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants