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

draft driver for STUSB4500 #14895

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SimonFilgis
Copy link
Contributor

@SimonFilgis SimonFilgis commented Nov 21, 2024

Right now it is only possible to change the delivered power on the fly by

  1. selecting PDO2 slot
  2. write to POO2
  3. renegotiate power by sw reset command

Note: Please adhere to Contributing Guidelines.

Summary

Create a basic driver to negotiate power delivery with walladapter using STM STUSB4500 chip.
This is a standalone driver that does not change any other code. It may be selected, or not via kconfig.

Impact

New feature. No impact to any existing user or application expected.

Testing

Tested on custom board.

@github-actions github-actions bot added Area: USB Size: L The size of the change in this PR is large labels Nov 21, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 21, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and testing information, it lacks crucial details. Here's a breakdown of what's missing:

  • Summary: Needs to be more detailed.

    • Why is this change necessary? Just saying "Create an ioctl driver..." isn't enough. Explain the problem the driver solves. Why is the current method (PDO2/POO2/reset) insufficient? What benefits does the ioctl driver provide?
    • What functional part of the code is being changed? Be specific. Which subsystem is affected? Are new files being added? Are existing files being modified? Where is this ioctl driver located in the codebase?
    • How does the change exactly work? Explain the mechanism of the ioctl driver. What ioctl commands are being introduced? How does it interact with the hardware? How does it differ from the existing power negotiation method?
    • Related Issues: Are there any related issues in the NuttX or NuttX Apps repositories? If not, consider creating one to track the feature request/bug this PR addresses.
  • Impact: "None expected" is almost never true. Think carefully about potential impacts.

    • New feature? Yes, explicitly state this.
    • Impact on the user: Will the user need to change their application code to use the new ioctl driver? If so, how? Provide examples.
    • Impact on build: Does the build system need to be modified to include the new driver? Are any new Kconfig options added?
    • Impact on hardware: Which architectures and boards are supported? Are any hardware-specific modifications required?
    • Impact on documentation: At a minimum, the new driver needs to be documented. State this explicitly. Where will the documentation be added?
    • Impact on security: Consider potential security implications. Could the ioctl driver be misused?
    • Impact on compatibility: Does this change break any existing functionality?
  • Testing: Insufficient detail.

    • Build Host(s): Provide specific information: OS version, compiler version (e.g., "Linux Ubuntu 20.04, GCC 9.4.0").
    • Target(s): Be specific: Architecture, board, configuration (e.g., "sim:qemu-arm", "stm32f4discovery:default").
    • Testing logs: "Tested on custom board" is meaningless without actual logs. Show relevant logs demonstrating the functionality before and after the change. Include commands used, output, and evidence that the power delivery negotiation is working as expected. Demonstrate the benefits over the previous method.

By addressing these points, the PR will be much more likely to be accepted. Remember to be clear, concise, and provide as much detail as possible.

@SimonFilgis SimonFilgis force-pushed the add_stusb4500_driver branch 2 times, most recently from 90c9862 to e035f45 Compare November 21, 2024 16:55
default n
select I2C
---help---
Enable device driver for ST standalone USB PD sink controller
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Enable device driver for ST standalone USB PD sink controller
Enable device driver for ST standalone USB PD (power delivery) sink controller that addresses sink up to 100 W (20 V; 5 A).

ret = I2C_TRANSFER(priv->i2c, msg, 2);
if (ret >= 0)
{
#define BUFFER_SIZE 128
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this macro to the right section of this file (top) or to stusb4500.h, also use STUSB4500_BUFFER_SIZE to avoid name collision

uint8_t battery : 2;
} bat;
}
USB_PD_SNK_PDO_TYPEDEF __attribute__((__packed__));
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to use packed attribute from compiler.h

right now it is only possible to change the delivered power on the fly
by
1. selecting PDO2 slot
2. write to POO2
3. renegotiate power by sw reset command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: USB Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants