Skip to content

Conversation

@anangl
Copy link
Member

@anangl anangl commented Sep 20, 2021

drivers: i2c_nrfx_twim: Use concatenation buffer by default

Issue an error logging message when the i2c_nrfx_twim driver lacks
a concatenation buffer big enough to properly handle a call to
i2c_burst_write() function, to give the user a hint what is wrong.

Also use by default a 16-bytes long concatenation buffer for every
instance of the i2c_nrfx_twim driver. Such value should cover most
of the simple uses of the i2c_burst_write() function, like those
in the stmemsc sensor drivers, and when a longer buffer is needed,
the user will be provided with the above message pointing to the
property that should be adjusted.

Fixes #33440.

drivers: i2c_nrfx_twim: Add handling of buffers located in flash

TWIM peripherals cannot perform write transactions from buffers
located in flash. The content of such buffers needs to be copied
to RAM before the actual transfer can be requested.
This commits adds a new property (zephyr,flash-buf-max-size) that
informs the driver how much space in RAM needs to be reserved for
such copying and adds proper handling of buffers located in flash.
This fixes an issue that caused that e.g. the DPS310 sensor driver
did not work on nRF SoCs that only have TWIM, not TWI peripherals.

Fixes #35550.

@anangl anangl added bug The issue is a bug, or the PR is fixing a bug area: Drivers area: I2C platform: nRF Nordic nRFx labels Sep 20, 2021
@anangl anangl added this to the v2.7.0 milestone Sep 20, 2021
@anangl
Copy link
Member Author

anangl commented Sep 20, 2021

cc @rajeev1986 @richardbarlow

Issue an error logging message when the i2c_nrfx_twim driver lacks
a concatenation buffer big enough to properly handle a call to
i2c_burst_write() function, to give the user a hint what is wrong.

Also use by default a 16-bytes long concatenation buffer for every
instance of the i2c_nrfx_twim driver. Such value should cover most
of the simple uses of the i2c_burst_write() function, like those
in the stmemsc sensor drivers, and when a longer buffer is needed,
the user will be provided with the above message pointing to the
property that should be adjusted.

Signed-off-by: Andrzej Głąbek <[email protected]>
TWIM peripherals cannot perform write transactions from buffers
located in flash. The content of such buffers needs to be copied
to RAM before the actual transfer can be requested.
This commits adds a new property (zephyr,flash-buf-max-size) that
informs the driver how much space in RAM needs to be reserved for
such copying and adds proper handling of buffers located in flash.
This fixes an issue that caused that e.g. the DPS310 sensor driver
did not work on nRF SoCs that only have TWIM, not TWI peripherals.

Signed-off-by: Andrzej Głąbek <[email protected]>
@anangl
Copy link
Member Author

anangl commented Sep 21, 2021

Rebased.

Copy link
Contributor

Choose a reason for hiding this comment

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

[00:00:16.188,903] <err> i2c_nrfx_twim: Need to use concatenation buffer and provided size is 
insufficient (1 + 1024 > 16). Adjust the zephyr,concat-buf-size property in the "I2C_0" node.

I would make it shorter.

diff --git a/drivers/i2c/i2c_nrfx_twim.c b/drivers/i2c/i2c_nrfx_twim.c
index a00d9fad00..a477f8fe57 100644
--- a/drivers/i2c/i2c_nrfx_twim.c
+++ b/drivers/i2c/i2c_nrfx_twim.c
@@ -86,13 +86,11 @@ static int i2c_nrfx_twim_transfer(const struct device *dev,
                 */
                if (concat_next || (msg_buf_used != 0)) {
                        if ((msg_buf_used + msgs[i].len) > concat_buf_size) {
-                               LOG_ERR("Need to use concatenation buffer and "
-                                       "provided size is insufficient "
-                                       "(%u + %u > %u). "
-                                       "Adjust the zephyr,concat-buf-size "
-                                       "property in the \"%s\" node.",
+                               LOG_ERR("Concatenation buffer is insufficient "
+                                       "(%u + %u > %u)",
                                        msg_buf_used, msgs[i].len,
-                                       concat_buf_size, dev->name);
+                                       concat_buf_size);
+                               LOG_ERR("Adjust the zephyr,concat-buf-size");
                                ret = -ENOSPC;
                                break;
                        }

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I wanted to give the user a clear indication what is wrong and how to solve the problem. Hence such verbose message. But the message is not supposed to occur normally, as it indicates an invalid build configuration, so I think its length should not be a problem and I'd rather keep it as it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree area: Drivers area: I2C bug The issue is a bug, or the PR is fixing a bug platform: nRF Nordic nRFx

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nRF91: DPS310 I2C driver not working lsm6dso sensor driver not working on nRF5340

4 participants