Setups with digital audio connections like SPDIF and ADAT require a
designated master clock to stay in sync. Add a sysctl setting to control
the preferred clock source for each HDSPe sound card. Complement this by
sysctl values to list available clock sources, show the currently
effective clock source and display the sync status of all connections.
Clock sources are named according to RME user manuals.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
This is part of the improvements developed on github as an alternative kernel module for HDSPe sound cards. I built that separately as a local port / package. As such these changes have been tested extensively with a HDSPe RayDAT card, and are in use for several months now.
Information on the hardware register values can be found in the well-structured linux snd-hdspe driver, with hdspe_raio.c being relevant here.
The following patch was built and briefly tested on 15.0-CURRENT amd64, again with a RayDAT card, I don't own an HDSPe AIO card.
Testing
- Set the clock_preference to internal (master) and playback to an AD/DA converter via ADAT.
- Set the clock_preference to a digital input (ADAT) and do a recording with the AD/DA converter as master clock.
- Check the values of clock_source and sync_status in both setups, and with ADAT connected to a different port.
Comment Actions
great work, just minor things
| sys/dev/sound/pci/hdspe.c | ||
|---|---|---|
| 311 | I'd do this for easier reading, but this is not enforced by style(9) -- it allows initialization in declaration I think struct sc_info *sc; sc = oidp->oid_arg1; | |
| 317 | may be later to rename AIO into HDSPE_AIO, and have HDSPE_RAYDAT etc. (less confusion) | |
| 338 | Per style(9) we need to agree on usage (or not usage) of braces in single line statements (other parts of driver is not using them) | |
| 415 | Per style(9) "Values in return statements should be enclosed in parentheses." | |
Comment Actions
Fix minor readability and style(9) issues.
Unless I missed something, this should resolve all issues:
- Initialization in declaration is common in snd_uaudio(4), but not here. Fixed.
- Renamed hardware type macros to HDPSE_AIO, HDSPE_RAYDAT.
- Lots of braces removed around if / else conditional single line statements.
- Return values in parentheses.
@br: If all is ok, I'd be glad if you could commit this. I don't have commit bits.
Thank you!
Comment Actions
Looks good. but I noticed these things just before committing
| sys/dev/sound/pci/hdspe.c | ||
|---|---|---|
| 330 | what is magic number 15? may be some macro is needed for that. | |
| sys/dev/sound/pci/hdspe.h | ||
| 129 | does CLOCK_MASK include master bit here? a bit of confusion. may be it should be like this /* Clock sources */ #define HDSPE_SETTING_MASTER (1 << 0) #define HDSPE_SETTING_CLOCK_SHIFT (1) #define HDSPE_SETTING_CLOCK_MASK (0x0f << HDSPE_SETTING_CLOCK_SHIFT) #define HDSPE_SETTING_CLOCK(m, n) ((((n) << HDSPE_SETTING_CLOCK_SHIFT) &\ HDSPE_SETTING_CLOCK_MASK) |\ ((m) & HDSPE_SETTING_MASTER)) #define HDSPE_STATUS1_CLOCK_SHIFT (28) #define HDSPE_STATUS1_CLOCK_MASK (0x0f << HDSPE_STATUS1_CLOCK_SHIFT) #define HDSPE_STATUS1_CLOCK(n) (((n) << HDSPE_STATUS1_CLOCK_SHIFT) &\ HDSPE_STATUS1_CLOCK_MASK) | |
| 132 | note there should be tab after "#define" not space | |
Comment Actions
I'll fix up the patches tomorrow, see inline comments.
| sys/dev/sound/pci/hdspe.c | ||
|---|---|---|
| 330 |
It's just the status value of the internal entry in the clock source table. I don't want to create macros for arbitrary values from that table, but I could look up that value in the table instead? | |
| sys/dev/sound/pci/hdspe.h | ||
| 129 |
CLOCK_MASK does include the master bit. It's one setting, the values are not independent. Have a look at the hdspe_clock_source tables in hdspe.c. The only reason to split it in two is to expose the enumeration logic in that table. If you prefer, we could compose the value directly in the table initializer with 0 << 1 | 1 up to 10 << 1 | 0? Also style(9) talks about macro functions with side effects, but for simple cases like this upper or lower case seems to be arbitrary in the sources. Any reason for that? | |
| 132 |
Missed that. Frankly it's the least obvious use of tabs I've seen in my whole career ;-) | |
Comment Actions
Changelog:
- Should be all tabs now between #define and macros.
- Table lookup instead of magic number 15 for clock_source sysctl.
- Moved clock source setting register value creation to tables in hdspe.c.
- Simplified and cleaned up clock source related macros.
- Removed unused and invalid HDSPM_CLOCK_MODE_MASTER macro.