Skip to content

drivers: i2c: Add I2C driver of it51xxx #88292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 29, 2025

Conversation

GTLin08
Copy link
Collaborator

@GTLin08 GTLin08 commented Apr 8, 2025

Implement the functions of I2C host and target.
I2CM: supports nine hosts and each one able located at I2C interface 0-12.
supports two 32 bytes dedicated FIFO mode for read and write.

I2CS: supports three targets and each one able located at I2C interface 0-8.
supports 16 bytes dedicated FIFO mode that only supports write or read mode and the maximum buffer size is 256 bytes.
support non-FIFO write to shared FIFO read mode. The maximum shared FIFO size for read is 256 bytes.

The APIs test include: i2c_write(), i2c_read(), i2c_burst_read(), i2c_burst_write(), i2c_write_read()

@GTLin08
Copy link
Collaborator Author

GTLin08 commented Apr 22, 2025

Rebased.

teburd
teburd previously approved these changes Apr 22, 2025
Dino-Li
Dino-Li previously approved these changes Apr 23, 2025
@kartben kartben requested a review from Copilot April 23, 2025 07:42
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new I2C driver for the it51xxx SoC, implementing both host and target functionality with dedicated FIFO modes and expanded channel support.

  • Introduces additional hardware configuration in soc.c to disable debug hardware target detection.
  • Adds SMBus register definitions in chip_chipregs.h and updates related DTS bindings and header files for channel and interface configuration.
  • Updates board configuration to enable I2C support.

Reviewed Changes

Copilot reviewed 7 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
soc/ite/ec/it51xxx/soc.c Adds debug disable commands for hardware target detection.
soc/ite/ec/it51xxx/chip_chipregs.h Defines SMBus interface registers with new register offsets.
include/zephyr/dt-bindings/i2c/it51xxx-i2c.h Introduces new DT bindings constants for I2C interfaces.
include/zephyr/dt-bindings/i2c/i2c.h Adds additional SMBus channel definitions.
dts/bindings/i2c/ite,it51xxx-i2c.yaml Provides detailed configuration examples for the new driver.
boards/ite/it515xx_evb/it515xx_evb.yaml Updates the board configuration to include I2C support.
Files not reviewed (8)
  • boards/ite/it515xx_evb/it515xx_evb.dts: Language not supported
  • drivers/i2c/CMakeLists.txt: Language not supported
  • drivers/i2c/Kconfig: Language not supported
  • drivers/i2c/Kconfig.it51xxx: Language not supported
  • dts/riscv/ite/it51xxx-pinctrl-map.dtsi: Language not supported
  • dts/riscv/ite/it51xxx.dtsi: Language not supported
  • tests/drivers/i2c/i2c_target_api/boards/it515xx_evb.conf: Language not supported
  • tests/drivers/i2c/i2c_target_api/boards/it515xx_evb.overlay: Language not supported

Comment on lines 331 to 333
#define SMB_SADFPCTL IT51XXX_SMB_BASE + 0x0a
#define SMB_SBDFPCTL IT51XXX_SMB_BASE + 0x2a
#define SMB_SCDFPCTL IT51XXX_SMB_BASE + 0x4a
Copy link
Preview

Copilot AI Apr 23, 2025

Choose a reason for hiding this comment

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

Consider adding parentheses around the arithmetic expression to ensure proper evaluation (e.g., ((IT51XXX_SMB_BASE) + 0x0a)). This change applies to all similar macro definitions.

Suggested change
#define SMB_SADFPCTL IT51XXX_SMB_BASE + 0x0a
#define SMB_SBDFPCTL IT51XXX_SMB_BASE + 0x2a
#define SMB_SCDFPCTL IT51XXX_SMB_BASE + 0x4a
#define SMB_SADFPCTL (IT51XXX_SMB_BASE + 0x0a)
#define SMB_SBDFPCTL (IT51XXX_SMB_BASE + 0x2a)
#define SMB_SCDFPCTL (IT51XXX_SMB_BASE + 0x4a)

Copilot uses AI. Check for mistakes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is rule 20.7 of the coding guidelines and should be addressed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 84 to 96
1 = SMB_SWITCH_INTERFACE0: Swich to interface0 SMCLK0/SMDAT0
2 = SMB_SWITCH_INTERFACE1: Swich to interface1 SMCLK1/SMDAT1
3 = SMB_SWITCH_INTERFACE2: Swich to interface2 SMCLK2/SMDAT2
4 = SMB_SWITCH_INTERFACE3: Swich to interface3 SMCLK3/SMDAT3
5 = SMB_SWITCH_INTERFACE4: Swich to interface4 SMCLK4/SMDAT4
6 = SMB_SWITCH_INTERFACE5: Swich to interface5 SMCLK5/SMDAT5
7 = SMB_SWITCH_INTERFACE6: Swich to interface6 SMCLK6/SMDAT6
8 = SMB_SWITCH_INTERFACE7: Swich to interface7 SMCLK7/SMDAT7
9 = SMB_SWITCH_INTERFACE8: Swich to interface8 SMCLK8/SMDAT8
10 = SMB_SWITCH_INTERFACE9: Swich to interface9 SMCLK9/SMDAT9
11 = SMB_SWITCH_INTERFACE10: Swich to interface10 SMCLK10/SMDAT10
12 = SMB_SWITCH_INTERFACE11: Swich to interface11 SMCLK11/SMDAT11
13 = SMB_SWITCH_INTERFACE12: Swich to interface12 SMCLK12/SMDAT12
Copy link
Preview

