drivers: i2c: add i2c controller implementation using rp2040 pio #81183
Conversation
zephyrbot
added
area: I2C
platform: Raspberry Pi Pico
yiding
force-pushed
the
pico-i2c-base
branch
2 times, most recently
from
d2aa54a to
7ed541b
Compare
|
I have a followup which adds support for using a DMA channel to improve performance, which roughly doubles the performance. I'll put that up as a separate PR. I have also tried implementing this driver using interrupts in two ways, both of which performs worse than this version.
If interested I can dig them up and post them elsewhere. |
yiding
force-pushed
the
pico-i2c-base
branch
from
7ed541b to
c836ee1
Compare
yiding
force-pushed
the
pico-i2c-base
branch
from
c836ee1 to
10e93ae
Compare
|
I'm not at all familiar with the pio peripheral, so I've reassigned. |
|
Fixed most style nits. @soburi Updated with additional fixes for handling "gather" style writes (e.g. as used by i2c_burst_write). @FlorianWeber1018 |
| * @name I2C PIO Program | ||
| * Based on pico example code: | ||
| * https://github.com/raspberrypi/pico-examples/blob/master/pio/i2c/i2c.pio |
There was a problem hiding this comment.
this program is actually BSD-3 licensed -- you can't just take it and implicitly relicense it as Apache 2. This probably needs some kind of TSC approval, or maybe PIO based driver should be in a dedicated module or something?
There was a problem hiding this comment.
Hm. Originally had the original license in this file but the linters didn't like that at all, so I meant to ask what should be done in this situation, but forgot about it. The C code interfacing with the PIO code is also based on the corresponding C code in the example as well, so if we want to be diligent about it (even though most of the C code here is pretty bespoke, it's still based on that example C code) that should also be licensed BSD3.
Is it not possible to have BSD3 code in tree? I thought BSD3 is compatible with Apache2, as long as we include the original license somewhere.
There was a problem hiding this comment.
Since the BSD-3 source code already exists, it seems like there should be no problem as long as the license is stated correctly.
There was a problem hiding this comment.
Could you put just the BSD-3 part into a separate file? (I think it would be fine to just make it a local header file.
The BSD-3 code already exists, so I think it would be best to explain it to TSC and include it as BSD-3.
There was a problem hiding this comment.
I don't think it would appease the license checker any more than this would.
I'll ask in this discord what's the best way forward here is. The docs are not clear.
There was a problem hiding this comment.
Understood. I'll follow their policy.
There was a problem hiding this comment.
"Importing code into the Zephyr OS from other projects that use a license other than the Apache 2.0 license needs to be fully understood in context and approved by the Zephyr governing board.":
https://docs.zephyrproject.org/latest/contribute/guidelines.html#licensing
I2C controller that uses PIO on RP2040. Using this we can drive up to 8 I2C busses in addition to the two supported by the built-in i2c peripheral. This driver is adapted from the pico sdk pio i2c example with some changes to fix identified issues. 1. Resetting the input shift register before reading a byte. Sometimes the shift register might get out of sync. 2. Resetting the SM after handling a NAK, related to shift registers counters getting out of sync. 3. Waiting for possible clock stretching when writing the I2C STOP. This is a blocking driver as I found versions interacting with the PIO via interrupts to perform much worse than simply waiting via `k_yield`. The driver attempts to load the PIO code only once in order to allow multiple driver instances to run the same code. It also uses the `reg` property as the SM, instead of relying on pico sdk to arbitrarily assign one, which seems to be the norm for other PIO-based drivers in the codebase. I'll put up a separate PR to do this on the main pio dts nodes and fix-up drivers if people think this is a good idea. Signed-off-by: Yiding Jia <[email protected]>
yiding
force-pushed
the
pico-i2c-base
branch
from
8083dc9 to
0ff0659
Compare
|
Scancode license checker gives this error, so it seems I can't just trivially declare the license of this file to BSD-3: |
henrikbrixandersen
added
the
Licensing
|
I created an issue #87378 per documentation. This isn't being contributed as an external component, but it's the closest process I've found for this situation. |
|
@yiding is it only the assembly code that you took from the RPi Pico SDK? Or some of the C code as well? If it's only the PIO asm code, then I suggest you move that to https://github.com/zephyrproject-rtos/hal_rpi_pico instead, and then the driver could be just Apache v2 licensed. |
I2C controller that uses PIO on RP2040. Using this we can drive up to 8 I2C busses in addition to the two supported by the built-in i2c peripheral.
This driver is adapted from the pico sdk pio i2c example with some changes to fix issues identified over the years.
This is a blocking driver as I found versions interacting with the PIO via interrupts to perform much worse than simply waiting via
k_yield.The driver attempts to load the PIO code only once in order to allow multiple driver instances to run the same code.
It also uses the
regproperty as the SM, instead of relying on pico sdk to arbitrarily assign one, which seems to be the norm for other PIO-based drivers in the codebase. I'll put up a separate PR to do this on the main pio dts nodes and fix-up drivers if people think this is a good idea.