Skip to content
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

Add TCPC driver for RT1715 #81109

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

recalci
Copy link
Contributor

@recalci recalci commented Nov 7, 2024

Add TCPCI functions and add driver for Richtek RT1715.

@recalci recalci force-pushed the usbc_tcpc_rt1715 branch 4 times, most recently from 84315d9 to 4cd7a1c Compare November 10, 2024 22:29
@recalci recalci marked this pull request as draft December 5, 2024 18:17
@recalci recalci marked this pull request as ready for review December 10, 2024 15:21
@recalci recalci force-pushed the usbc_tcpc_rt1715 branch 2 times, most recently from 8bacd7b to 5d3b6b4 Compare December 10, 2024 15:49
drivers/usb_c/tcpc/tcpci.c Show resolved Hide resolved
drivers/usb_c/tcpc/rt1715.c Outdated Show resolved Hide resolved
drivers/usb_c/tcpc/rt1715.c Outdated Show resolved Hide resolved
drivers/usb_c/tcpc/rt1715.c Outdated Show resolved Hide resolved
drivers/usb_c/tcpc/rt1715.c Outdated Show resolved Hide resolved
drivers/usb_c/tcpc/rt1715.c Outdated Show resolved Hide resolved
drivers/usb_c/tcpc/rt1715.c Outdated Show resolved Hide resolved
drivers/usb_c/tcpc/rt1715.c Outdated Show resolved Hide resolved

if (data->rx_sop_prime_enable) {
return tcpci_tcpm_set_rx_type(&cfg->bus,
TCPC_REG_RX_DETECT_SOP_SOPP_SOPPP_HRST_MASK);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you enabling both SOP' and SOP'' messages here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that both Numaker and STM32 enable them as well. However, I couldn't find the datasheet for the PS8xxx series, so I suspect that it may not support SOP'' messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

The PS8xxx does support SOP' and SOP''. However, the API documentation for tcpc_sop_prime_enable() indicates this command is used to enable SOP' messages, with no mention of SOP''.

The ChromeOS code that USB-C support is based on does enable SOP' and SOP'' for this command.

Do you mind updating the documentation for tcpc_sop_prime_enable() to note that this enables both SOP' and SOP''?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Please review.

@recalci recalci force-pushed the usbc_tcpc_rt1715 branch 2 times, most recently from 55c7eb8 to a8bb603 Compare December 14, 2024 12:54

if (data->rx_sop_prime_enable) {
return tcpci_tcpm_set_rx_type(&cfg->bus,
TCPC_REG_RX_DETECT_SOP_SOPP_SOPPP_HRST_MASK);
Copy link
Contributor

Choose a reason for hiding this comment

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

The PS8xxx does support SOP' and SOP''. However, the API documentation for tcpc_sop_prime_enable() indicates this command is used to enable SOP' messages, with no mention of SOP''.

The ChromeOS code that USB-C support is based on does enable SOP' and SOP'' for this command.

Do you mind updating the documentation for tcpc_sop_prime_enable() to note that this enables both SOP' and SOP''?

struct pd_msg *msg = &data->rx_msg;
int ret = 0;

buf[0].buf = TCPC_REG_RX_BUFFER;
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't correct. You're writing the TCPC register number into a pointer.

Suggested change
buf[0].buf = TCPC_REG_RX_BUFFER;
uint8_t tcpc_reg = TCPC_REG_RX_BUFFER;
buf[0].buf = *tcpc_reg;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my fault. fixed

drivers/usb_c/tcpc/ps8xxx.c Outdated Show resolved Hide resolved
Copy link
Contributor

@keith-zephyr keith-zephyr left a comment

Choose a reason for hiding this comment

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

Looking good. One last ask. Please add a build test that links in this driver.

See #83187 for an example.

Locally, you can run ./scripts/twister -ivc -s drivers.usb.build.i2c to verify the test.

@zephyrbot zephyrbot added the area: USB Universal Serial Bus label Dec 20, 2024
@recalci
Copy link
Contributor Author

recalci commented Dec 20, 2024

Looking good. One last ask. Please add a build test that links in this driver.

See #83187 for an example.

Locally, you can run ./scripts/twister -ivc -s drivers.usb.build.i2c to verify the test.

Thanks for your review.
The commit has been added. In order to ensure that everything works well together, this PR contains #83187 now. If #83187 is merged first, I will rebase and update this PR.

@keith-zephyr
Copy link
Contributor

Looking good. One last ask. Please add a build test that links in this driver.
See #83187 for an example.
Locally, you can run ./scripts/twister -ivc -s drivers.usb.build.i2c to verify the test.

Thanks for your review. The commit has been added. In order to ensure that everything works well together, this PR contains #83187 now. If #83187 is merged first, I will rebase and update this PR.

#83187 has now merged.

Add generic functions that will be common to all TCPCI compliant drivers.

Signed-off-by: Jianxiong Gu <jianxiong.gu@outlook.com>
Add support for RT1715.

Signed-off-by: Jianxiong Gu <jianxiong.gu@outlook.com>
This commit modifies the ps8xxx.c driver to use the generic TCPCI function.

Signed-off-by: Jianxiong Gu <jianxiong.gu@outlook.com>
Clearly mention that this function enables both SOP' and SOP'' messages.

Signed-off-by: Jianxiong Gu <jianxiong.gu@outlook.com>
Add a build test to verify richtek,rt1715 driver builds correctly.

Signed-off-by: Jianxiong Gu <jianxiong.gu@outlook.com>
@recalci
Copy link
Contributor Author

recalci commented Dec 21, 2024

Looking good. One last ask. Please add a build test that links in this driver.
See #83187 for an example.
Locally, you can run ./scripts/twister -ivc -s drivers.usb.build.i2c to verify the test.

Thanks for your review. The commit has been added. In order to ensure that everything works well together, this PR contains #83187 now. If #83187 is merged first, I will rebase and update this PR.

#83187 has now merged.

Just updated. Local tests passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: USB Universal Serial Bus area: USB-C
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants