Adds read-only property for the I2C EEPROM Target #75777

Manu3l0us · 2024-07-11T12:47:29Z

In case the EEPROM data stored in the buffer shall be not writable from the I2C interface, the read-only property can now be set in the device tree.

drivers/i2c/target/eeprom_target.c

henrikbrixandersen

Looks good! Now all that's needed is a corresponding test case in tests/drivers/i2c/i2c_target_api/ 👍

Manu3l0us · 2024-07-15T10:52:35Z

Looks good! Now all that's needed is a corresponding test case in tests/drivers/i2c/i2c_target_api/ 👍

Good point, I will figure out how and update the PR once done.

Manu3l0us · 2024-07-15T12:02:32Z

@henrikbrixandersen I am thinking about adding another EEPROM which is read-only to the device tree overlays within the test project. Also an EEPROM with 16bit addressing should be added to increase test coverage of the existing code which also supports this since a couple of months. I could probably do that at the same time. Does this sound like the right approach to you?

henrikbrixandersen · 2024-07-15T15:25:39Z

@henrikbrixandersen I am thinking about adding another EEPROM which is read-only to the device tree overlays within the test project. Also an EEPROM with 16bit addressing should be added to increase test coverage of the existing code which also supports this since a couple of months. I could probably do that at the same time. Does this sound like the right approach to you?

Yes, that sounds good along with some accompanying test cases.

github-actions · 2024-09-14T00:30:24Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

Manu3l0us · 2024-09-26T13:21:34Z

I took this up again as I was busy with other stuff during the last months. I tried to run the existing test on a RPi Pico but couldn't get it to work until now. What hardware setup is needed to run those tests?

Manu3l0us · 2024-11-14T14:07:58Z

@henrikbrixandersen I finally had the time to write a simple test and I pushed my work in progress. Some questions:

On the i2c_dw based target I'm using here, no NACK is generated and therefore I don't get a non-zero return code. Therefore I currently commented this check out for the test to run. Should I add a platform dependent test condition or how would you handle this?
I added the overlay for the rpi_pico which I currently use and I updated the overlay for it. What is the general expectation when adding test cases? Shall I update the other overlays as well? I don't have the hardware to test any of those boards.

In case the EEPROM data stored in the buffer shall be not writable from the I2C interface, the read-only property can now be set in the device tree. Signed-off-by: Manuel Aebischer <[email protected]>

This additional test case verifies that an i2c_eeprom_target, which is set as read-only in the device tree cannot be written from the I2C bus. Signed-off-by: Manuel Aebischer <[email protected]>

Manu3l0us · 2024-11-14T18:37:53Z

Okay, I saw that the boards are tested in CI, so I can safely add it to the overlays without breaking the tests

henrikbrixandersen

Thanks! However, I don't think we need this read-only EEPROM added to each and every board here (and we really need to cut down on these boards).

Perhaps make the test conditional on the device being available (skip it if not present) and just add it to the board(s), you've tested with?

henrikbrixandersen · 2024-11-23T19:18:12Z

tests/drivers/i2c/i2c_target_api/src/main.c

+#ifndef CONFIG_I2C_DW
+	zassert_not_equal(ret, 0, "Write attempt to read-only EEPROM 2 succeeded unexpectedly");
+#endif


Why is this needed?

Usually the target should generate a NAK if it's being written and it's write protected, therefore the write on the controller side should return a non-zero return code. Unfortunately, the DesignWare I2C does not seem to implement this NAKing when used in target mode. I see if I can get hold of some other boards (e.g. STM32) to verify this as well.

Manu3l0us · 2024-11-23T20:38:40Z

Thanks! However, I don't think we need this read-only EEPROM added to each and every board here (and we really need to cut down on these boards).

Perhaps make the test conditional on the device being available (skip it if not present) and just add it to the board(s), you've tested with?

Okay, understood. Will clean this up the coming week hopefully.

github-actions · 2025-01-23T00:33:13Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

decsny

dt prop is fine

msalau · 2025-03-25T08:57:26Z

drivers/i2c/target/eeprom_target.c

+#define I2C_EEPROM_INIT(inst)                                                                      \
+	static struct i2c_eeprom_target_data i2c_eeprom_target_##inst##_dev_data = {               \
+		.address_width = DT_INST_PROP_OR(inst, address_width, 8),                          \
+		.read_only = DT_INST_PROP_OR(inst, read_only, 8),                                  \


Looks like a copy-paste error. The read-only property is always present due to its type being boolean so there is no reason to give it a default value.

Suggested change

.read_only = DT_INST_PROP_OR(inst, read_only, 8), \

.read_only = DT_INST_PROP(inst, read_only), \

henrikbrixandersen · 2025-05-02T08:12:51Z

