Unify i2c_master API #14337
Unify i2c_master API #14337
Conversation
| #ifdef __AVR__ | ||
| i2c_start(I2C_ADDRESS_SA0_1, 100); | ||
| #else | ||
| i2c_start(I2C_ADDRESS_SA0_1); | ||
| #endif |
There was a problem hiding this comment.
Looks like this driver uses the high level I2C API (i2c_transmit()), except for this place; I suspect that this code could just be removed (the “normal” SSD1306/SH1106 OLED driver does not do anything like this, and works).
| i2c_start(address); | ||
| i2c_start(address, timeout); |
There was a problem hiding this comment.
This can just be removed, because the existing ChibiOS implementation of i2c_start() is useless (and everything that is done there is done inside i2c_readReg() again).
| i2c_status_t i2c_start(uint8_t address, uint16_t timeout); | ||
| i2c_status_t i2c_write(uint8_t data, uint16_t timeout); | ||
| int16_t i2c_read_ack(uint16_t timeout); | ||
| int16_t i2c_read_nack(uint16_t timeout); | ||
| i2c_status_t i2c_transmit(uint8_t address, const uint8_t* data, uint16_t length, uint16_t timeout); | ||
| i2c_status_t i2c_receive(uint8_t address, uint8_t* data, uint16_t length, uint16_t timeout); | ||
| i2c_status_t i2c_writeReg(uint8_t devaddr, uint8_t regaddr, const uint8_t* data, uint16_t length, uint16_t timeout); | ||
| i2c_status_t i2c_readReg(uint8_t devaddr, uint8_t regaddr, uint8_t* data, uint16_t length, uint16_t timeout); | ||
| void i2c_stop(void); |
There was a problem hiding this comment.
I think that i2c_start() and i2c_stop() should also be moved into the #if defined(__AVR__) block — the existing ChibiOS implementation of these functions does something completely different.
i2c_start() and i2c_stop() in the ChibiOS driver can be dropped completely — I don't think that there is any code that actually wants to use these functions in their current form (some code may be using them by mistake, like the QWIIK OLED driver).
There was a problem hiding this comment.
I'm not worried about the usage of i2c_start right now, as that's going to have to be tackled as part of the rework for #7967. That change would potentially introduce further platform specific ifdefs, which is what I want to avoid long term. Given the vast mess in this area.... theres going to be a fair few iterations to get things truly uniform.
There was a problem hiding this comment.
The point is, there is a low level API where you have functions that actually cause I2C start and stop signaling on the bus, and a high level API where all I2C details like start/stop are handled internally and not exposed to the API users. That high level API may need functions like i2c_lock() and i2c_unlock(), which is what #7967 tried to implement, but those functions should not be called i2c_start() and i2c_stop(), if we want to use those names for the low level I2C start/stop functions.
And maybe there should be a header file provided by the actual driver implementation, because there is also #12616 with the bitbang I2C implementation, and that implementation can also provide the low level API. So that #if defined(__AVR__) may become #if defined(I2C_RAW_API_AVAILABLE) or something like that.
There was a problem hiding this comment.
Its not that I dont get your point, its that I dont care for this phase of the work. The plan is to defer some of the API decisions till when they are reliant to the ongoing work. The i2c_start changes that are within this PR are limited to having the same function declaration. I know the functionality is not the same (i was the one who added the i2c_scanner keymap), and that will need some further work.
The ifdef is just there while I refactor, its not planned to have any platform specific code within the i2c_master.h file. The consumer should not care what is implementing the functionallity. The only reason that is done in a few places right now is down to short term goals. Both the existing bitbang PR proposals need some work, as i wouldnt be happy if they merged in their current form.
There was a problem hiding this comment.
As it is right now, the ARM_ATSAM keyboards also won't compile, cause the common i2c_master.h header file is found, and there are no common features between atsam's i2c_master.h and the common one.
Anyway let me know if I can help. Also let me know what changes you'd like to see in the i2c bitbang implementation.
| #include <hal.h> | ||
|
|
||
| #ifdef I2C1_BANK | ||
| # define I2C1_SCL_BANK I2C1_BANK |
There was a problem hiding this comment.
If this is moved out of the header file, something like this will start failing: See keyboards/matrix/m20add/m20add.c line 67
|
Thank you for your contribution! |
|
Thank you for your contribution! |
Description
Quite a lot to unpick
i2c_write/i2c_read_ack/i2c_read_nackon AVR where possiblei2c_starton ChbiosTypes of Changes
Checklist