drivers: i2c: Introduce basic bflb I2C driver #98364
Conversation
VynDragon
force-pushed
the
bflb_i2c
branch
4 times, most recently
from
f6169ed to
95cac46
Compare
zephyrbot
added
area: Boards/SoCs
area: I2C
area: Devicetree Bindings
area: RISCV
zephyrbot
requested review from
edersondisouza,
fkokosinski,
josuah,
katsuster,
kgugala,
masz-nordic,
mgielda,
npitre,
teburd,
tgorochowik and
ycsin
There was a problem hiding this comment.
Interesting to see what's happening with phase0, phase1, phase2, phase3 (timing of individual signal steps of I2C AFAIU).
I would have assumed sys_read32/sys_write32 would have been enough to tell GCC not to shuffle the order of IO register read/write, curious to follow-up on that.
Just proposing reminders about MHZ() but not sure if that's something you even wanted to use (i.e. other I2C drivers don't use them).
VynDragon
force-pushed
the
bflb_i2c
branch
2 times, most recently
from
2b71baa to
37cdc50
Compare
Adds i2c binding and nodes Signed-off-by: Camille BAUD <[email protected]>
|
@teburd PTAL |
teburd
left a comment
teburd
left a comment
There was a problem hiding this comment.
Please use kernel primitives for queueing threads rather than looping on hardware busy status.
| return 0; | ||
| } | ||
|
|
||
| while (i2c_bflb_busy(dev) && !sys_timepoint_expired(end_timeout)) { |
There was a problem hiding this comment.
k_sem/k_mutex here instead of busy looping to provide a thread queuing mechanism with a timeout.
e.g.
k_sem_take(data->lock, K_FOREVER);There was a problem hiding this comment.
This is not intended for checking thread queuing, it's for bad states. there no thread queuing mechanism, i suppose there should be one?
There was a problem hiding this comment.
Yes there should be a semaphore or mutex here on the blocking transfer call to block threads until the hardware is usable.
| } | ||
| i += ret; | ||
| end_timeout = sys_timepoint_calc(K_MSEC(I2C_WAIT_TIMEOUT_MS)); | ||
| while ((i2c_bflb_busy(dev) |
There was a problem hiding this comment.
You have some interrupts it seems to get events from the hardware, tight looping for completions like this is I'd say generally frowned on if you have an interrupt to tell you instead.
Broadly speaking looping over the messages int he thread context, sending, waiting, repeat isn't workable with any other I2C API (submit) which means supporting this device with sensors will involve a fallback mechanism which is less than optimal.
Non-blocking comment but highly recommended.
There was a problem hiding this comment.
Well it does mostly check the interrupts, are you saying it should set a completion flag from the interrupt instead and semaphore wait for it?
There was a problem hiding this comment.
You should set a completion flag (semaphore) one time when the entire transfer of i2c_msg array is done. You should not be looping in i2c_transfer ideally.
Writing your driver this way will enable you to add .submit later much easier (highly recommended).
There was a problem hiding this comment.
Ah i see. I am aware of this issue, this is intended to be basic like this, improvement on this angle would be done when/if submit and other async stuffs are implemented, I have no issue rewriting this bit when this will be necessary.
There was a problem hiding this comment.
This is why its a non-blocking comment, it'd be nice, but its not expected in this PR
Introduce synchronous i2c driver Signed-off-by: Camille BAUD <[email protected]>
Adds I2C tests running Signed-off-by: Camille BAUD <[email protected]>
Fix bad formatting. Signed-off-by: Camille BAUD <[email protected]>
|
5 participants
No DMA, no async.
Tested on bl602 and bl618.