drivers: add i2c driver support for MSPM0 #94627

d-philpot · 2025-08-19T03:21:51Z

Adds I2C controller and target support for MSPM0. Includes driver changes, dts and board updates.

drivers/i2c/i2c_mspm0.c

comment addressed

parthitce

This driver needs lot of re-factor. In summary,

ISR context is handling all the data path, which needs to be abstracted.
Target and Controller support needs to be split.
Multi target support needs to be compile time based on devicetree target nodes.
Current usage of data section is high and needs to be optimized based on rodata / data place-holding.

Current code base is not compilable as well. So IMO it makes sense to put in DRAFT until it's ready for review.

teburd

There's some confusing things going on, maybe because the HAL is handling interrupts? It's unclear, but those should be handled by Zephyr not by the HAL if that's the case.

teburd · 2025-08-21T15:36:14Z

drivers/i2c/i2c_mspm0.c

+DL_I2C_startControllerTransfer(config->base, data->addr, DL_I2C_CONTROLLER_DIRECTION_TX,
+ data->msg.len);
+
+while (!(DL_I2C_getControllerStatus(config->base) & DL_I2C_CONTROLLER_STATUS_IDLE)) {


Tight polling loops like this aren't inherently wrong but typically there's an interrupt signal available.

Would recommend changing the driver, if possible, to use an interrupt (event driven) state machine that is kicked off by the transfer function, and completed when the state machine signals back to the thread that had called transfer blocking on a k_sem_take by giving a semaphore.

If you really want the tight poll loop here instead, or there's no other option, use a WAIT_FOR() macro with a timeout such that even given a faulty i2c transfer the caller isn't blocked forever and instead may get an error.

https://docs.zephyrproject.org/latest/doxygen/html/group__sys-util.html#ga68eb35df6b4715714312df90209accee

Looking into refactoring transfer to use semaphores

teburd · 2025-08-21T15:36:38Z

drivers/i2c/i2c_mspm0.c

+while (!(DL_I2C_getControllerStatus(config->base) & DL_I2C_CONTROLLER_STATUS_IDLE)) {
+}
+
+if (data->state == I2C_MSPM0_TIMEOUT) {


And I maybe spoke too soon if there's a hardware timeout available.

There is a configurable SCL low hardware timeout available

teburd · 2025-08-21T15:39:03Z

dts/bindings/i2c/ti,mspm0-i2c.yaml

+ reg:
+ required: true
+
+ interrupts:


I don't see the interrupts being used?

teburd · 2025-08-21T15:39:13Z

dts/arm/ti/mspm0/l/mspm0lx22x.dtsi

+#address-cells = <1>;
+#size-cells = <0>;
+reg = <0x400f2000 0x2000>;
+interrupts = <26 0>;


It doesn't seem like the driver supports interrupts

Interrupts are used for controller and target logic

d-philpot · 2025-08-22T19:22:32Z

This driver needs lot of re-factor. In summary,

ISR context is handling all the data path, which needs to be abstracted.

Target and Controller support needs to be split.

Multi target support needs to be compile time based on devicetree target nodes.

Current usage of data section is high and needs to be optimized based on rodata / data place-holding.

Current code base is not compilable as well. So IMO it makes sense to put in DRAFT until it's ready for review.

The code base is compliable. Target support is split and optional from controller via kconfig define. Multi target support is compile time based on Kconfig defines

d-philpot · 2025-08-27T19:27:47Z

@teburd Ping for re-review. Refactored transmit logic to use semaphores instead of busy wait looping

tbursztyka

ISR handler would gain in clarity being split: irq types handled in dedicated inline functions and as noted already: target stuff being on their own as well.

drivers/i2c/i2c_mspm0.c

github-actions · 2025-09-11T21:47:55Z

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_ti	zephyrproject-rtos/hal_ti@`cc04902`	zephyrproject-rtos/hal_ti@`7b34b50`	zephyrproject-rtos/hal_ti@cc049020..7b34b502

✅ All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

d-philpot · 2025-10-07T20:27:41Z

@teburd @tbursztyka @ssekar15 Please review

d-philpot · 2025-10-27T15:29:34Z

@teburd ping for re-review, updated manifest

parthitce

Am still having a closer look.

parthitce · 2025-10-28T15:17:51Z

drivers/i2c/i2c_mspm0.c

+struct i2c_msg transaction_msg;
+int ret = 0;
+
+k_sem_take(data->i2c_busy_sem, K_FOREVER);


IMO there is no real protection against the target mode here. Move below the is_target condition check?

parthitce · 2025-10-28T15:22:45Z

drivers/i2c/i2c_mspm0.c

+uint32_t speed_config;
+
+/* Init power */
+DL_I2C_reset(config->base);


Not sure reset before enabling power will work

parthitce · 2025-10-28T15:50:22Z

drivers/i2c/i2c_mspm0.c

+ret = clock_control_get_rate(clk_dev, (clock_control_subsys_t)config->clock_subsys,
+ &clock_rate);
+if (ret < 0) {
+ret = -ENODEV;


propagate the clock error instead?

parthitce · 2025-10-28T15:50:54Z

drivers/i2c/i2c_mspm0.c

+goto out;
+}
+
+if (dev_config & I2C_MSG_ADDR_10_BITS) {


this sanity check can be done as entry into the function and return directly

parthitce · 2025-10-28T15:58:31Z

boards/ti/lp_mspm0g3507/lp_mspm0g3507.dts

 ti,pwm-mode = "EDGE_ALIGN";
 ti,period = <1000>;
 };
+


IMO this commit can be dropped. Sample or App can decide the whole usage in dts, pins and Kconfig. Doesn't needs to be enabled by default.

parthitce · 2025-10-28T16:00:07Z

dts/arm/ti/mspm0/g/mspm0g110x.dtsi

+
+/ {
+soc {
+i2c0: i2c@400f0000 {


can we keep the common i2c which is available across all series in mspm0.dtsi?

future mspm0 devices will not all have base i2c0 peripheral

parthitce · 2025-10-28T16:10:30Z

drivers/i2c/i2c_mspm0.c

@@ -0,0 +1,814 @@
+/*


bindings is not part of this commit. either adjust the commit message or add the bindings file here

parthitce · 2025-10-28T16:20:19Z

drivers/i2c/i2c_mspm0.c

+return 0;
+}
+
+static int i2c_mspm0_reset_peripheral_target(const struct device *dev)


Shall we instead wrap this with recover_bus and leave the decision to consumer?

parthitce · 2025-10-28T16:22:06Z

drivers/i2c/i2c_mspm0.c

+struct i2c_target_config *target_config;
+const struct i2c_target_callbacks *target_callbacks;
+#endif /* CONFIG_I2C_TARGET */
+bool is_target;


this can be dropped IMO

is_target is used during runtime for apis and ISRs

parthitce · 2025-10-28T16:24:29Z

drivers/i2c/i2c_mspm0.c

+goto out;
+}
+
+k_sem_take(data->i2c_busy_sem, K_FOREVER);


Protecting the controller / IP as a resource doesn't need to lock for the whole function. Applies everywhere.

parthitce · 2025-10-28T16:26:30Z

drivers/i2c/i2c_mspm0.c

+{
+struct i2c_mspm0_data *data = dev->data;
+*dev_config = data->dev_config;
+return 0;


if the i2c is not configured, this API shall return error as per API description

parthitce · 2025-10-28T16:27:14Z

drivers/i2c/i2c_mspm0.c

+data->dev_config = dev_config;
+
+#if (CONFIG_I2C_SCL_LOW_TIMEOUT != 0)
+ret = i2c_mspm0_configure_timeout(dev, period, CONFIG_I2C_SCL_LOW_TIMEOUT);


proceeding on error?

parthitce · 2025-10-28T16:29:12Z

drivers/i2c/i2c_mspm0.c

+DL_I2C_setTargetOwnAddress(config->base, data->target_config->address);
+DL_I2C_setTargetTXFIFOThreshold(config->base, DL_I2C_TX_FIFO_LEVEL_BYTES_1);
+DL_I2C_setTargetRXFIFOThreshold(config->base, DL_I2C_RX_FIFO_LEVEL_BYTES_1);
+DL_I2C_enableTargetTXTriggerInTXMode(config->base);


but the trigger levels aren't handled in ISR?

The FIFO trigger interrupts are used in the ISR

Changes default pin properties for I2C. Signed-off-by: Dylan Philpot <d-philpot@ti.com>

Includes driver implementation for I2C controller and peripheral support. Signed-off-by: Dylan Philpot <d-philpot@ti.com>

Adds devicetree entries for different I2C peripherals on MSPM0 devices and dts binding changes for I2C driver. Signed-off-by: Dylan Philpot <d-philpot@ti.com>

sonarqubecloud · 2025-11-04T13:59:57Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

zephyrbot added platform: Texas Instruments MSPM0 area: I2C platform: TI SimpleLink Texas Instruments SimpleLink MCU labels Aug 19, 2025

zephyrbot requested review from ssekar15, teburd, vaishnavachath and vanti August 19, 2025 03:22

zephyrbot assigned teburd Aug 19, 2025

pdgendt previously requested changes Aug 19, 2025

View reviewed changes

drivers/i2c/i2c_mspm0.c Outdated Show resolved Hide resolved

d-philpot force-pushed the i2c_dev branch from 72c1da1 to cda6ccf Compare August 19, 2025 14:37

parthitce linked an issue Aug 20, 2025 that may be closed by this pull request

I2C linumiz/zephyr#70

Open

parthitce requested changes Aug 20, 2025

View reviewed changes

teburd requested changes Aug 21, 2025

View reviewed changes

d-philpot force-pushed the i2c_dev branch 3 times, most recently from 1f122c6 to b708dc5 Compare August 27, 2025 18:27

d-philpot requested review from pdgendt and teburd August 27, 2025 19:27

tbursztyka requested changes Sep 9, 2025

View reviewed changes

drivers/i2c/i2c_mspm0.c Outdated Show resolved Hide resolved

drivers/i2c/i2c_mspm0.c Outdated Show resolved Hide resolved

d-philpot force-pushed the i2c_dev branch from b708dc5 to 02e52af Compare September 11, 2025 21:46

zephyrbot added the area: Charger label Sep 11, 2025

zephyrbot requested a review from rriveramcrus September 11, 2025 21:47

github-actions bot added manifest manifest-hal_ti DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Sep 11, 2025

d-philpot force-pushed the i2c_dev branch from 02e52af to 914b39b Compare September 11, 2025 21:59

d-philpot requested review from parthitce and tbursztyka October 1, 2025 01:01

tbursztyka previously approved these changes Oct 1, 2025

View reviewed changes

d-philpot dismissed tbursztyka’s stale review via 16d5a03 October 7, 2025 20:18

d-philpot force-pushed the i2c_dev branch from 4b6403f to 16d5a03 Compare October 7, 2025 20:18

tbursztyka previously approved these changes Oct 8, 2025

View reviewed changes

nandojve removed platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) platform: Microchip MEC Microchip MEC Platform labels Oct 20, 2025

d-philpot dismissed tbursztyka’s stale review via 620bab0 October 21, 2025 19:45

d-philpot force-pushed the i2c_dev branch from 16d5a03 to 620bab0 Compare October 21, 2025 19:45

zephyrbot added area: Devicetree Bindings area: Boards/SoCs labels Oct 21, 2025

teburd previously approved these changes Oct 21, 2025

View reviewed changes

d-philpot dismissed teburd’s stale review via 3ccd1f6 October 27, 2025 14:19

d-philpot force-pushed the i2c_dev branch from 620bab0 to 3ccd1f6 Compare October 27, 2025 14:19

github-actions bot removed the DNM (manifest) This PR should not be merged (controlled by action-manifest) label Oct 27, 2025

zephyrbot requested a review from teburd October 27, 2025 14:21

d-philpot requested a review from tbursztyka October 27, 2025 15:29

parthitce requested changes Oct 28, 2025

View reviewed changes

d-philpot force-pushed the i2c_dev branch 2 times, most recently from 46a5870 to 04a2416 Compare November 3, 2025 20:58

d-philpot added 3 commits November 4, 2025 07:54

west: update to latest hal commit

77e39ad

Changes default pin properties for I2C. Signed-off-by: Dylan Philpot <d-philpot@ti.com>

drivers: add I2C mspm0 driver

b7f1107

Includes driver implementation for I2C controller and peripheral support. Signed-off-by: Dylan Philpot <d-philpot@ti.com>

dts: add I2C nodes for MSPM0

2579374

Adds devicetree entries for different I2C peripherals on MSPM0 devices and dts binding changes for I2C driver. Signed-off-by: Dylan Philpot <d-philpot@ti.com>

d-philpot force-pushed the i2c_dev branch from 04a2416 to 2579374 Compare November 4, 2025 13:55

d-philpot requested a review from parthitce November 5, 2025 15:32

drivers: add i2c driver support for MSPM0 #94627

Are you sure you want to change the base?

drivers: add i2c driver support for MSPM0 #94627

Conversation

d-philpot commented Aug 19, 2025

Uh oh!

parthitce left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

teburd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-philpot Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-philpot commented Aug 22, 2025

d-philpot commented Aug 27, 2025

tbursztyka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

d-philpot commented Oct 7, 2025

d-philpot commented Oct 27, 2025

parthitce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-philpot Nov 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 4, 2025

Quality Gate passed

Labels

8 participants

parthitce left a comment •

edited

Loading

d-philpot Aug 22, 2025 •

edited

Loading

github-actions bot commented Sep 11, 2025 •

edited

Loading

d-philpot Nov 2, 2025 •

edited

Loading