Copilot AI Apr 23, 2025

Choose a reason for hiding this comment

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

There is a spelling mistake: 'Swich' should be corrected to 'Switch'.

Suggested change
1 = SMB_SWITCH_INTERFACE0: Swich to interface0 SMCLK0/SMDAT0
2 = SMB_SWITCH_INTERFACE1: Swich to interface1 SMCLK1/SMDAT1
3 = SMB_SWITCH_INTERFACE2: Swich to interface2 SMCLK2/SMDAT2
4 = SMB_SWITCH_INTERFACE3: Swich to interface3 SMCLK3/SMDAT3
5 = SMB_SWITCH_INTERFACE4: Swich to interface4 SMCLK4/SMDAT4
6 = SMB_SWITCH_INTERFACE5: Swich to interface5 SMCLK5/SMDAT5
7 = SMB_SWITCH_INTERFACE6: Swich to interface6 SMCLK6/SMDAT6
8 = SMB_SWITCH_INTERFACE7: Swich to interface7 SMCLK7/SMDAT7
9 = SMB_SWITCH_INTERFACE8: Swich to interface8 SMCLK8/SMDAT8
10 = SMB_SWITCH_INTERFACE9: Swich to interface9 SMCLK9/SMDAT9
11 = SMB_SWITCH_INTERFACE10: Swich to interface10 SMCLK10/SMDAT10
12 = SMB_SWITCH_INTERFACE11: Swich to interface11 SMCLK11/SMDAT11
13 = SMB_SWITCH_INTERFACE12: Swich to interface12 SMCLK12/SMDAT12
1 = SMB_SWITCH_INTERFACE0: Switch to interface0 SMCLK0/SMDAT0
2 = SMB_SWITCH_INTERFACE1: Switch to interface1 SMCLK1/SMDAT1
3 = SMB_SWITCH_INTERFACE2: Switch to interface2 SMCLK2/SMDAT2
4 = SMB_SWITCH_INTERFACE3: Switch to interface3 SMCLK3/SMDAT3
5 = SMB_SWITCH_INTERFACE4: Switch to interface4 SMCLK4/SMDAT4
6 = SMB_SWITCH_INTERFACE5: Switch to interface5 SMCLK5/SMDAT5
7 = SMB_SWITCH_INTERFACE6: Switch to interface6 SMCLK6/SMDAT6
8 = SMB_SWITCH_INTERFACE7: Switch to interface7 SMCLK7/SMDAT7
9 = SMB_SWITCH_INTERFACE8: Switch to interface8 SMCLK8/SMDAT8
10 = SMB_SWITCH_INTERFACE9: Switch to interface9 SMCLK9/SMDAT9
11 = SMB_SWITCH_INTERFACE10: Switch to interface10 SMCLK10/SMDAT10
12 = SMB_SWITCH_INTERFACE11: Switch to interface11 SMCLK11/SMDAT11
13 = SMB_SWITCH_INTERFACE12: Switch to interface12 SMCLK12/SMDAT12

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

Coding guidelines issue pointed out by Copilot should be addressed. Thanks!

@GTLin08 GTLin08 dismissed stale reviews from Dino-Li and teburd via 079acab April 23, 2025 08:06
@GTLin08 GTLin08 force-pushed the it51xxx_add_i2c_driver branch from d187210 to 079acab Compare April 23, 2025 08:06
@GTLin08
Copy link
Collaborator Author

GTLin08 commented Apr 24, 2025

Hi @teburd, @Dino-Li, @kartben,
I’ve made changes based on Copilot’s requests. Please revisit the updated PR. Thanks!

Dino-Li
Dino-Li previously approved these changes Apr 24, 2025
@kartben kartben dismissed their stale review April 25, 2025 10:01

feedback addressed - thx!

GTLin08 added 2 commits April 29, 2025 15:14
Implement the functions of I2C host and target.
I2CM: supports nine hosts and each one able located at I2C interface
      0~12.
      supports two 32 bytes dedicated FIFO mode for read and write.
I2CS: supports three targets and each one able located at I2C
      interface 0~8.
      supports 16 bytes dedicated FIFO mode that only supports write or
      read mode and the maximum buffer size is 256 bytes.
      support non-FIFO write to shared FIFO read mode. The maximum
      shared FIFO size for read is 256 bytes.
The APIs test include: i2c_write(), i2c_read(), i2c_burst_read(),
                       i2c_burst_write(), i2c_write_read()

Signed-off-by: Tim Lin <[email protected]>
Add I2C target node for it515xx_evb test.

Signed-off-by: Tim Lin <[email protected]>
@GTLin08
Copy link
Collaborator Author

GTLin08 commented Apr 29, 2025

Hi @Dino-Li ,

Rebased and fixed the issue as described below:

While working with I2C3, we discovered an issue in the previous PR where the reg property was incorrectly defined as

i2c3: i2c@f04178 {
     reg = <0x00f04178 0x0028>;
}

This should be:

i2c3: i2c@f04178 {
     reg = <0x00f04178 0x0028
            0 1>;
}

I've corrected this in the latest update.

@GTLin08
Copy link
Collaborator Author

GTLin08 commented Apr 29, 2025

Hi @teburd ,

Any comments would be greatly appreciated.

Copy link
Collaborator

@teburd teburd left a comment

Choose a reason for hiding this comment

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

lgtm, reapproving

@kartben kartben merged commit f91943a into zephyrproject-rtos:main Apr 29, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: I2C area: RISCV RISCV Architecture (32-bit & 64-bit) platform: ITE ITE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants