[RFC]: moving to eliminate dts_fixup files

[pabigot]

DTS fixup files exist, as far as I can reconstruct, to bridge between names formerly used to specify device hardware resource assignments in Kconfig with names that are generated when the resource relationships are defined in device tree source files. They are ugly, verbose, and fragile; in short, a source of errors and maintenance pain.

Work like #11707 gives a hint for how we can get rid of fixup files, but there are impacts in naming that propagate to platform-independent driver code and to application code.

The purpose of this issue is to test the Zephyr community's tolerance for the consequences of this proposal.

As an example let's use samples/sensor/ccs811. The sensor exists on the nrf52_pca20020, and driver code was converted to using DT_ prefix instead of CONFIG_ prefix in #11180. As a result, the existing driver requires the following fixups:

• DT_CCS811_NAME used in DEVICE_AND_API_INIT (and preferably instead of "CCS811" in the application call to device_get_binding());
• DT_CCS811_I2C_MASTER_DEV_NAME used in the driver
• DT_CCS811_I2C_ADDR used in the driver

The board DTS file contains this:

&i2c0 {
        ccs811: ccs811@5a {
                compatible = "ams,ccs811";
                reg = <0x5a>;
                label = "CCS811";
        };
};

In the absence of any dts_fixup.h entries we end up with the following in generated_dts_board.h:

// block 1
#define DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_BASE_ADDRESS 0x5a
#define DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_BUS_NAME     "I2C_0"
#define DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_LABEL        "CCS811"
// block 2
#define DT_AMS_CCS811_I2C_0_AMS_CCS811_5A_BASE_ADDRESS DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_BASE_ADDRESS
#define DT_AMS_CCS811_I2C_0_AMS_CCS811_5A_BUS_NAME     DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_BUS_NAME
#define DT_AMS_CCS811_I2C_0_AMS_CCS811_5A_LABEL        DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_LABEL
#define DT_AMS_CCS811_I2C_0_AMS_CCS811_5A_SIZE         DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_SIZE
// block 3
#define I2C_0_AMS_CCS811_5A_BASE_ADDRESS               DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_BASE_ADDRESS
#define I2C_0_AMS_CCS811_5A_BUS_NAME                   DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_BUS_NAME
#define I2C_0_AMS_CCS811_5A_LABEL                      DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_LABEL
#define I2C_0_AMS_CCS811_5A_SIZE                       DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_SIZE

Block 1 comes from the device tree directly, block 2 is synthesized by extract_dts_includes using a new
alias name incorporating the compatible property, and block 3 comes from the defaulted --old-alias-names. (I think the _SIZE generations are a bug in the extractor as there is no such symbol.)

None of these can be used directly in the sensor driver because the board-specific I2C bus and address are part of the identifier.

We can add the following alias to the DTS file:

/ {
        aliases {
                ccs811 = &ccs811;
        };
};

As a result we automatically get these added to generated_dts_board.h (excluding the _SIZE aliases):

// block 1
#define DT_AMS_CCS811_CCS811_BASE_ADDRESS DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_BASE_ADDRESS
#define DT_AMS_CCS811_CCS811_BUS_NAME     DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_BUS_NAME
#define DT_AMS_CCS811_CCS811_LABEL        DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_LABEL
// block 2
#define CCS811_BASE_ADDRESS               DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_BASE_ADDRESS
#define CCS811_BUS_NAME                   DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_BUS_NAME
#define CCS811_LABEL                      DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_LABEL

where extract_dts_includes has produced both old-style and new-style names.

IMO --old-alias-names should be deprecated because the symbols aren't prefixed with DT_, but the new aliases are decoupled from any board-specific wiring and are suitable for use in cross-platform drivers.

So if we're willing to make global name changes like the following:

• Replace DT_CCS811_NAME with DT_AMS_CCS811_CCS811_LABEL
• Replace DT_CCS811_I2C_MASTER_DEV_NAME with DT_AMS_CCS811_CCS811_BUS_NAME
• Replace DT_CCS811_I2C_ADDR with DT_AMS_CCS811_CCS811_BASE_ADDRESS

