Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: sensor: add ScioSense ENS160 driver #67343

Merged
merged 1 commit into from Feb 26, 2024

Conversation

ggrs
Copy link
Contributor

@ggrs ggrs commented Jan 8, 2024

Add driver for ScioSense ENS160 multi-gas sensor. The driver includes support for I2C and SPI, attributes for setting temperature and humidity compensation and data ready trigger. Also add ScioSense to the list of vendor prefixes.

@zephyrbot zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: Sensors Sensors labels Jan 8, 2024
@ggrs ggrs force-pushed the ens160_driver branch 2 times, most recently from 96c0724 to b70cb5c Compare January 10, 2024 00:33
MaureenHelm
MaureenHelm previously approved these changes Jan 10, 2024
Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@MaureenHelm
Copy link
Member

Needs to be rebased.

@teburd @yperess please take a look

@ggrs
Copy link
Contributor Author

ggrs commented Jan 22, 2024

@MaureenHelm rebase done

MaureenHelm
MaureenHelm previously approved these changes Jan 23, 2024
teburd
teburd previously approved these changes Feb 6, 2024
@ggrs ggrs requested a review from MaureenHelm February 6, 2024 21:51
Copy link
Contributor

@aasinclair aasinclair left a comment

Choose a reason for hiding this comment

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

Looks really good, just a couple of comments

return 0;
}

static int ens160_new_data(const struct device *dev)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static int ens160_new_data(const struct device *dev)
static bool ens160_new_data(const struct device *dev)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 87 to 91
if (FIELD_GET(ENS160_STATUS_NEWDAT, status) == 0x01) {
return 1;
}

return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (FIELD_GET(ENS160_STATUS_NEWDAT, status) == 0x01) {
return 1;
}
return 0;
return FIELD_GET(ENS160_STATUS_NEWDAT, status) != 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

chan == (enum sensor_channel)SENSOR_CHAN_ENS160_AQI);

if (!IS_ENABLED(CONFIG_ENS160_TRIGGER)) {
WAIT_FOR((ens160_new_data(dev) == 1), ENS160_TIMEOUT_US, k_msleep(10));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
WAIT_FOR((ens160_new_data(dev) == 1), ENS160_TIMEOUT_US, k_msleep(10));
WAIT_FOR(ens160_new_data(dev), ENS160_TIMEOUT_US, k_msleep(10));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

{
const struct ens160_config *config = dev->config;

return i2c_burst_write_dt(&config->i2c, reg, data, len);
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately i2c_burst_write is problematic, so should be avoided (#20154).

You will need to create a combined write buffer here, something like:

Suggested change
return i2c_burst_write_dt(&config->i2c, reg, data, len);
__ASSERT(len == 2, "Only 2 byte write are supported");
uint8_t buff[] = {reg, data[0], data[1]};
return i2c_write_dt(&config->i2c, buff, sizeof(buff));
}

Alternatively you may wish to have a write 2 registers function instead.
If it is unlikely that there will be need to writes more than 2 bytes, this is a good option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not aware of this problem. Thank you for pointing it out.

The datasheet (version 1.3) mentions in section 16.2.4 a write operation to a set of consecutive registers for configuring threshold interrupt, but some details are absent in the current revision. Therefore, this driver does not implement threshold triggers for the same reason.

However, I imagine that in a future revision the datasheet may become clearer and there will be the need for writing more than two bytes.

Because of this, I opted for your first suggestion.

Add driver for ScioSense ENS160 multi-gas sensor. The driver includes
support for I2C and SPI, attributes for setting temperature and
humidity compensation and data ready trigger.
Also add ScioSense to the list of vendor prefixes.

Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
@ggrs ggrs requested a review from aasinclair February 8, 2024 21:45
@fabiobaltieri fabiobaltieri added this to the v3.7.0 milestone Feb 9, 2024
@nashif nashif merged commit 3850f4c into zephyrproject-rtos:main Feb 26, 2024
18 checks passed
@ggrs ggrs deleted the ens160_driver branch February 26, 2024 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: Sensors Sensors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants