-
Notifications
You must be signed in to change notification settings - Fork 8.4k
drivers: usb: udc: stm32: move power configuration to common code #100519
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
base: main
Are you sure you want to change the base?
drivers: usb: udc: stm32: move power configuration to common code #100519
Conversation
c183a92 to
24ca408
Compare
Create infrastructure for shared USB common code on STM32 family, and move the Power Controller configuration logic to common code. Signed-off-by: Mathieu Choplain <[email protected]>
24ca408 to
51c1f86
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create infrastructure for shared USB common code on STM32 family, and move
the Power Controller configuration logic to common code.
But it is not shared. You just moved the code from UDC driver. It is not worth it to touch legacy driver at this point. What is the reason to move the code?
If there is stm32_usb_pwr_enable(void), then you need also stm32_usb_pwr_disable(). stm32_usb_pwr.c actually looks like regulator driver. Why not use regulator API and implement a regulator driver? What you would need to implement is just
static DEVICE_API(regulator, api) = {
.enable = stm32_usbreg_enable,
.disable = stm32_usbreg_disable,
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it is not shared.
It is not shared for now, but would become shared when/if an STM32 UHC driver is implemented.
It is not worth it to touch legacy driver
What do you mean by legacy driver? This is the mainline UDC driver, usb_dc_stm32.c is left untouched.
If there is stm32_usb_pwr_enable(void), then you need also stm32_usb_pwr_disable().
I did not implement disable() because it requires refcounting, but...
stm32_usb_pwr.c actually looks like regulator driver.
...good remark - and this solves refcounting issues. I'll try implementing as a regulator driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a quick and dirty implementation, I don't think a regulator driver will be an acceptable solution. Turning this code into a regulator driver (and thus enabling the regulator subsystem!) adds an unjustified ~1K to the ROM footprint (on our smallest product with USB support, that's 2% of ROM size!)
Would you be OK if the code was in soc/st/common instead of drivers/usb/common?
(note that next PR in patchlist will place things in drivers/usb/common either way - unless that directory is pure legacy, in which case what's there should be migrated to a Nordic SoC-specific folder 🙂)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not shared for now, but would become shared when/if an STM32 UHC driver is implemented.
Just FYI, UHC shim driver for DWC2 controller would not be acceptable.
It is not worth it to touch legacy driver
What do you mean by
legacy driver? This is the mainline UDC driver,usb_dc_stm32.cis left untouched.
From the commit message it is not clear where it would be shared, so I have to guess.
If there is stm32_usb_pwr_enable(void), then you need also stm32_usb_pwr_disable().
I did not implement
disable()because it requires refcounting, but...
But why move still "not yet" shared code and keep it half-backed?
After a quick and dirty implementation, I don't think a regulator driver will be an acceptable solution. Turning this code into a regulator driver (and thus enabling the regulator subsystem!) adds an unjustified ~1K to the ROM footprint (on our smallest product with USB support, that's 2% of ROM size!)
AFAIK there is no regulator subsystem and the increased footprint sounds acceptable to me, we should stimulate the market, but I do not insist on it. Please add stm32_usb_pwr_disable().



Create infrastructure for shared USB common code on STM32 family, and move the Power Controller configuration logic to common code.
The power configuration is left unchanged for now, but it can be further cleaned up and improved in the future.