and more generally accept the names synthesized from the device tree as canonical instead of whatever was used in the past, I think we can entirely get rid of dts_fixup.h files. But take into account what is visible in application code: e.g. the parameter to device_get_binding will change for samples that didn't hard-code the label. Within this development cycle it's already changed (CONFIG_FXOS8700_NAME to DT_FXOS8700_NAME) so a second permanent change (... to DT_NXP_FXOS8700_FXOS8700_LABEL) doesn't seem too harsh.

We could also consider this comment which would change the generated symbols for aliases to:

#define DT_CCS811_BASE_ADDRESS DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_BASE_ADDRESS
#define DT_CCS811_BUS_NAME     DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_BUS_NAME
#define DT_CCS811_LABEL        DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_LABEL

This is closer to the pre-device-tree CONFIG_foo naming scheme, and is probably safe as it'd be the responsibility of the device tree overlay that defines the alias to make sure that it identifies a node with the expected compatibility. It would eliminate the oddity of names like DT_AMS_CCS811_CCS811_LABEL with its duplicated CCS811 component.

[diegosueiro]

@pabigot,

One question came up in my mind.
Why do we even need to explicitly add an alias to generate a more "friendly" #define?
Maybe this could be generated automatically.

In my understanding, we should be using the alias to create meaningful names. For the FXOS8700 we could have an alias like accel = &fxos8700;, for example.

[pabigot]

@diegosueiro I think you're proposing adding or substituting more generic aliases, instead of only an alias that conforms to the device name. I don't have an objection to the adding, though if we do it should probably be from a specific set of standard aliases: e.g. always use accelerometer not sometimes accel.

Consider this:

/ {
    accelerometer = &fxos8700;
    fxos8700 = &fxos8700;
}

Here we specify the device by function, but also by the model field of the corresponding compatible property. You'd use the first in application code that uses the generic sensor API, but the second if your code used features present in the fxos8700 but not all other accelerometer drivers. The driver would use the second.

I'm motivated to the specific name because the existing sensor API is too generic for complete functionality. Devices may or may not support certain types of trigger. The CCS811 indoor air quality sensor simply cannot be used correctly without additional API that can't be supported by the existing generic sensor API. Once some more basic questions are resolved I plan to issue proposals to address that.

Or a device might be a magnetometer and/or gyroscope as well. I don't think you'd want generic aliases that all refer to the same device because the operations used might conflict if all specified on the same device.

I think most non-trivial applications are written for cases where the developer knows the underlying hardware and intends to take advantage of it. So providing a way to clearly indicate the intended target hardware by using its name in the source is a good thing.

If your point is that the extraction tools should automatically be generating an "alias" based on the model component of a compatible property, that's an approach but it's starting to get complex. I expect to see what's available by looking at device tree source files. If magic in preprocessors is changing or adding stuff, it's going to be very difficult to figure out what's going on.

[diegosueiro]

@pabigot,

If we always want to have #defines like #defines DT_AMS_CCS811_LABEL DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_LABEL, I think that this should be performed automatically, without needing to add an alias in the device tree.

Then the alias could be used by the user if he/she wants to generate a more meaningful/generic names.

[sdrsdr]

Aliases are super useful if you have two sensors with the same driver but on different buses or addresses. Also aliases isolates you somewhat from schematics changes (from bus to bus) all you have to do is adjust the device tree and your C/C++ code remains the same. Auto generated aliases will just shorting some defines but might get you in trouble if you're having same chips on different buses (it is not common but is possible)

[erwango]

Fixup files are temporary solution waiting to be able to generate device driver code from device tree.

Requirements for this are here:

• Requirements for driver devices generation using device tree #10904

Prototypes solutions:

• [RFC] Template prototype using jinja2 templating engine #10957
• Generated driver instantiation prototype  #8561

Hopefully, once available, we'll get rid of (most of) the fixups.

[pabigot]

10904 outlines design goals, not requirements (how would you verify "Code generation provided should be easy to maintain"?) and half the text describes a proposed solution using template-based code generation.

