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 I2S Audio Driver (WDM) #15

Merged
merged 6 commits into from
Nov 28, 2023
Merged

Add I2S Audio Driver (WDM) #15

merged 6 commits into from
Nov 28, 2023

Conversation

coolstar
Copy link
Collaborator

@coolstar coolstar commented Nov 28, 2023

Add I2S Audio driver. Reads ACPI properties from the bus driver to enumerate audio endpoints.

Supports:

  1. I2S Output (Orange Pi 5 jack)
  2. I2S Input (Orange Pi 5 jack)

Supported, but untested:

  1. HDMI Audio (Requires VOP driver for this to work -- though the endpoint is enumerated provided ACPI is setup for it)

Notes:

  1. I2S Input is buggy and only works correctly 1 in 3 times. 2 out of the 3 times, the microphone input just gets static. But once it's working, it stays working reliably for at least 12+ hours

@coolstar
Copy link
Collaborator Author

Will do a followup PR to switch all the drivers over to READ_REGISTER_ULONG / WRITE_REGISTER_ULONG

@mariobalanica
Copy link
Member

mariobalanica commented Nov 28, 2023

  • The driver binary is not part of the artifact. Does it build to the same location as the other drivers?

  • Source\Utilities\pl330dma.h could be moved to drivers\include since it may be used by other drivers. It's also missing include guards.

  • Can you rename the device desc to "Rockchip I2S Controller" for consistency?

  • Not a fan of the multiple vcxprojs TBH, the files could be all included in the base project without changing the directory structure (which you said you care about).

Looks good to me otherwise. Great work!

@idigdoug
Copy link
Collaborator

idigdoug commented Nov 28, 2023

Will do a followup PR to switch all the drivers over to READ_REGISTER_ULONG / WRITE_REGISTER_ULONG

If you're concerned about performance, you can consider READ_REGISTER_NOFENCE_ULONG.

op_REGISTER_ULONG:

  • Does an ARM dsb 0xF to do a "full-system data sync barrier". This also
  • Then does a bit of magic to prevent the compiler from using certain optimizations that cause problems with register access.
  • Then does an actual ldr or str instruction, no optimizations allowed (compiler is not allowed to move the instruction above or below other REGISTER instructions and is not allowed to load more than once or store more than once).

op_REGISTER_ULONG_NOFENCE:

  • Same as a op_REGISTER_ULONG except without the dsb.

My understanding is that in most cases, op_REGISTER_ULONG_NOFENCE is sufficient. Though honestly, I'm a bit unclear about when you actually need the dsb, as it doesn't seem to do a lot of good in the cases I've considered (i.e. by itself it doesn't flush cache so it doesn't fix DMA).

@coolstar
Copy link
Collaborator Author

  • The driver binary is not part of the artifact. Does it build to the same location as the other drivers?

Might have been missing as the package was removed (fixed this), as the output location was correct

  • Source\Utilities\pl330dma.h could be moved to drivers\include since it may be used by other drivers. It's also missing include guards.

Done

  • Can you rename the device desc to "Rockchip I2S Controller" for consistency?

Done

  • Not a fan of the multiple vcxprojs TBH, the files could be all included in the base project without changing the directory structure (which you said you care about).

Could look into this for a follow-up PR

Looks good to me otherwise. Great work!

@coolstar coolstar merged commit afe2340 into master Nov 28, 2023
2 checks passed
@coolstar coolstar deleted the wdm-audio branch November 30, 2023 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants