On 08/22/11 18:28, Adrian Knoth wrote:

I'll post the changes to hdspm.c in a second, but that's just a preview for discussion, not necessarily the final patch.

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index 493e394..0518cfe 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -918,6 +918,7 @@ struct hdspm {

int last_external_sample_rate; /* samplerate mystic ... */ int last_internal_sample_rate; + uint32_t dds; int system_sample_rate;

int dev; /* Hardware vars... */ @@ -1355,6 +1356,7 @@ static void hdspm_set_dds_value(struct hdspm *hdspm, int rate)

if (rate >= 112000) rate /= 4; + hdspm->dds = (u32) n; else if (rate >= 56000) rate /= 2;

@@ -6093,7 +6095,7 @@ static int snd_hdspm_hwdep_ioctl(struct snd_hwdep *hw, struct file *file, void __user *argp = (void __user *)arg; struct hdspm *hdspm = hw->private_data; struct hdspm_mixer_ioctl mixer; - struct hdspm_config info; + struct hdspm_config config; struct hdspm_status status; struct hdspm_version hdspm_version; struct hdspm_peak_rms *levels; @@ -6199,21 +6201,36 @@ static int snd_hdspm_hwdep_ioctl(struct snd_hwdep *hw, struct file *file,

case SNDRV_HDSPM_IOCTL_GET_CONFIG:

- memset(&info, 0, sizeof(info)); + memset(&config, 0, sizeof(config)); spin_lock_irq(&hdspm->lock); - info.pref_sync_ref = hdspm_pref_sync_ref(hdspm); - info.wordclock_sync_check = hdspm_wc_sync_check(hdspm); - - info.system_sample_rate = hdspm->system_sample_rate; - info.autosync_sample_rate = - hdspm_external_sample_rate(hdspm); - info.system_clock_mode = hdspm_system_clock_mode(hdspm); - info.clock_source = hdspm_clock_source(hdspm); - info.autosync_ref = hdspm_autosync_ref(hdspm); - info.line_out = hdspm_line_out(hdspm); - info.passthru = 0; + config.dds = hdspm->dds; + if (hdspm->system_sample_rate <= 48000) { + config.freq = hdspm->system_sample_rate; + config.wck_mul = 1; + } else if (hdspm->system_sample_rate <= 96000) { + config.freq = hdspm->system_sample_rate/2; + config.wck_mul = 2; + } else { + config.freq = hdspm->system_sample_rate/4; + config.wck_mul = 4; + } + config.clock_mode = hdspm_system_clock_mode(hdspm); + + switch (hdspm->io_type) { + case MADI: + config.card_specific.madi.line_out = hdspm_line_out(hdspm); + config.card_specific.madi.tms = hdspm_c_tms(hdspm); + config.card_specific.madi.input_select = hdspm_input_select(hdspm); + config.card_specific.madi.output_mode = hdspm_tx_64(hdspm); + config.card_specific.madi.sync_ref = hdspm_pref_sync_ref(hdspm); + break; + + default: + break; + } + spin_unlock_irq(&hdspm->lock); - if (copy_to_user((void __user *) arg, &info, sizeof(info))) + if (copy_to_user((void __user *) arg, &config, sizeof(config))) return -EFAULT; break;

@@ -6239,10 +6256,8 @@ static int snd_hdspm_hwdep_ioctl(struct snd_hwdep *hw, struct file *file,

statusregister = hdspm_read(hdspm, HDSPM_statusRegister); - status.card_specific.madi.madi_input = - (statusregister & HDSPM_AB_int) ? 1 : 0; status.card_specific.madi.channel_format = - (statusregister & HDSPM_TX_64ch) ? 1 : 0; + (statusregister & HDSPM_RX_64ch) ? 1 : 0; /* TODO: Mac driver sets it when f_s>48kHz */ status.card_specific.madi.frame_format = 0;

At Mon, 22 Aug 2011 18:28:59 +0200, Adrian Knoth wrote:

Hi!

Florian Faber has sent me some new bits of his work on hdspm. While the changes to hdspm.c are straight-forward, he also cleaned up struct hdspm_config and hdspm_status, thus changing the userspace API/ABI:

diff --git a/include/sound/hdspm.h b/include/sound/hdspm.h index 1f59ea2..25eab09 100644 --- a/include/sound/hdspm.h +++ b/include/sound/hdspm.h @@ -58,17 +58,20 @@ struct hdspm_peak_rms { /* ------------ CONFIG block IOCTL ---------------------- */

struct hdspm_config {

• unsigned char pref_sync_ref;
• unsigned char wordclock_sync_check;
• unsigned char madi_sync_check;
• unsigned int system_sample_rate;
• unsigned int autosync_sample_rate;
• unsigned char system_clock_mode;
• unsigned char clock_source;
• unsigned char autosync_ref;
• unsigned char line_out;
• unsigned int passthru;
• unsigned int analog_out;

• uint32_t dds; /* dds increment */
• uint16_t freq; /* 32000/44100/48000 */
• uint8_t wck_mul; /* 1/2/4 */
• uint8_t clock_mode; /* 0=master, 1=slave */
• union {

• struct {
  

• 	uint8_t line_out;     /* 0=disabled, 1=enabled */
  

• 	uint8_t tms;          /* 0=disabled, 1=enabled */
  

• 	uint8_t input_select; /* 0=BNC, 1=optical */
  

• 	uint8_t output_mode;  /* 0=56ch, 1=64ch */
  

• 	uint8_t output_frame; /* 0=48K, 1=96K */
  

• 	uint8_t sync_ref;     /* 0=WCK, 1=MADI, 2=TCO/SYNC_IN, 3=SYNC_IN */
  

• } madi;
  

• } card_specific;

};

#define SNDRV_HDSPM_IOCTL_GET_CONFIG \ @@ -161,7 +164,6 @@ struct hdspm_status { uint8_t sync_madi; /* enum hdspm_sync */ uint8_t sync_tco; /* enum hdspm_sync */ uint8_t sync_in; /* enum hdspm_sync */

• 	uint8_t madi_input; /* enum hdspm_madi_input */
  uint8_t channel_format; /* enum hdspm_madi_channel_format */
  uint8_t frame_format; /* enum hdspm_madi_frame_format */
  

  } madi;

I guess this means we'd need to redefine two ioctl numbers. Besides this, are there any current users of this code?

It's hardly to tell. But in general, once when the code is upstreamed, basically you can't change in an incompatible way. So, changing the existing ioctl struct is no-go. It's worse than dropping the ioctl. Please implement a new ioctl instead.

thanks,

Takashi