drivers: i2c: i2c_dw: adding optional feature support and improvements

Handling invalid 0 length i2c transfer and logging error
if i2c transfer is initiated with length 0.
Modified i2c shell to support i2c scan command only if
I2C_ZERO_LEN_XFER_SUPPORTED Kconfig flag is enabled.
Updated i2c shell for checking i2c-speed parameter in
i2c speed shell command.

Signed-off-by: samit singh sikarwar <[email protected]>

Author: samitsingh7

drivers/i2c/Kconfig

@@ -22,6 +22,12 @@ config I2C_SHELL
 	  The I2C shell supports scanning, bus recovery, I2C read and write
 	  operations.
 
+config I2C_ZERO_LEN_XFER_SUPPORTED
+	bool "I2C transfer without data (data length 0)"
+	default y
+	help
+	  Enable this flag to support 0 length I2C transfers(quick commands).
+
 config I2C_STATS
 	bool "I2C device Stats"
 	depends on STATS

drivers/i2c/i2c_dw.c

@@ -1,8 +1,8 @@
-/* dw_i2c.c - I2C file for Design Ware */
+/* i2c_dw.c - I2C file for Design Ware */
 
 /*
- * Copyright (c) 2015 Intel Corporation
  * Copyright (c) 2022 Andrei-Edward Popa
+ * Copyright (c) 2015-2023 Intel Corporation
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -351,7 +351,7 @@ static inline void i2c_dw_transfer_complete(const struct device *dev)
 }
 
 #ifdef CONFIG_I2C_TARGET
-static inline uint8_t i2c_dw_read_byte_non_blocking(const struct device *dev);
+static inline int i2c_dw_read_byte_non_blocking(const struct device *dev, uint8_t *data);
 static inline void i2c_dw_write_byte_non_blocking(const struct device *dev, uint8_t data);
 static void i2c_dw_slave_read_clear_intr_bits(const struct device *dev);
 #endif
@@ -364,8 +364,8 @@ static void i2c_dw_isr(const struct device *port)
 	int ret = 0;
 	uint32_t reg_base = get_regs(port);
 
-	/* Cache ic_intr_stat for processing, so there is no need to read
-	 * the register multiple times.
+	/* Cache I2C Interrupt Status Register(ic_intr_stat) for processing,
+	 * so there is no need to read the register multiple times.
 	 */
 	intr_stat.raw = read_intr_stat(reg_base);
 
@@ -512,7 +512,7 @@ static int i2c_dw_setup(const struct device *dev, uint16_t slave_address)
 		 * Make sure to set both the master_mode and slave_disable_bit
 		 * to both 0 or both 1
 		 */
-		LOG_DBG("I2C: host configured as Master Device");
+		LOG_DBG("I2C: host configured as Controller Device");
 		ic_con.bits.master_mode = 1U;
 		ic_con.bits.slave_disable = 1U;
 	} else {
@@ -619,7 +619,7 @@ static int i2c_dw_transfer(const struct device *dev, struct i2c_msg *msgs, uint8
 {
 	struct i2c_dw_dev_config *const dw = dev->data;
 	struct i2c_msg *cur_msg = msgs;
-	uint8_t msg_left = num_msgs;
+	uint8_t msg_left;
 	uint8_t pflags;
 	int ret;
 	uint32_t reg_base = get_regs(dev);
@@ -635,6 +635,21 @@ static int i2c_dw_transfer(const struct device *dev, struct i2c_msg *msgs, uint8
 		return ret;
 	}
 
+	/*
+	 * I2C transfer with 0 length are invalid but
+	 * SMbus quick commands uses 0 length transfer.
+	 * And in order to support SMbus quick commands, the driver
+	 * has to be enhanced.
+	 */
+	for (msg_left = 0; msg_left < num_msgs; msg_left++) {
+		if (msgs[msg_left].len == 0) {
+			LOG_ERR("Invalid transfer length 0 for msgs[%d]", msg_left);
+			return -EINVAL;
+		}
+	}
+
+	msg_left = num_msgs;
+
 	/* First step, check if there is current activity */
 	if (test_bit_status_activity(reg_base) || (dw->state & I2C_DW_BUSY)) {
 		ret = -EBUSY;
@@ -741,14 +756,13 @@ static int i2c_dw_runtime_configure(const struct device *dev, uint32_t config)
 	struct i2c_dw_dev_config *const dw = dev->data;
 	const struct i2c_dw_rom_config *const rom = dev->config;
 	uint32_t value = 0U;
-	uint32_t rc = 0U;
+	uint32_t ret = 0U;
 	uint32_t reg_base = get_regs(dev);
 
-	dw->app_config = config;
 
 	/* Make sure we have a supported speed for the DesignWare model */
 	/* and have setup the clock frequency and speed mode */
-	switch (I2C_SPEED_GET(dw->app_config)) {
+	switch (I2C_SPEED_GET(config)) {
 	case I2C_SPEED_STANDARD:
 		/* Following the directions on DW spec page 59, IC_SS_SCL_LCNT
 		 * must have register values larger than IC_FS_SPKLEN + 7
@@ -832,14 +846,17 @@ static int i2c_dw_runtime_configure(const struct device *dev, uint32_t config)
 
 			dw->hcnt = value;
 		} else {
-			rc = -EINVAL;
+			ret = -EINVAL;
 		}
 		break;
 	default:
 		/* TODO change */
-		rc = -EINVAL;
+		ret = -EINVAL;
 	}
 
+	if (ret == 0) {
+		dw->app_config = config;
+	}
 	/*
 	 * Clear any interrupts currently waiting in the controller
 	 */
@@ -852,26 +869,30 @@ static int i2c_dw_runtime_configure(const struct device *dev, uint32_t config)
 	 */
 	dw->app_config |= I2C_MODE_CONTROLLER;
 
-	return rc;
+	return ret;
 }
 
 #ifdef CONFIG_I2C_TARGET
-static inline uint8_t i2c_dw_read_byte_non_blocking(const struct device *dev)
+static inline int i2c_dw_read_byte_non_blocking(const struct device *dev, uint8_t *data)
 {
 	uint32_t reg_base = get_regs(dev);
 
 	if (!test_bit_status_rfne(reg_base)) { /* Rx FIFO must not be empty */
+		LOG_ERR("Rx FIFO is empty");
 		return -EIO;
 	}
 
-	return (uint8_t)read_cmd_data(reg_base);
+	*data = (uint8_t)read_cmd_data(reg_base);
+
+	return 0;
 }
 
 static inline void i2c_dw_write_byte_non_blocking(const struct device *dev, uint8_t data)
 {
 	uint32_t reg_base = get_regs(dev);
 
 	if (!test_bit_status_tfnt(reg_base)) { /* Tx FIFO must not be full */
+		LOG_ERR("Tx FIFO is full");
 		return;
 	}
 
@@ -925,7 +946,7 @@ static int i2c_dw_set_slave_mode(const struct device *dev, uint8_t addr)
 	write_tx_tl(0, reg_base);
 	write_rx_tl(0, reg_base);
 
-	LOG_DBG("I2C: Host registered as Slave Device");
+	LOG_DBG("I2C: Host registered as Target Device");
 
 	return 0;
 }
@@ -947,13 +968,11 @@ static int i2c_dw_slave_register(const struct device *dev, struct i2c_target_con
 
 static int i2c_dw_slave_unregister(const struct device *dev, struct i2c_target_config *cfg)
 {
-	struct i2c_dw_dev_config *const dw = dev->data;
-	int ret;
+	struct i2c_dw_dev_config * const dw = dev->data;
 
 	dw->state = I2C_DW_STATE_READY;
-	ret = i2c_dw_set_master_mode(dev);
 
-	return ret;
+	return (int)i2c_dw_set_master_mode(dev);
 }
 
 static void i2c_dw_slave_read_clear_intr_bits(const struct device *dev)

drivers/i2c/i2c_dw.h

@@ -1,7 +1,7 @@
-/* dw_i2c.h - header for Design Ware I2C operations */
+/* i2c_dw.h - header for Design Ware I2C operations */
 
 /*
- * Copyright (c) 2015 Intel Corporation
+ * Copyright (c) 2015-2023 Intel Corporation
  *
  * SPDX-License-Identifier: Apache-2.0
  */

drivers/i2c/i2c_shell.c

@@ -33,6 +33,7 @@ static int get_bytes_count_for_hex(char *arg)
 	return MIN(MAX_BYTES_FOR_REGISTER_INDEX, length);
 }
 
+#if CONFIG_I2C_ZERO_LEN_XFER_SUPPORTED
 /*
  * This sends I2C messages without any data (i.e. stop condition after
  * sending just the address). If there is an ACK for the address, it
@@ -94,6 +95,7 @@ static int cmd_i2c_scan(const struct shell *shell_ctx,
 
 	return 0;
 }
+#endif
 
 /* i2c recover <device> */
 static int cmd_i2c_recover(const struct shell *shell_ctx,
@@ -318,6 +320,12 @@ static int cmd_i2c_speed(const struct shell *shell_ctx, size_t argc, char **argv
 	}
 
 	speed = strtol(argv[ARGV_DEV + 1], NULL, 10);
+
+	if (speed > I2C_SPEED_DT) {
+		shell_error(shell_ctx, "Invalid speed parameter");
+		return -EINVAL;
+	}
+
 	ret = i2c_get_config(dev, &dev_config);
 	if (ret == 0) {
 		dev_config &= ~I2C_SPEED_MASK;
@@ -349,10 +357,12 @@ static void device_name_get(size_t idx, struct shell_static_entry *entry)
 SHELL_DYNAMIC_CMD_CREATE(dsub_device_name, device_name_get);
 
 SHELL_STATIC_SUBCMD_SET_CREATE(sub_i2c_cmds,
+#if CONFIG_I2C_ZERO_LEN_XFER_SUPPORTED
 	SHELL_CMD_ARG(scan, &dsub_device_name,
 		      "Scan I2C devices\n"
 		      "Usage: scan <device>",
 		      cmd_i2c_scan, 2, 0),
+#endif
 	SHELL_CMD_ARG(recover, &dsub_device_name,
 		      "Recover I2C bus\n"
 		      "Usage: recover <device>",

dts/bindings/i2c/snps,designware-i2c.yaml

@@ -5,7 +5,7 @@ description: Synopsys DesignWare I2C node
 
 compatible: "snps,designware-i2c"
 
-include: [i2c-controller.yaml, pinctrl-device.yaml, pcie-device.yaml]
+include: [i2c-controller.yaml, reset-device.yaml, pinctrl-device.yaml, pcie-device.yaml]
 
 properties:
   interrupts: