Disable I2C interface in ICM42688P sensor when it is initialized #22184
Conversation
| FIFO_COUNT_REC = Bit6, | ||
| FIFO_COUNT_ENDIAN = Bit5, | ||
| SENSOR_DATA_ENDIAN = Bit4, | ||
| UI_SIFS_CFG = Bit1 | Bit0, |
There was a problem hiding this comment.
How about something like this? We need a better register abstraction...
| UI_SIFS_CFG = Bit1 | Bit0, | |
| UI_SIFS_CFG_DISABLE_I2C = Bit1 | Bit0, |
There was a problem hiding this comment.
I also added the definitions to the copy of the file under boards/modalai/voxl2-slpi/src/drivers/icm42688p just in case. I am not sure how it is used, but I guess it doesn't harm at least, as it didn't exist before.
| register_bank0_config_t _register_bank0_cfg[size_register_bank0_cfg] { | ||
| // Register | Set bits, Clear bits | ||
| { Register::BANK_0::INTF_CONFIG0, INTF_CONFIG0_BIT::UI_SIFS_CFG, 0}, // 0x3 disable I2C |
There was a problem hiding this comment.
Can you put this just before INTF_CONFIG1 (0x4D) to keep it in order?
There was a problem hiding this comment.
Yes, I intended it to be as early as possible, since it protects the bus. But it doesn't make much difference, it looks better like you suggested. Done
| register_bank0_config_t _register_bank0_cfg[size_register_bank0_cfg] { | ||
| // Register | Set bits, Clear bits | ||
| { Register::BANK_0::INTF_CONFIG0, INTF_CONFIG0_BIT::UI_SIFS_CFG, 0}, // 0x3 disable I2C |
There was a problem hiding this comment.
The reset value of INTF_CONFIG0 is 0x30 which means FIFO_COUNT_ENDIAN and SENSOR_DATA_ENDIAN are also enabled by default.
Just for clarity can you either add them to the register config table or remove them from the INTF_CONFIG0_BITs? I know it's not quite there yet, but as much as possible I want to get to the point where our register config table is a declarative description of the entire register state (for the subset we care about).
There was a problem hiding this comment.
My original code just added the bits 0,1 , which disables the I2C, but didn't alter the default bits. I added the other two bits in here for clarity as you suggested.
jlaitine
force-pushed
the
icm42688p_disable_i2c
branch
2 times, most recently
from
d30f06f
to
009a100
Compare
This prevents accidental misconfiguration via I2C if there are multiple SPI devices on the same bus. The device may hear it's i2c address and write some register, while there is data transfer ongoing with another device. Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
jlaitine
force-pushed
the
icm42688p_disable_i2c
branch
from
009a100
to
293666f
Compare
|
@dagar, just checking if this was forgotten, or perhaps not needed in upstream px4? I believe I addressed the comments, so let's merge, modify further or close this PR 🙂 |
|
Looks good to me, but why is the voxl2-slpi version only partially updated? Hopefully that driver will be eliminated long term, but I think something must be missing in QuRT SPI support. |
I didn't update the voxl2 driver as I wasn't sure whether the change was wanted there or not;. Also, I didn't have that hw at hand. It is of course simple to add if wanted. |
This prevents accidental misconfiguration via I2C if there are multiple SPI devices on the same bus. The device may hear it's i2c address and write some register, while there is data transfer ongoing with another device.