I was sorta hoping for something to explain why we aren't just walking a device tree blob at startup (using existing practice from Linux), or generating an immutable data structure with pre-populated driver configuration objects at compile time and walking that at startup (more suitable for embedded applications with fixed resources). Or maybe that's fundamentally what all that stuff with databases or jinja code generation accomplishes. I haven't read it closely but going in I'm scared.

So we have two competing proposals for a solution? How is the selection to be made? And when?

What I propose doesn't cover multiple instances of the same device on different busses/addresses, but then neither does what we have now (at least for any sensor I've seen). For #11545 I'm currently looking at something like this in a cross-platform application dts_fixup.h:

#if defined(CONFIG_HAS_DTS_I2C)

#ifndef DT_SHT3XD_NAME

#ifdef DT_NORDIC_NRF_I2C_40003000_SENSIRION_SHT3XD_44_LABEL

#define DT_SHT3XD_NAME			DT_NORDIC_NRF_I2C_40003000_SENSIRION_SHT3XD_44_LABEL
#define DT_SHT3XD_I2C_ADDR		DT_NORDIC_NRF_I2C_40003000_SENSIRION_SHT3XD_44_BASE_ADDRESS
#define DT_SHT3XD_I2C_MASTER_DEV_NAME	DT_NORDIC_NRF_I2C_40003000_SENSIRION_SHT3XD_44_BUS_NAME

#ifdef DT_NORDIC_NRF_I2C_40003000_SENSIRION_SHT3XD_44_ALERT_GPIOS_CONTROLLER
#define DT_SHT3XD_GPIO_ALERT_DEV_NAME	DT_NORDIC_NRF_I2C_40003000_SENSIRION_SHT3XD_44_ALERT_GPIOS_CONTROLLER
#define DT_SHT3XD_GPIO_ALERT_PIN	DT_NORDIC_NRF_I2C_40003000_SENSIRION_SHT3XD_44_ALERT_GPIOS_PIN
#endif /* GPIO_ALERT */

#else /* board selection */
#error No device tree bindings for SHT3XD
#endif /* board selection */

#endif /* DT_SHT3XD_NAME */

#endif /* CONFIG_HAS_DTS_I2C */

and I'd really like to do something better.

[erwango]

I was sorta hoping for something to explain why we aren't just walking a device tree blob at startup (using existing practice from Linux)

Well, we want to keep footprint as small a possible. And the only blob might just fill the memory space for some of the devices supported. Besides, the parsing code would occupy memory as well. This is why we're looking for a solution that would do the parsing before build time.

[pabigot]

I was sorta hoping for something to explain why we aren't just walking a device tree blob at startup (using existing practice from Linux)

Well, we want to keep footprint as small a possible. And the only blob might just fill the memory space for some of the devices supported. Besides, the parsing code would occupy memory as well. This is why we're looking for a solution that would do the parsing before build time.

Thus the next part you didn't quote:

or generating an immutable data structure with pre-populated driver configuration objects at compile time

which may or may not be what those proposed solutions are doing.

More importantly it seems there's disagreement on whether aliases are the right solution. Alternatives of generating DT_foo symbols based on node label properties as suggested by @diegosueiro are a possible approach that would require small enhancements to existing tooling and would produce identifiers consistent with the actual device name.

I'd be willing to do a PR to add that feature if there are members who would be willing to approve it in principle.

[carlescufi]

@pabigot I think this requires additional discussion based on your proposal and what we have been considering so far. We should wait for @galak to be able to take a look at this first. @erwango has already commented on the current proposal to get rid of the fixup files.

[pabigot]

Agreed, input from @galak is important. My own perspective is still evolving as I get deeper; I'm now less a fan of aliases and also of label. A dedicated property, possibly even marked as specific to zephyr (zephyr,binding-name = "UART0") seems a better approach.

I also believe that the overall solution should include an option to place the device tree blob in the application so it can process the information it needs without having to muck about with tooling that may be ignorant of certain properties.