sensors: Specify sensor channel in DT

[kornelduleba]

Introduction

Configuration parameters in Zephyr are normally set in DT. In particular consumers of a device usually refer to it using phandle-array type, which allows them to specify configuration parameters in addition to the phandle itself.
The goal of this proposal is to enable the specification of a sensor channel through DT.

Problem description

Some temperature sensors expose multiple temperature probes from a single device. The probe to read the temperature from is selected using the sensor channel. Usually driver specific, a.k.a private channels are used for this purpose. See PR with F75303 driver as example - #60833.
In addition to that Zephyr itself defines three different temperature channels:

        /** Device die temperature in degrees Celsius. */
        SENSOR_CHAN_DIE_TEMP,
        /** Ambient temperature in degrees Celsius. */
        SENSOR_CHAN_AMBIENT_TEMP,
        /** Gauge temperature  **/
        SENSOR_CHAN_GAUGE_TEMP,

Application that reads the temperature needs to specify which channel to use. Right now it has to be hardcoded in the source itself. This is not ideal, and definitely not scalable.

Proposed change

This proposal enables the specification of a sensor channel in DT. As mentioned above, it's aimed at consumer applications of temperature sensors, but it might prove to be useful in other types of sensors as well.

Detailed RFC

The change can be split into two parts:

1. Define #sensor-cells property in sensor-device.yaml binding. This way a sensor device node can be referenced using the phandle-array property type.
2. Define temperature related sensor channels in a separate header under dt-bindings directory. In the future additional channels might be added.

Proposed change (Detailed)

The Commit with the proposed change can be found here.

To make sure that the #sensor-cells property is inherited by all sensor devices bindings defined in sensor-device.yaml binding. Additionally it’s marked as optional to retain backward compatibility.

  "#sensor-cells":
    type: int
    required: false
    description: |
      Number of items to expect in a sensor specifier.
      By default it's only one attribute is expected - a sensor channel to use.

sensor-cells:
  - channel

Then each DT node of a sensor that wants to use this has to be modified to contain: #sensor-cells = <1>;

To expose all temperature related channels in DT their values have been redefined in a dedicated header under dt-bindings directory.

/* The values were copied over from drivers/sensor.h */
#define DT_SENSOR_CHAN_DIE_TEMP 12
#define DT_SENSOR_CHAN_AMBIENT_TEMP 13
#define DT_SENSOR_CHAN_GAUGE_TEMP 43

The redefinition is necessary for two reasons:

1. DT parsing logic doesn’t support enums, so extracting enum sensor_channel is not an option.
2. We can’t remove the enum itself and replace it with macros, as that would break the stable API.

In the consumer DT node the following can be added to reference the temperature sensors

sensors = <&temp_cpu DT_SENSOR_CHAN_DIE_TEMP>,
          <&soc_pct2075 DT_SENSOR_CHAN_AMBIENT_TEMP>,
          <&amb_pct2075 DT_SENSOR_CHAN_AMBIENT_TEMP>;

Dependencies

N/A

Concerns and Unresolved Questions

Admittedly duplication of sensor channel value definitions is not among the best coding practices. Any suggestion on how to implement this in a cleaner way is more than appreciated.

Alternatives

1. Remove the enum sensor_channel altogether and define sensor channels using macros. This would cause a stable API breakage, hence I decided not to go this way.

[github-actions]

Hi @semihalf-Duleba-Kornel! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

[fabiobaltieri]

cc @MaureenHelm @avisconti @teburd @yperess (Sensors) @mbolivar-ampere @galak (Devicetree) @gmarull @carlescufi @keith-zephyr

@semihalf-Duleba-Kornel you may want to open a draft or DNM PR for 2810e96, that way people can comment on the diff

[gmarull]

should we align with iio? https://github.com/devicetree-org/dt-schema/tree/main/dtschema/schemas/iio

[teburd]

should we align with iio? https://github.com/devicetree-org/dt-schema/tree/main/dtschema/schemas/iio

I'd say no myself. We don't have IIO, and IIO (I tried) turns out to be a poor fit, IMHO, for Zephyr.

[yperess]

