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: lsm6dsv16x: add i3c support #79346

Merged
merged 3 commits into from
Dec 18, 2024

Conversation

XenuIsWatching
Copy link
Member

@XenuIsWatching XenuIsWatching commented Oct 2, 2024

Add I3C support for the lsm6dsv16x, this also includes streaming with i3c ibi

@avisconti
Copy link
Collaborator

@jonas-rem Hey Jonas, can you possibly take a look to this PR? I think you may be in a good position to help us out here. I added you to the reviewers. Thanks very much!

Copy link
Collaborator

@avisconti avisconti left a comment

Choose a reason for hiding this comment

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

Very good job. Some notes, but I'm very happy that people are contributing here.

drivers/sensor/st/lsm6dsv16x/lsm6dsv16x.h Show resolved Hide resolved
drivers/sensor/st/lsm6dsv16x/lsm6dsv16x_trigger.c Outdated Show resolved Hide resolved
tests/drivers/build_all/sensor/i3c.dtsi Show resolved Hide resolved
Copy link
Collaborator

@avisconti avisconti left a comment

Choose a reason for hiding this comment

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

Looks almost good to me. Just a small change.

lsm6dsv16x_reset_set(ctx, LSM6DSV16X_RESTORE_CTRL_REGS);
do {
lsm6dsv16x_reset_get(ctx, &rst);
} while (rst != LSM6DSV16X_READY);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it would be safer to retry a fixed number of times (with delay) and then FAIL if it never returned LSM6DSV16X_READY.

Copy link
Member Author

Choose a reason for hiding this comment

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

I found there might be an issue with the reset flow, here and i'll have to revisit this section here anyway to "change stuff"

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean in the lsm6dsv16x_reset_set()/lsm6dsv16x_reset_get() APIs themselves? Because in this case I need to debug it as well (I'm the owner of that APIs...). Pls let me know

MaureenHelm
MaureenHelm previously approved these changes Nov 22, 2024
@XenuIsWatching
Copy link
Member Author

XenuIsWatching commented Nov 24, 2024

setting to DNM until #80177 is merged in

@XenuIsWatching XenuIsWatching added the DNM This PR should not be merged (Do Not Merge) label Nov 24, 2024
@XenuIsWatching XenuIsWatching force-pushed the lsm6dsv16x-i3c branch 3 times, most recently from 1b42358 to 0c622ec Compare November 27, 2024 21:45
Copy link
Collaborator

@avisconti avisconti left a comment

Choose a reason for hiding this comment

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

I will review PR later on better. Just noted this simple stuff to be fixed

/* Try for next submission in the transaction */
ctx->txn_curr = rtio_txn_next(ctx->txn_curr);
if (ctx->txn_curr) {
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

So in this case the sqe does not get completed neither with 'ok' nor with 'err'. Is this correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

lets keep comments about the rtio within #80177

A typo was made where if the i3c bus is not null, then it will not set
the ibi callback.

Signed-off-by: Ryan McClelland <ryanmcclelland@meta.com>
Add I3C support to the lsm6dsv16x.

Signed-off-by: Ryan McClelland <ryanmcclelland@meta.com>
Add a build test for the i3c lsm6dsv16x.

Signed-off-by: Ryan McClelland <ryanmcclelland@meta.com>
@XenuIsWatching XenuIsWatching removed DNM This PR should not be merged (Do Not Merge) platform: Nuvoton NPCX Nuvoton NPCX platform: NXP Drivers NXP Semiconductors, drivers labels Dec 16, 2024
@XenuIsWatching
Copy link
Member Author

rebasing now that #80177 is merged

* 8th byte: EMB_FUNC_STATUS
* 9th byte: FSM_STATUS
* 10th byte: MLC_STATUS
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I'm not able to find this anywhere on the public documentation (AN and datasheet). I could ask here internally to the design team, but I think that in zephyr we should make visible only whatever is public. Am I missing something? Can you point me to the proper doc?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I checked with design team. It seems that information is currently only internal, maybe shared with some specific customer. Nevertheless, it seems that we may publish it in the future on the AN (not sure why it is not there already...). I'm still investigating about it...

Copy link
Collaborator

@avisconti avisconti Dec 17, 2024

Choose a reason for hiding this comment

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

The general opinion inside ST is that will be documented in the next lsm6dsv16x DS versions, so I'm fine with leaving this part in.

@kartben kartben merged commit 73af324 into zephyrproject-rtos:main Dec 18, 2024
27 of 28 checks passed
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.

5 participants