drivers: i2c: mchp_sercom_g1: add multi-address target support by sashapapo-mchp · Pull Request #16 · Zephyr4Microchip/zephyr

sashapapo-mchp · 2026-01-19T16:39:44Z

Add support for registering up to two I2C target addresses on a single SERCOM instance, leveraging the hardware's dual-address capability via the AMODE field.

Changes:

Replace single target_config/target_callbacks with target_cfgs[2] pointer array to support multiple registered configurations
Add target_cfg_count and active_target_cfg tracking
Add helper functions for address matching and configuration lookup
Update i2c_target_register() for slot-based registration with validation of 7-bit/10-bit address constraints
Update i2c_target_unregister() with slot compaction when removing the primary address
Update interrupt handlers to dispatch to the correct configuration based on the matched address

The secondary address uses the ADDRMASK register with AMODE set to 2_ADDRESSES mode. Both addresses must be 7-bit when using dual-address mode per hardware requirements.

fabin-mchp · 2026-01-27T10:49:55Z

-	struct i2c_target_config target_config;
-	struct i2c_target_callbacks target_callbacks;
+	/* Registered target configs (hardware supports up to two addresses). */
+	struct i2c_target_config *target_cfgs[2];


Add macro for size

Added I2C_MCHP_TARGET_CFG_SLOTS and used it for the array/loops.

fabin-mchp · 2026-01-27T11:02:57Z

-		/* Load the first byte for host read */
-		target_cb->read_requested(&data->target_config, &data->rx_tx_data);
+		/* Host read (target transmits) */
+		if (target_cb->read_requested) {


Add explicit NULL check

Added explicit NULL checks for active_target_cfg and callbacks in address-match.

Explicit check should be added like this as per our coding guide lines
if (target_cb->read_requested != NULL)

fabin-mchp · 2026-01-27T11:06:50Z

+		return;
+	}
+
+	data->active_target_cfg = cfg;


data->active_target_cfg further not used in the function

Now used directly; cfg local removed and callbacks use active_target_cfg.