I agree with this idea overall, I did something very similar with my pitch for the sensor subsystem (https://github.com/zephyrproject-rtos/zephyr/pull/56963/files#diff-3144f3f020602f03044b01fa08365e64c890a32f059002db005450463f6ab640R12) where I added the sensor types (very very similar to channels). I think having the channels in the devicetree and adding them to the sensor info struct is a great solution.

[teburd]

I think its generally a great idea, but as noted because some sensors provide multiple channels of the same type (the referenced PR #60833) it would be nicer if this could refer to not just the type but also the index of the channel.

Frankly that problem should be solved before this, or minimally this new binding should always refer to channel index 0 to start until the problem is fixed in the sensor API.

So I'd change this slightly for future proofing to be

phandle channel_type channel_index

[fabiobaltieri]

I think its generally a great idea, but as noted because some sensors provide multiple channels of the same type (the referenced PR #60833) it would be nicer if this could refer to not just the type but also the index of the channel.

That's very interesting, are you aware of any previous proposal for adding an index parameter to the sensor API?

[teburd]

I think its generally a great idea, but as noted because some sensors provide multiple channels of the same type (the referenced PR #60833) it would be nicer if this could refer to not just the type but also the index of the channel.

That's very interesting, are you aware of any previous proposal for adding an index parameter to the sensor API?

Yes, in my two attempts at updating the API as well as the current iteration from @yperess that supports a decoder, where the channel type is returned as each channel is iterated over. Though somewhat implicit in the current decoder API it was gamed out and thought of on some level.

This was one of the many requirements/problems laid out in #1387 and #13718 by @microbuilder

[gmarull]

should we align with iio? https://github.com/devicetree-org/dt-schema/tree/main/dtschema/schemas/iio

I'd say no myself. We don't have IIO, and IIO (I tried) turns out to be a poor fit, IMHO, for Zephyr.

Note that IIO scheme can be used in DT anyway, in the end, this is about describing the hardware.

[teburd]

should we align with iio? https://github.com/devicetree-org/dt-schema/tree/main/dtschema/schemas/iio

I'd say no myself. We don't have IIO, and IIO (I tried) turns out to be a poor fit, IMHO, for Zephyr.

Note that IIO scheme can be used in DT anyway, in the end, this is about describing the hardware.

That's fine but then how would io-channels map to sensor channel types which are completely different things? iio channels are described by a channel spec I believe? It's unclear what a io-channel in this scenario would be.

e.g. is it this? https://docs.kernel.org/driver-api/iio/core.html#c.iio_chan_spec and the io-channel in this case is the mapped channel id?

Zephyr sensors lack this channel descriptor and mapping idea entirely for better or worse, there's no notion that sensor X has channel 0 which represents acceleration in the X axis in SI units, iio seems to have this sort of descriptor available and mapped to a numerical channel.

[yperess]

We use IIO and I agree with Tom, this issue is complex enough without adding another requirement to it. I would vote for us finding a clean way to migrate things forward in a way that fits Zephyr (i.e. backward compatible). I think that listing these "capabilities" in devicetree is great, but some projects have very specific application code and probably won't benefit from this. In a similar way, the sensor info struct is only added behind a Kconfig. That's the pattern we should model here. This is great for generic apps/libraries that want to iterate of expose sensors to another client, but it's not really needed for many other applications.

[kornelduleba]

@semihalf-Duleba-Kornel you may want to open a draft or DNM PR for 2810e96, that way people can comment on the diff

Considering that there is no opposition to the approach I proposed here I'll just finish the code and make a normal PR.
I'll probably have it done EOD, or tomorrow.

I think its generally a great idea, but as noted because some sensors provide multiple channels of the same type (the referenced PR #60833) it would be nicer if this could refer to not just the type but also the index of the channel.

Frankly that problem should be solved before this, or minimally this new binding should always refer to channel index 0 to start until the problem is fixed in the sensor API.

So I'd change this slightly for future proofing to be

phandle channel_type channel_index

I'll update the bindings to be more flexible.
We can move part of the sensor-cells specification to binding of a specific sensor.
This way each device can specify whether it wants to have (channel, index), or just (channel) specified in the DT.

When it comes to adding "index" to the sensor API itself it might be a bit more complicated.
There are two ways to do it that I can think of:

1. Break the driver API and change the definitions of sensor_sample_fetch_t and sensor_channel_get_t to take an argument index. The existing get and fetch syscalls could be replaced with helpers that point to the new API with index set to 0.
2. Create two syscalls - fetch and get - that use the index argument.

Option 1. is much cleaner, but it breaks API. With option 2 we also bloat the API and increase the overall flash size.
Do we care about the breakage? It'll only affect out-of-tree sensor drivers, rather then the consumers/application that use the sensor API.

should we align with iio? https://github.com/devicetree-org/dt-schema/tree/main/dtschema/schemas/iio

I'd say no myself. We don't have IIO, and IIO (I tried) turns out to be a poor fit, IMHO, for Zephyr.

Note that IIO scheme can be used in DT anyway, in the end, this is about describing the hardware.

That's fine but then how would io-channels map to sensor channel types which are completely different things? iio channels are described by a channel spec I believe? It's unclear what a io-channel in this scenario would be.

e.g. is it this? https://docs.kernel.org/driver-api/iio/core.html#c.iio_chan_spec and the io-channel in this case is the mapped channel id?

Zephyr sensors lack this channel descriptor and mapping idea entirely for better or worse, there's no notion that sensor X has channel 0 which represents acceleration in the X axis in SI units, iio seems to have this sort of descriptor available and mapped to a numerical channel.

Apart from that we also might want to specify a pair of (channel, index).
For that reason alone iio-channel can't be used here.

[kornelduleba]

@teburd Take a look at https://github.com/semihalf-Duleba-Kornel/zephyr/commits/sensor-index-dt.
It's based on #61271 and it adds support specifying the channel index in DT.
I made it to show that the #61271 can be easily expanded to support that.

[kornelduleba]

@teburd Could you please take a look at #61271?
Also do you have any thoughts about the approach for the index specification?
See two comments above this one.

[mbolivar-ampere]

cc @mbolivar-ampere @galak (Devicetree)

No objections from me assuming the sensor maintainers agree.

[MaureenHelm]

In the consumer DT node the following can be added to reference the temperature sensors

sensors = <&temp_cpu DT_SENSOR_CHAN_DIE_TEMP>,
         <&soc_pct2075 DT_SENSOR_CHAN_AMBIENT_TEMP>,
         <&amb_pct2075 DT_SENSOR_CHAN_AMBIENT_TEMP>;

What hardware would the consumer DT node represent? It seems like this approach would lead to nodes in the devicetree that have no relation to hardware.

[kornelduleba]

In the consumer DT node the following can be added to reference the temperature sensors

sensors = <&temp_cpu DT_SENSOR_CHAN_DIE_TEMP>,
         <&soc_pct2075 DT_SENSOR_CHAN_AMBIENT_TEMP>,
         <&amb_pct2075 DT_SENSOR_CHAN_AMBIENT_TEMP>;

What hardware would the consumer DT node represent? It seems like this approach would lead to nodes in the devicetree that have no relation to hardware.

Is having a physical piece of hardware that corresponds to a DT node a hard requirement?
From what I can see there are some pseudo-devices already described in the Zephyr bindings e.g. input-longpress.

I'm currently working on implementing a thermal framework, something similar to Linux thermal zones.
Basically the node would describe a configuration for a subsystem that would periodically read temperature from sensor(s) and then perform an action, e.g adjust fan speed, toggle a PROCHOT pin etc.

[fabiobaltieri]

Is having a physical piece of hardware that corresponds to a DT node a hard requirement? From what I can see there are some pseudo-devices already described in the Zephyr bindings e.g. input-longpress.

Yeah I think these sort of soft-devices are the right thing to do in these cases, before the input-longpress there was cdc-acm-uart, now we have the lvgl input nodes.

Besides being a good spot for a structured config and laying down a pattern for the config/data struct and multiple instances in the driver, you also get the phandle based dependency check for the initialization sequence by doing that.

[jfischer-no]

Is having a physical piece of hardware that corresponds to a DT node a hard requirement? From what I can see there are some pseudo-devices already described in the Zephyr bindings e.g. input-longpress.

Yeah I think these sort of soft-devices are the right thing to do in these cases, before the input-longpress there was cdc-acm-uart, now we have the lvgl input nodes.

Virtual-devices, CDC ACM UART is a type of emulated UART device, in the application it can be replaced with a real controller.

Besides being a good spot for a structured config and laying down a pattern for the config/data struct and multiple instances in the driver, you also get the phandle based dependency check for the initialization sequence by doing that.

+1

[mbolivar-ampere]

Is having a physical piece of hardware that corresponds to a DT node a hard requirement?
From what I can see there are some pseudo-devices already described in the Zephyr bindings e.g. input-longpress.

In that case I think this should be zephyr,sensors:

https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html#the-zephyr-prefix

You must add this prefix to property names in the following cases: [...] Configuration values that are specific to a Zephyr device driver.

[...] Though devicetree in general is a hardware description and configuration language, it is Zephyr’s only mechanism for configuring driver behavior for an individual struct device. Therefore, as a compromise, we do allow some software configuration in Zephyr’s devicetree bindings, as long as they use this prefix to show that they are Zephyr specific.

[kornelduleba]

In that case I think this should be zephyr,sensors:
https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html#the-zephyr-prefix

That's pretty much the plan :)
The consumer of this node would use a compatible that looks like "zephyr,foo".
This is outside the scope of this RFC.
Right now what I'm proposing is to extend the sensor/DT APIs so that a sensor and its channel can be directly referenced in DT using the phandle-array type.