sound/midi: Clarify MPU-401 init #1905
Open
+96
−63
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
n-p-soft
added a commit
to n-p-soft/freebsd-src
that referenced
this pull request
Nov 29, 2025
NOTE: this depends on pull requests freebsd#1905 and freebsd#1892. MIDI code in emu10kx driver was disabled due to possible locking problems in MIDI core, which should have been corrected by freebsd#1905. Here, we also fixed that the card interrupts were enabled too early and disabled too late. This was tested under FreeBSD 15.0 using a SoundBlaster Audigy (chipset CA0100, I will also try one Audigy2). Signed-off-by: Nicolas Provost <[email protected]>
n-p-soft
added a commit
to n-p-soft/freebsd-src
that referenced
this pull request
Nov 29, 2025
NOTE: this depends on pull requests freebsd#1902 and freebsd#1905. MIDI code in emu10kx driver was disabled due to possible locking problems in MIDI core, which should have been corrected by freebsd#1902. Here, we also fixed that the card interrupts were enabled too early and disabled too late. This was tested under FreeBSD 15.0 using a SoundBlaster Audigy (chipset CA0100, I will also try one Audigy2). Signed-off-by: Nicolas Provost <[email protected]>
First, we try to better init MPU-401 chip so that it is put in UART mode using the recommended way: <reset><wait_for_ack><enable_uart_mode>. This must be done before the interrupt handler is active, so that the ACK is the only data received between the two commands. Actually this setup may occur after interrupts are enabled for the card. Second, there is a mechanism to periodically call the interrupt handler, probably to address very old or spurious cards. We remove this because all cards in the tree using MPU-401 (CSMedia chips, Crystal chips, and Soundblasters) have proper interrupt handling for MIDI events. Rather old hardware is concerned; but PCI soundcards may work fine using a PCIe-to-PCI bridge, and some have pretty decent chipsets. Signed-off-by: Nicolas Provost <[email protected]>
christosmarg
reviewed
Dec 2, 2025
Contributor
christosmarg
left a comment
christosmarg
left a comment
There was a problem hiding this comment.
Mostly good, just some notes.
| for (i = 0; i < MPU_TRYDATA; i++) { | ||
| if (TXRDY(m)) | ||
| return (1); | ||
| else if (RXRDY(m)) |
Contributor
There was a problem hiding this comment.
As I mentioned in the old PR, it'd be good to have a comment here explaining the reason for discarding input. It is not obvious to someone looking at this for the first time.
| return (0); | ||
| } | ||
|
|
||
| /* Some cards only support UART mode but others will start in |
Contributor
There was a problem hiding this comment.
The comment should start on the next line. Same applies to other multi-line comments.
| } | ||
|
|
||
| /* Some cards only support UART mode but others will start in | ||
| * "Intelligent" mode, so we must try to switch to UART mode. |
Contributor
There was a problem hiding this comment.
Here we say that "we must try to switch to UART mode" but I do not think it is entirely clear why we must do this. I'm not saying it's not correct to do this, but the comment should also explain the "why" in my opinion.
| i++; | ||
| /* DELAY(100); */ | ||
| s = STATUS(m); | ||
| else |
| if ((m->flags & M_TXEN) && (m->si)) { | ||
| callout_reset(&m->timer, 1, mpu401_timeout, m); | ||
| /* Read and buffer all input bytes, then send data. | ||
| * Note that pending input may inhibits data sending. |
| callout_reset(&m->timer, 1, mpu401_timeout, m); | ||
| /* Read and buffer all input bytes, then send data. | ||
| * Note that pending input may inhibits data sending. | ||
| * Spurious cards may also be sticky, so limit the count of tries |
Contributor
There was a problem hiding this comment.
Should explain what "sticky" means. Also "number of tries", not "count of tries".
| * (probably MPU cards have no more than 16 bytes as internal buffer). | ||
| */ | ||
| for (i = 0; RXRDY(m) && i < MPU_INTR_BUF; i++) { | ||
| /* XXX we should check midi_in has room in its buffer, else |
Contributor
There was a problem hiding this comment.
Is there a reason this patch does not do what the comment here says?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First, we try to better init MPU-401 chip so that it is put in UART mode using the recommended way: <wait_for_ack><enable_uart_mode>. This must be done before the interrupt handler is active, so that the ACK is the only data received between the two commands. Actually this setup may occur after interrupts are enabled for the card.
Second, there is a mechanism to periodically call the interrupt handler, probably to address very old or spurious cards. We remove this because all cards in the tree using MPU-401 (CSMedia chips, Crystal chips, and Soundblasters) have proper interrupt handling for MIDI events.
Rather old hardware is concerned; but PCI soundcards may work fine using a PCIe-to-PCI bridge, and some have pretty decent chipsets.