fabin-mchp · 2026-01-27T11:07:56Z

 {
-	const struct i2c_target_callbacks *target_cb = &data->target_callbacks;
+	uint16_t matched_addr = i2c_mchp_get_matched_addr(dev);
+	struct i2c_target_config *cfg = i2c_mchp_find_target_cfg(data, matched_addr);


Can we use data->active_target_cfg directly here instead of extra local variable

Done; address-match uses data->active_target_cfg directly.

fabin-mchp · 2026-01-27T11:09:57Z

-	const struct i2c_target_callbacks *target_cb = &data->target_callbacks;
+	uint16_t matched_addr = i2c_mchp_get_matched_addr(dev);
+	struct i2c_target_config *cfg = i2c_mchp_find_target_cfg(data, matched_addr);
+	const struct i2c_target_callbacks *target_cb = NULL;


Can we initialize here directly
const struct i2c_target_callbacks *target_cb = data->active_target_cfg->callbacks

Done; target_cb now comes from data->active_target_cfg->callbacks.

fabin-mchp · 2026-01-27T13:47:11Z

+
+	for (size_t i = 0U; i < ARRAY_SIZE(data->target_cfgs); i++) {
+		if (data->target_cfgs[i] == target_cfg) {
+			slot = (int)i;


Same comments applicable as above

Same loop updates applied in unregister path as well.

fabin-mchp · 2026-01-27T13:47:27Z

+
+	if (slot < 0) {
+		retval = -EINVAL;
+		goto unlock;


Avoid using goto

Replaced goto unlock with do { ... } while (false) in unregister.

fabin-mchp · 2026-01-27T13:48:59Z

+	}
+
+	if (!data->target_mode) {
+		i2c_controller_enable(dev, false);


Why i2c_controller_enable is called from here to disable controller

fabin-mchp · 2026-01-27T13:53:01Z

 		return -EINVAL;
 	}
-	if (data->target_mode != true) {
-		LOG_ERR("device are not configured as target device\n");


Why LOG_ERR is removed

These have been restored.

fabin-mchp · 2026-01-28T04:06:15Z

+
+	k_mutex_lock(&data->i2c_bus_mutex, K_FOREVER);
+
+	if (!data->target_mode || (data->target_cfg_count == 0U)) {


explicit check for data->target_mode

sunil-abraham · 2026-01-29T05:21:47Z

Could you please amend the changes and force-push to the same commit?
We don’t need to keep the review rework as a separate commit, since Zephyr history only needs the feature-addition commit.

fabin-mchp · 2026-01-29T12:51:51Z

Hi @sashapapo-mchp ,

I suggest adding multi-address target support as a separate feature. You can create a config option in Kconfig.mchp and use a macro to control the logic, so the current g1 driver remains unaffected. This way, users can easily enable or disable the feature as needed. For reference, you can check how I2C_TARGET_BUFFER_MODE is implemented.

Thanks
Fabin

sashapapo-mchp · 2026-01-29T14:30:13Z

adding multi-address target support as a separate feature. You can create a config option in Kconfig.mchp and use a macro to control the logic, so the current g1 driver remains unaffected. This way, users can easily enable or disable the feature as needed. For reference, you can check how I2C_TARGET_BUFFER_MODE is implemented.

The current driver will be heavily affected regardless. This is one of several patches that will come. I can add the KConfig, and will. But the driver will be affected.

sashapapo-mchp · 2026-01-29T14:38:46Z

Could you please amend the changes and force-push to the same commit? We don’t need to keep the review rework as a separate commit, since Zephyr history only needs the feature-addition commit.

Done. It is one commit now. KConfig added.

fabin-mchp

Please make sure the existing driver is not affected with your changes for this multi address support feature

fabin-mchp · 2026-02-04T12:48:25Z

-		/* Load the first byte for host read */
-		target_cb->read_requested(&data->target_config, &data->rx_tx_data);
+		/* Host read (target transmits) */
+		if (target_cb->read_requested) {


Explicit check should be added like this as per our coding guide lines
if (target_cb->read_requested != NULL)

fabin-mchp · 2026-02-04T12:50:02Z

-
-		/* Host writing */
-		target_cb->write_requested(&data->target_config);
+		/* Host write (target receives) */


Change to if (target_cb->write_requested != NULL)

fabin-mchp · 2026-02-04T12:51:37Z

 				/* Request a new buffer of data to send */
-				retval = target_cb->buf_read_requested(
-					&data->target_config, &data->tx_buf_ptr, &data->tx_len);
+				if (target_cb->buf_read_requested) {


same as previous comment if (target_cb->buf_read_requested != NULL)

fabin-mchp · 2026-02-04T12:53:36Z

+	const struct i2c_target_callbacks *target_cb =
+		(active_cfg != NULL) ? active_cfg->callbacks : NULL;
+	int retval = I2C_MCHP_SUCCESS;
+	uint8_t last_ack_state = i2c_target_get_lastbyte_ack_status(dev);


Why moved from if condition it was already checked in
if ((data->firstReadAfterAddrMatch == true) || (i2c_target_get_lastbyte_ack_status(dev) == I2C_MCHP_TARGET_ACK_STATUS_RECEIVED_ACK)) {

Updated to previous way of checking.

fabin-mchp · 2026-02-04T12:55:47Z

+		if (target_cb->read_requested) {
+			target_cb->read_requested(data->active_target_cfg, &data->rx_tx_data);
+		}
+		i2c_byte_write(dev, data->rx_tx_data);


Why i2c_byte_write(dev, data->rx_tx_data); is added here. Can you clarify?. It was already done inside i2c_target_data_ready()

You are correct, good find. This has been fixed.

fabin-mchp · 2026-02-04T13:06:18Z

+		return false;
+	}
+
+	return (cfg->address & 0x3FFU) == normalized_addr;


can we add a bracket return ((cfg->address & 0x3FFU) == normalized_addr);

fabin-mchp · 2026-02-04T13:06:58Z

+			continue;
+		}
+
+		if (i2c_mchp_target_matches_addr(data->target_cfgs[i], addr)) {


explicit true check

fabin-mchp · 2026-02-04T13:11:14Z

 	k_mutex_lock(&data->i2c_bus_mutex, K_FOREVER);
-	data->target_config.address = target_cfg->address;
-	data->target_callbacks.write_requested = target_cfg->callbacks->write_requested;
-	data->target_callbacks.write_received = target_cfg->callbacks->write_received;


We can't blindly remove the callbacks from here as it affects the already existing driver. The tests added for those will fail. So this can't be removed.

fabin-mchp · 2026-02-04T13:12:38Z

+		i2c_target_int_flag_clear(dev, SERCOM_I2CS_INTFLAG_Msk);
+		i2c_target_status_clear(dev, SERCOM_I2CS_STATUS_Msk);
+	} else {
+		i2c_target_enable(dev, false);


Will this affects the existing driver implementation

fabin-mchp · 2026-02-04T13:13:23Z

+			}
+		}
+
+		if (retval != 0) {


Avoid using do while() loops

fabin-mchp · 2026-02-04T13:18:51Z

adding multi-address target support as a separate feature. You can create a config option in Kconfig.mchp and use a macro to control the logic, so the current g1 driver remains unaffected. This way, users can easily enable or disable the feature as needed. For reference, you can check how I2C_TARGET_BUFFER_MODE is implemented.

The current driver will be heavily affected regardless. This is one of several patches that will come. I can add the KConfig, and will. But the driver will be affected.

The addition of this feature should not cause any failures in the existing test cases that have already passed.

sashapapo-mchp · 2026-02-04T17:10:48Z

adding multi-address target support as a separate feature. You can create a config option in Kconfig.mchp and use a macro to control the logic, so the current g1 driver remains unaffected. This way, users can easily enable or disable the feature as needed. For reference, you can check how I2C_TARGET_BUFFER_MODE is implemented.

The current driver will be heavily affected regardless. This is one of several patches that will come. I can add the KConfig, and will. But the driver will be affected.

The addition of this feature should not cause any failures in the existing test cases that have already passed.

I've ran both of the suggested tests and there are no issues, both i2c async and target_api pass as expected.

For async I used the onboard AT24MAC402 on my board.
For target_api I used an external AARDVARK as the master to do reads/writes.

sunil-abraham · 2026-02-18T12:31:37Z

+			}
+
+			if ((slot < 0) && (data->target_cfgs[i] == NULL)) {
+				slot = i;


Why not exit the loop, if slot is allocated?

This loop serves two purposes. It finds a free slot and then also detects if the particular target_cfg is also registered. If we add a break here, it would just find the first free slot and break, potentially registering the same target_cfg twice. A break here will not work without restructuring this and breaking it into two loops.

Another implemenation option is, not to use array of target_cfg. Instead You can use active_target_cfg and alternate_target_cfg pointers. It can be swapped or assigned NULL as required. (Assuming, devices supporting more than 2 client addresses may be using another i2c gx driver version.)

if((active_target_cfg == target_cfg) || (alternate_target_cfg == target_cfg)) { -EALREADY;} else if(active_target_cfg == NULL) { .. } else if(alternate_target_cfg == NULL) { .. } else { -EBUSY; }

sunil-abraham · 2026-02-18T12:32:30Z

+
+	/* Prefer slot 0 if both are empty */
+	if ((retval == 0) && (slot == 1) && (data->target_cfgs[0] == NULL)) {
+		slot = 0;


Better to have simpler logic, instead of resetting the slot again to 0.

Agreed, I think this can be removed based on the previous checks. It might be dead code. I will correct.

sunil-abraham · 2026-02-18T12:36:23Z

+			i2c_target_status_clear(dev, SERCOM_I2CS_STATUS_Msk);
+		}
+
+		data->target_cfgs[slot] = target_cfg;


You are saving only the pointer to the structure, that user has provided.
target_cfg may be pointing to a structure in user stack.

Not storing the target config within the driver is a standard pattern. Having the client responsible for the memory allocation and storage of the target configs is a pattern that other vendors such as st follow.

See here:
https://github.com/zephyrproject-rtos/zephyr/blob/a43e516ff2f7bc7091835bf9a97c979937a75366/drivers/i2c/i2c_ll_stm32_v2.c#L207

sunil-abraham · 2026-02-18T12:36:56Z

+	uint8_t target_cfg_count;
+#else
+	/* Single target configuration (legacy behavior). */
+	struct i2c_target_config *target_cfg;


Where is the memory to store target config structure elements?

We do not need to store the target config, the client will allocate memory for this. Same comment as above.

sunil-abraham · 2026-02-18T12:38:19Z

 #ifdef CONFIG_I2C_TARGET
-	struct i2c_target_config target_config;
-	struct i2c_target_callbacks target_callbacks;
+#ifdef CONFIG_I2C_MCHP_TARGET_MULTI_ADDR


Both cases can be combined (single address and multi address)

I2C_MCHP_TARGET_CFG_SLOTS = 1

I2C_MCHP_TARGET_CFG_SLOTS = 2

sunil-abraham · 2026-02-18T12:38:39Z

-	struct i2c_target_callbacks target_callbacks;
+#ifdef CONFIG_I2C_MCHP_TARGET_MULTI_ADDR
+	/* Registered target configs (hardware supports up to two addresses). */
+	struct i2c_target_config *target_cfgs[I2C_MCHP_TARGET_CFG_SLOTS];


Get I2C_MCHP_TARGET_CFG_SLOTS, from device tree.

The G1 i2c peripheral can only support 2 I2C addresses as a target. Having it in the device tree would allow a user to set it to whatever they want and it would cause confusion during compile time.

Ideally, we wouldn't even have a config or define. You should simply be able to call i2c_mchp_target_register twice and it would work. This is the pattern that other vendors follow, there is no configuration in the device tree or in the config.

sunil-abraham · 2026-02-19T11:12:31Z

 #include <zephyr/irq.h>
 #include <zephyr/logging/log.h>
 #include <zephyr/drivers/i2c.h>
 #include <zephyr/drivers/dma.h>


Please use latest code.
dma related headers are included with
#ifdef CONFIG_I2C_MCHP_DMA_DRIVEN

sunil-abraham · 2026-02-20T04:08:30Z

+	ten_bit = ((target_cfg->flags & I2C_TARGET_FLAGS_ADDR_10_BITS) != 0U);
+	addr_limit = ten_bit ? 0x3FFU : 0x7FU;
+
+	if (target_cfg->address > addr_limit) {


This if condition can be combined with
if (target_cfg->address == I2C_INVALID_ADDR)

sunil-abraham · 2026-02-20T04:14:50Z

+		retval = -EBUSY;
+	}
+
+	if (retval == 0) {


Could avoid checking retval == 0, multiple times, by using
k_mutex_lock()
do {
logic .....
if failure, break;
logic .....
} while(0)
k_mutex_unlock()

sunil-abraham · 2026-02-20T05:00:31Z

+			}
+
+			if ((slot < 0) && (data->target_cfgs[i] == NULL)) {
+				slot = i;


Another implemenation option is, not to use array of target_cfg. Instead You can use active_target_cfg and alternate_target_cfg pointers. It can be swapped or assigned NULL as required. (Assuming, devices supporting more than 2 client addresses may be using another i2c gx driver version.)

if((active_target_cfg == target_cfg) || (alternate_target_cfg == target_cfg)) { -EALREADY;} else if(active_target_cfg == NULL) { .. } else if(alternate_target_cfg == NULL) { .. } else { -EBUSY; }

Add support for registering up to two I2C target addresses on a single SERCOM instance, leveraging the hardware's dual-address capability via the AMODE field. Changes: - Replace single target_config/target_callbacks with target_cfgs[2] pointer array to support multiple registered configurations - Add target_cfg_count and active_target_cfg tracking - Add helper functions for address matching and configuration lookup - Update i2c_target_register() for slot-based registration with validation of 7-bit/10-bit address constraints - Update i2c_target_unregister() with slot compaction when removing the primary address - Update interrupt handlers to dispatch to the correct configuration based on the matched address The secondary address uses the ADDRMASK register with AMODE set to 2_ADDRESSES mode. Both addresses must be 7-bit when using dual-address mode per hardware requirements. Signed-off-by: Sasha Papo <sasha.papo@microchip.com> Made-with: Cursor

sashapapo-mchp force-pushed the i2c-target-multiaddr branch from b1d5bde to a9c874d Compare January 19, 2026 18:52

NhMchp self-assigned this Jan 21, 2026

NhMchp requested review from AzharMCHP and fabin-mchp January 21, 2026 17:51

fabin-mchp requested changes Jan 28, 2026

View reviewed changes

NhMchp requested a review from sunil-abraham January 28, 2026 08:24

sashapapo-mchp force-pushed the i2c-target-multiaddr branch from 4b93830 to 69c186e Compare January 29, 2026 14:38

fabin-mchp requested changes Feb 4, 2026

View reviewed changes

sashapapo-mchp force-pushed the i2c-target-multiaddr branch from 69c186e to e6698ca Compare February 4, 2026 17:08

sunil-abraham requested changes Feb 18, 2026

View reviewed changes

sunil-abraham requested changes Feb 20, 2026

View reviewed changes

sashapapo-mchp force-pushed the i2c-target-multiaddr branch from e6698ca to 7878a7e Compare February 27, 2026 16:31


		k_mutex_lock(&data->i2c_bus_mutex, K_FOREVER);

		if (!data->target_mode \|\| (data->target_cfg_count == 0U)) {

Uh oh!

Conversation

sashapapo-mchp commented Jan 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sashapapo-mchp Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunil-abraham commented Jan 29, 2026

Uh oh!

fabin-mchp commented Jan 29, 2026

Uh oh!

sashapapo-mchp commented Jan 29, 2026

Uh oh!

sashapapo-mchp commented Jan 29, 2026

Uh oh!

fabin-mchp 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sashapapo-mchp Jan 28, 2026 •

edited

Loading

fabin-mchp left a comment •

edited

Loading