drivers: sensor: Add RRH46410 driver #80139
Conversation
TotallyOriginalUsername
force-pushed
the
rrh46410-driver
branch
2 times, most recently
from
0bd9869 to
b05372a
Compare
TotallyOriginalUsername
force-pushed
the
rrh46410-driver
branch
from
b05372a to
546b7ce
Compare
zephyrbot
requested review from
avisconti,
jeppenodgaard,
MaureenHelm,
teburd,
tristan-google,
ubieda and
yperess
TotallyOriginalUsername
force-pushed
the
rrh46410-driver
branch
2 times, most recently
from
5faa766 to
60cdb90
Compare
|
Hello @MaureenHelm, could you please review this driver? |
TotallyOriginalUsername
force-pushed
the
rrh46410-driver
branch
from
60cdb90 to
14dd5a4
Compare
TotallyOriginalUsername
force-pushed
the
rrh46410-driver
branch
from
14dd5a4 to
1938054
Compare
|
@TotallyOriginalUsername could you address the compliance check failures? This will also need to be rebased on main |
TotallyOriginalUsername
force-pushed
the
rrh46410-driver
branch
5 times, most recently
from
c8ac9a6 to
dafa2f2
Compare
The compliance check failures have been addressed, and it has been rebased on main |
| k_sleep(K_MSEC(100)); | ||
|
|
||
| err = gpio_pin_set_dt(&cfg->reset_gpio, 0); | ||
| if (err != 0) { | ||
| LOG_ERR("Setting GPIO pin level failed: %d\n", err); | ||
| } | ||
|
|
||
| k_sleep(K_MSEC(600)); |
There was a problem hiding this comment.
Are you sure you want to add 700 ms of boot delay?
There was a problem hiding this comment.
I have added the boot delay there to make sure there is no unexpected behavior from the sensor at startup, which sometimes happens when using I2C.
Would it be better to leave it to the user to manually reset the sensor if needed, or to reduce the delay time to the minimum(500ms)/surround it with an if for I2C?
The other things have been adjusted, thanks
There was a problem hiding this comment.
@MaureenHelm @teburd there are a lot of sensors like this. I'm starting to think that we should have some pattern for a 2+ phase init for sensor drivers. We can kinda fake it right now. We have https://docs.zephyrproject.org/latest/kernel/drivers/index.html#deferred-initialization so we could call device_init() on a SYS_INIT which would kick off the process, then instead of sleeping we could use a timer or a deferred work item to call device_init() again and continue the init process until it finishes. The device would have to maintain an internal state (maybe a place to use the fsm subsystem)?
TotallyOriginalUsername
force-pushed
the
rrh46410-driver
branch
from
dafa2f2 to
59736d0
Compare
| SENSOR_CHAN_IAQ = SENSOR_CHAN_PRIV_START, | ||
| /** 2 bytes Total Volatile Organic Compounds */ | ||
| SENSOR_CHAN_TVOC, | ||
| /** 2 bytes Ethanol Equivalent */ | ||
| SENSOR_CHAN_ETOH, | ||
| /** 2 bytes Estimated CO2 */ | ||
| SENSOR_CHAN_ECO2, | ||
| /** 1 byte Relative IAQ */ | ||
| SENSOR_CHAN_RELIAQ, |
There was a problem hiding this comment.
These custom enums need to have the sensor name in them. e.g., SENSOR_CHAN_RRH46410_IAQ
Adds Renesas RRH46410 gas sensor module driver. Signed-off-by: Mariëlle Korthout <[email protected]>
TotallyOriginalUsername
force-pushed
the
rrh46410-driver
branch
from
59736d0 to
240c7b5
Compare
| rc = rrh46410_read_sample(dev, &data->sample_counter, &data->iaq_sample, | ||
| &data->tvoc_sample, &data->etoh_sample, &data->eco2_sample, &data->reliaq_sample); |
There was a problem hiding this comment.
Please run clang-format on all .c and .h files
| #include <zephyr/drivers/sensor.h> | ||
| #include <zephyr/rtio/rtio.h> | ||
|
|
||
| #if DT_ANY_INST_ON_BUS_STATUS_OKAY(i2c) |
There was a problem hiding this comment.
There's no need to #if here, just include the i2c.h it won't hurt anything and it makes the file more readable.
| #define RRH46410_MAX_RESPONSE_DELAY 150 /* Add margin to the specified 50 in datasheet */ | ||
|
|
||
| union rrh46410_bus_cfg { | ||
| #if DT_ANY_INST_ON_BUS_STATUS_OKAY(i2c) |
There was a problem hiding this comment.
Please include #define DT_DRV_COMPAT in this file otherwise you're depending on include order.
Adds Renesas RRH46410 gas sensor module driver.
The reset pin gets used to make sure the sensor starts from suspend mode.