i2c HAL refactor #265
i2c HAL refactor #265
Conversation
…ucts generation script
fix hardware
|
|
||
| /** | ||
| * Computes default timing parameters for a particular I2C speed, given the | ||
| * clock period, in nanoseconds. | ||
| * | ||
| * Returns an unspecified value for an invalid speed. | ||
| */ | ||
| static i2c_config_t default_timing_for_speed(i2c_speed_t speed, |
There was a problem hiding this comment.
please, comment the function in doxyg. format
sw/device/lib/drivers/i2c/i2c.c
Outdated
| } | ||
| uint32_t lowest_target_device_speed_khz; | ||
| switch (timing_config.lowest_target_device_speed) { | ||
| case kDifI2cSpeedStandard: | ||
| case kI2cSpeedStandard: | ||
| lowest_target_device_speed_khz = 100; |
There was a problem hiding this comment.
can we have defines for those values?
| /** | ||
| * @file i2c.c | ||
| * @date 17/04/2023 | ||
| * @brief This is the main file for the HAL of the I2C peripheral |
There was a problem hiding this comment.
missing here the author and version
| } | ||
|
|
||
|
|
||
| i2c_config_t default_timing_for_speed(i2c_speed_t speed, |
There was a problem hiding this comment.
this function relies on multiple ad-hoc values, should we define those in DEFINEs etc?
| .stop_signal_hold_cycles = round_up_divide(500, clock_period_nanos), | ||
| }; | ||
| default: | ||
| return (i2c_config_t){0}; |
There was a problem hiding this comment.
this is a little bit weird, let's discuss this
| /* | ||
| * The "enabled" state. | ||
| typedef enum i2c_result { | ||
| /** |
There was a problem hiding this comment.
why are you assigning values to this if that are already by default?
sw/device/lib/drivers/i2c/i2c.h
Outdated
| /** **/ | ||
| /****************************************************************************/ | ||
|
|
||
| #ifndef _TEMPLATE_C_SRC |
There was a problem hiding this comment.
this needs to be changed
There was a problem hiding this comment.
we need to speak about how to conceptualise the i2c design (HAL and upper layers). Right now the control config/block is transparent for anyone regardless of the layer he/she can access to (I can modify the config even if I am in the APP layer). Plus, there is no IRQ management control into the current HAL. Let's talk about this (for example, check TWI info in here: https://infocenter.nordicsemi.com/index.jsp?topic=%2Fstruct_sdk%2Fstruct%2Fsdk_nrf5_latest.html)
Major refactoring of the HAL for the i2c peripheral.
The main changes are due to the usage of the memory mapped
i2c_peristructure to access directly the registers.Moreover the code has been modified to be more clear and compact.
The
irq_index()function has been completely removed and substituted with sanity checks on the irq variable. For this, an additional field in thei2c_irq_tenum has been added in order to always keep track of the number of different interrupt types related to the i2c.