@Manu3l0us Any plans for resuming work on this?

henrikbrixandersen · 2025-06-22T16:10:10Z

@Manu3l0us I'm going to close this due to lack of response, although I'd love to see this feature make it in. Please reopen it if you decide to come back to this.

Manu3l0us requested a review from henrikbrixandersen as a code owner July 11, 2024 12:47

zephyrbot added area: EEPROM area: I2C labels Jul 11, 2024

zephyrbot requested a review from teburd July 11, 2024 12:48

zephyrbot assigned teburd Jul 11, 2024

Manu3l0us force-pushed the read_only_eeprom_target branch from aba8e43 to b119543 Compare July 11, 2024 12:57

zephyrbot added the size: XS A PR changing only a single line of code label Jul 11, 2024

Manu3l0us force-pushed the read_only_eeprom_target branch from b119543 to 0aa34cf Compare July 11, 2024 14:09

henrikbrixandersen requested changes Jul 11, 2024

View reviewed changes

drivers/i2c/target/eeprom_target.c Outdated Show resolved Hide resolved

Manu3l0us force-pushed the read_only_eeprom_target branch 2 times, most recently from 2e4621a to 2abe1e1 Compare July 11, 2024 17:03

Manu3l0us requested a review from henrikbrixandersen July 11, 2024 18:24

henrikbrixandersen requested changes Jul 12, 2024

View reviewed changes

github-actions bot added the Stale label Sep 14, 2024

github-actions bot removed the Stale label Sep 27, 2024

Manu3l0us force-pushed the read_only_eeprom_target branch from 2abe1e1 to b6d33a6 Compare November 14, 2024 13:53

zephyrbot added the area: Devicetree label Nov 14, 2024

zephyrbot requested review from decsny and galak November 14, 2024 13:53

Manu3l0us force-pushed the read_only_eeprom_target branch 4 times, most recently from 9816428 to 0db9102 Compare November 14, 2024 15:02

i2c: target: eeprom_target: add read-only property

c172b4b

In case the EEPROM data stored in the buffer shall be not writable from the I2C interface, the read-only property can now be set in the device tree. Signed-off-by: Manuel Aebischer <[email protected]>

Manu3l0us force-pushed the read_only_eeprom_target branch 3 times, most recently from f136954 to 7d2aeea Compare November 14, 2024 15:59

tests: i2c: i2c_target_api: Add test for read-only eeprom target

800ce8d

This additional test case verifies that an i2c_eeprom_target, which is set as read-only in the device tree cannot be written from the I2C bus. Signed-off-by: Manuel Aebischer <[email protected]>

Manu3l0us force-pushed the read_only_eeprom_target branch from 7d2aeea to 800ce8d Compare November 14, 2024 18:36

Manu3l0us requested a review from henrikbrixandersen November 19, 2024 12:34

henrikbrixandersen requested changes Nov 23, 2024

View reviewed changes

github-actions bot added the Stale label Jan 23, 2025

decsny reviewed Jan 27, 2025

View reviewed changes

github-actions bot removed the Stale label Jan 28, 2025

msalau reviewed Mar 25, 2025

View reviewed changes

henrikbrixandersen closed this Jun 22, 2025

	.read_only = DT_INST_PROP_OR(inst, read_only, 8), \
	.read_only = DT_INST_PROP(inst, read_only), \

Adds read-only property for the I2C EEPROM Target #75777

Adds read-only property for the I2C EEPROM Target #75777

Uh oh!

Conversation

Manu3l0us commented Jul 11, 2024

Uh oh!

Uh oh!

henrikbrixandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Manu3l0us commented Jul 15, 2024

Uh oh!

Manu3l0us commented Jul 15, 2024

Uh oh!

henrikbrixandersen commented Jul 15, 2024

Uh oh!

github-actions bot commented Sep 14, 2024

Uh oh!

Manu3l0us commented Sep 26, 2024

Uh oh!

Manu3l0us commented Nov 14, 2024

Uh oh!

Manu3l0us commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henrikbrixandersen left a comment

Choose a reason for hiding this comment

Uh oh!

henrikbrixandersen Nov 23, 2024

Choose a reason for hiding this comment

Uh oh!

Manu3l0us Nov 23, 2024

Choose a reason for hiding this comment

Uh oh!

Manu3l0us commented Nov 23, 2024

Uh oh!

github-actions bot commented Jan 23, 2025

Uh oh!

decsny left a comment

Choose a reason for hiding this comment

Uh oh!

msalau Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

henrikbrixandersen commented May 2, 2025

Uh oh!

henrikbrixandersen commented Jun 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Manu3l0us commented Nov 14, 2024 •

edited

Loading