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.

Sign up for GitHub

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

Unify i2c_master API #14337

Closed
wants to merge 3 commits into from
Closed

Unify i2c_master API #14337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a353839
Unify i2c_master API
zvecr Sep 6, 2021
a5205ad
Unify i2c_master API
zvecr Sep 6, 2021
4e4dc73
Fix up start refs
zvecr Sep 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 23 additions & 21 deletions platforms/avr/drivers/i2c_master.h → drivers/i2c_master.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,21 @@
/* Copyright (C) 2019 Elia Ritterbusch
+
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
/* Copyright 2021 QMK
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 of the License, or
* (at your option) any later version.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*/
/* Library made by: g4lvanix
* GitHub repository: https://github.com/g4lvanix/I2C-master-lib
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#pragma once

#define I2C_READ 0x01
#define I2C_WRITE 0x00
#include <stdint.h>

typedef int16_t i2c_status_t;

Expand All @@ -33,11 +28,18 @@ typedef int16_t i2c_status_t;

void i2c_init(void);
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);
Comment on lines 30 to 35
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

@zvecr zvecr Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


// deprecated - do not use
#if defined(__AVR__)
# define I2C_READ 0x01
# define I2C_WRITE 0x00

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);
#endif
4 changes: 0 additions & 4 deletions drivers/qwiic/micro_oled.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,7 @@ static uint8_t micro_oled_screen_buffer[LCDWIDTH * LCDHEIGHT / 8] = {0};
void micro_oled_init(void) {
i2c_init();

#ifdef __AVR__
i2c_start(I2C_ADDRESS_SA0_1, 100);
#else
i2c_start(I2C_ADDRESS_SA0_1);
#endif
Comment on lines -115 to -119
Copy link
Contributor

Choose a reason for hiding this comment

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

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).


// Display Init sequence for 64x48 OLED module
send_command(DISPLAYOFF); // 0xAE
Expand Down
4 changes: 2 additions & 2 deletions keyboards/handwired/onekey/keymaps/i2c_scanner/keymap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@
# pragma message("ChibiOS is currently 'best effort' and might not report accurate results")

i2c_status_t i2c_start_bodge(uint8_t address, uint16_t timeout) {
i2c_start(address);
i2c_start(address, timeout);
Copy link
Contributor

Choose a reason for hiding this comment

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

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).


// except on ChibiOS where the only way is do do "something"
uint8_t data = 0;
return i2c_readReg(address, 0, &data, sizeof(data), TIMEOUT);
return i2c_readReg(address, 0, &data, sizeof(data), timeout);
}

# define i2c_start i2c_start_bodge
Expand Down
7 changes: 5 additions & 2 deletions platforms/avr/drivers/i2c_master.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
# define I2C_START_RETRY_COUNT 20
#endif // I2C_START_RETRY_COUNT

#define I2C_ACTION_READ 0x01
#define I2C_ACTION_WRITE 0x00

#define TWBR_val (((F_CPU / F_SCL) - 16) / 2)

#define MAX(X, Y) ((X) > (Y) ? (X) : (Y))
Expand Down Expand Up @@ -154,7 +157,7 @@ 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 status = i2c_start(address | I2C_WRITE, timeout);
i2c_status_t status = i2c_start(address | I2C_ACTION_WRITE, timeout);

for (uint16_t i = 0; i < length && status >= 0; i++) {
status = i2c_write(data[i], timeout);
Expand All @@ -166,7 +169,7 @@ i2c_status_t i2c_transmit(uint8_t address, const uint8_t* data, uint16_t length,
}

i2c_status_t i2c_receive(uint8_t address, uint8_t* data, uint16_t length, uint16_t timeout) {
i2c_status_t status = i2c_start(address | I2C_READ, timeout);
i2c_status_t status = i2c_start(address | I2C_ACTION_READ, timeout);

for (uint16_t i = 0; i < (length - 1) && status >= 0; i++) {
status = i2c_read_ack(timeout);
Expand Down
75 changes: 74 additions & 1 deletion platforms/chibios/drivers/i2c_master.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,80 @@
#include "quantum.h"
#include "i2c_master.h"
#include <string.h>
#include <ch.h>
#include <hal.h>

#ifdef I2C1_BANK
# define I2C1_SCL_BANK I2C1_BANK
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is moved out of the header file, something like this will start failing: See keyboards/matrix/m20add/m20add.c line 67

# define I2C1_SDA_BANK I2C1_BANK
#endif

#ifndef I2C1_SCL_BANK
# define I2C1_SCL_BANK GPIOB
#endif

#ifndef I2C1_SDA_BANK
# define I2C1_SDA_BANK GPIOB
#endif

#ifndef I2C1_SCL
# define I2C1_SCL 6
#endif
#ifndef I2C1_SDA
# define I2C1_SDA 7
#endif

#ifdef USE_I2CV1
# ifndef I2C1_OPMODE
# define I2C1_OPMODE OPMODE_I2C
# endif
# ifndef I2C1_CLOCK_SPEED
# define I2C1_CLOCK_SPEED 100000 /* 400000 */
# endif
# ifndef I2C1_DUTY_CYCLE
# define I2C1_DUTY_CYCLE STD_DUTY_CYCLE /* FAST_DUTY_CYCLE_2 */
# endif
#else
// The default timing values below configures the I2C clock to 400khz assuming a 72Mhz clock
// For more info : https://www.st.com/en/embedded-software/stsw-stm32126.html
# ifndef I2C1_TIMINGR_PRESC
# define I2C1_TIMINGR_PRESC 0U
# endif
# ifndef I2C1_TIMINGR_SCLDEL
# define I2C1_TIMINGR_SCLDEL 7U
# endif
# ifndef I2C1_TIMINGR_SDADEL
# define I2C1_TIMINGR_SDADEL 0U
# endif
# ifndef I2C1_TIMINGR_SCLH
# define I2C1_TIMINGR_SCLH 38U
# endif
# ifndef I2C1_TIMINGR_SCLL
# define I2C1_TIMINGR_SCLL 129U
# endif
#endif

#ifndef I2C_DRIVER
# define I2C_DRIVER I2CD1
#endif

#ifdef USE_GPIOV1
# ifndef I2C1_SCL_PAL_MODE
# define I2C1_SCL_PAL_MODE PAL_MODE_STM32_ALTERNATE_OPENDRAIN
# endif
# ifndef I2C1_SDA_PAL_MODE
# define I2C1_SDA_PAL_MODE PAL_MODE_STM32_ALTERNATE_OPENDRAIN
# endif
#else
// The default PAL alternate modes are used to signal that the pins are used for I2C
# ifndef I2C1_SCL_PAL_MODE
# define I2C1_SCL_PAL_MODE 4
# endif
# ifndef I2C1_SDA_PAL_MODE
# define I2C1_SDA_PAL_MODE 4
# endif
#endif

static uint8_t i2c_address;

static const I2CConfig i2cconfig = {
Expand Down Expand Up @@ -77,7 +149,8 @@ __attribute__((weak)) void i2c_init(void) {
}
}

i2c_status_t i2c_start(uint8_t address) {
i2c_status_t i2c_start(uint8_t address, uint16_t timeout) {
(void)timeout;
i2c_address = address;
i2cStart(&I2C_DRIVER, &i2cconfig);
return I2C_STATUS_SUCCESS;
Expand Down
113 changes: 0 additions & 113 deletions platforms/chibios/drivers/i2c_master.h
View file
Open in desktop

This file was deleted.