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:ak09918c use RTIO #82665

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

FlorianWeber1018
Copy link
Contributor

make usage of the RTIO in the ak09918c driver.

@FlorianWeber1018 FlorianWeber1018 force-pushed the ak09918_using_rtio branch 11 times, most recently from 63e76e6 to da1ce65 Compare December 13, 2024 10:06
@FlorianWeber1018 FlorianWeber1018 force-pushed the ak09918_using_rtio branch 2 times, most recently from 6b5345a to e7bac6b Compare December 14, 2024 10:29
@FlorianWeber1018
Copy link
Contributor Author

CI is failing because of #82992

teburd
teburd previously approved these changes Jan 8, 2025
return sqe;
}

struct rtio_sqe *i2c_rtio_copy_reg_burst_read(struct rtio *r, struct rtio_iodev *iodev,
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are nice little helpers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you like those It would be consequent to add also a i2c_rtio_copy_reg_read_byte helper as well, but this is not needed by this driver. If required I can make a separate PR for those helper functions and and add that as well.

LOG_ERR("Failed to start measurement.");
return -EIO;
AKM09918C_CNTL2_SINGLE_MEASURE) == 0) {
return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why inverse the return codes here? The typical early return path in Zephyr functions is the failure case unless there happens to be a shortcut for a success. Does the CNTL1_PWR_DOWN mode not being set constitute an IO error really?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Short answer: you are right - I have reverted the change.

I thought that the device is in power down mode could be the only valid mode when trying to trigger a single shot, but thats not correct at all.
From my understanding the device is in continues measurement mode (by setting the SENSOR_ATTR_SAMPLING_FREQUENCY to a valid value) the start_measurement was also called by sample sample_fetch and should just don´t do anything and return 0. That´s not that elegant but not related to this PR.

@teburd teburd dismissed their stale review January 8, 2025 17:56

Mean to add comments not approve just yet, oops!


#endif /* CONFIG_I2C_RTIO */

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation looks odd here

*
* @return a value from i2c_transfer()
*/
static inline int i2c_transfer_dt(const struct i2c_dt_spec *spec,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't be indented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@teburd
Copy link
Collaborator

teburd commented Jan 9, 2025

@MaureenHelm seems like some issue building with adxl345 in CI with this change, which doesn't make much sense, can you take a look? https://github.com/zephyrproject-rtos/zephyr/actions/runs/12695436379/job/35387323768?pr=82665#step:11:2327

extend i2c library to create rtio sqes for reading and writing bytes

Signed-off-by: Florian Weber <[email protected]>
make usage of the RTIO in the ak09918c driver

Signed-off-by: Florian Weber <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants