[RME MADI/RayDAT/AIO/AES] Changing hdspm_config and hdspm_status

[RME MADI/RayDAT/AIO/AES] Changing hdspm_config and hdspm_status

[Adrian Knoth]

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?

The changes are not upstream, yet, so we could arrange for a grace
period, maybe saying something like "Starting with Linux-3.x and
ALSA-1.0.y, you'll have to change your code". X and Y are yet to be
chosen wisely.

If you're interested to check if the upcoming changes affect your code,
I can send you the complete driver. I'll post the changes to hdspm.c in
a second, but that's just a preview for discussion, not necessarily the
final patch.


Cheers

Re: [RME MADI/RayDAT/AIO/AES] Changing hdspm_config and hdspm_status

[Adrian Knoth]

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;

Re: [RME MADI/RayDAT/AIO/AES] Changing hdspm_config and hdspm_status

[Adrian Knoth]

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

> +		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;
> +		}

Obviously, the other cards are missing. I can contribute RayDAT, if need
be (since I don't use the config ioctl at all, at least I don't need
it).

Fredrik: Do you still work on your hdspconf patch for hdspm? If so,
you'd probably be affected. Add "case MADIface:" below "case MADI:" to
see if it works.


>  			statusregister =
>  				hdspm_read(hdspm, HDSPM_statusRegister);
> -			status.card_specific.madi.madi_input =
> -				(statusregister & HDSPM_AB_int) ? 1 : 0;

MADI input (BNC/Optical) is a config option.

>  			status.card_specific.madi.channel_format =
> -				(statusregister & HDSPM_TX_64ch) ? 1 : 0;
> +				(statusregister & HDSPM_RX_64ch) ? 1 : 0;

This was simply wrong in the current driver: the status struct should
not tell if we're sending 64ch format, it should inform the user if
we're receiving 64ch on the wire.


Cheers

Re: [RME MADI/RayDAT/AIO/AES] Changing hdspm_config and hdspm_status

[Takashi Iwai]

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