[PATCH 00/11] HDSPM: RME AIO/RayDAT extensions and cleanups

[PATCH 00/11] HDSPM: RME AIO/RayDAT extensions and cleanups

[Adrian Knoth]

Hi!

Here's a series of patches that most importantly add support for AIO's
Analog Expansion Boards (AEBs) and all the missing controls to adjust
card settings (output levels, optical/coaxial etc.).

I also refactored a couple of functions to clean up the code and fixed
the S/PDIF sync/frequency reporting.


Cheers

Adrian Knoth (11):
  ALSA: hdspm - Add missing defines for RME AIO and RayDAT
  ALSA: hdspm - Introduce hdspm_is_raydat_or_aio()
  ALSA: hdspm - Augment HDSPM_TOGGLE_SETTING for AIO/RayDAT
  ALSA: hdspm - Drop duplicate code in hdspm_set_system_clock_mode()
  ALSA: hdspm - Add S/PDIF and WCK48 controls for RME RayDAT
  ALSA: hdspm - Add S/PDIF, XLR, WCK48 and ADAT-in controls for RME AIO
    cards
  ALSA: hdspm - Refactor ENUMERATED_CTL_INFO into function
  ALSA: hdspm - Introduce generic AIO tristate control
  ALSA: hdspm - Enable AD/DA/PH gains and S/PDIF-Input select on AIO
  ALSA: hdspm - Add support for AEBs on RME AIO
  ALSA: hdspm - Fix S/PDIF Sync status and frequency on RME AIO

 sound/pci/rme9652/hdspm.c | 254 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 199 insertions(+), 55 deletions(-)

-- 
1.8.3.1

[PATCH 01/11] ALSA: hdspm - Add missing defines for RME AIO and RayDAT

[Adrian Knoth]

The driver did not support all possible configurations. These defines
will be used by later commits to add the missing functionality.

Signed-off-by: Adrian Knoth <adi@drcomp.erfurt.thur.de>

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index bd50193..a0fc961 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -258,6 +258,25 @@ MODULE_SUPPORTED_DEVICE("{{RME HDSPM-MADI}}");
 
 #define HDSPM_wclk_sel (1<<30)
 
+/* additional control register bits for AIO*/
+#define HDSPM_c0_Wck48				0x20 /* also RayDAT */
+#define HDSPM_c0_Input0				0x1000
+#define HDSPM_c0_Input1				0x2000
+#define HDSPM_c0_Spdif_Opt			0x4000
+#define HDSPM_c0_Pro				0x8000
+#define HDSPM_c0_clr_tms			0x10000
+#define HDSPM_c0_AEB1				0x20000
+#define HDSPM_c0_AEB2				0x40000
+#define HDSPM_c0_LineOut			0x80000
+#define HDSPM_c0_AD_GAIN0			0x100000
+#define HDSPM_c0_AD_GAIN1			0x200000
+#define HDSPM_c0_DA_GAIN0			0x400000
+#define HDSPM_c0_DA_GAIN1			0x800000
+#define HDSPM_c0_PH_GAIN0			0x1000000
+#define HDSPM_c0_PH_GAIN1			0x2000000
+#define HDSPM_c0_Sym6db				0x4000000
+
+
 /* --- bit helper defines */
 #define HDSPM_LatencyMask    (HDSPM_Latency0|HDSPM_Latency1|HDSPM_Latency2)
 #define HDSPM_FrequencyMask  (HDSPM_Frequency0|HDSPM_Frequency1|\
-- 
1.8.3.1

[PATCH 02/11] ALSA: hdspm - Introduce hdspm_is_raydat_or_aio()

[Adrian Knoth]

RME RayDAT and AIO cards are new designs with different register
settings. Since we need to distinguish them from older cards multiple
times in the driver, refactor the code into a separate helper function.

No functional change intended.

Signed-off-by: Adrian Knoth <adi@drcomp.erfurt.thur.de>

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index a0fc961..32a87dc 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -1011,6 +1011,12 @@ static inline int HDSPM_bit2freq(int n)
 	return bit2freq_tab[n];
 }
 
+static bool hdspm_is_raydat_or_aio(struct hdspm *hdspm)
+{
+	return ((AIO == hdspm->io_type) || (RayDAT == hdspm->io_type));
+}
+
+
 /* Write/read to/from HDSPM with Adresses in Bytes
    not words but only 32Bit writes are allowed */
 
@@ -5142,9 +5148,8 @@ static int snd_hdspm_set_defaults(struct hdspm * hdspm)
 
 	all_in_all_mixer(hdspm, 0 * UNITY_GAIN);
 
-	if (hdspm->io_type == AIO || hdspm->io_type == RayDAT) {
+	if (hdspm_is_raydat_or_aio(hdspm))
 		hdspm_write(hdspm, HDSPM_WR_SETTINGS, hdspm->settings_register);
-	}
 
 	/* set a default rate so that the channel map is set up. */
 	hdspm_set_rate(hdspm, 48000, 1);
-- 
1.8.3.1

[PATCH 03/11] ALSA: hdspm - Augment HDSPM_TOGGLE_SETTING for AIO/RayDAT

[Adrian Knoth]

The HDSPM_TOGGLE_SETTING functions alter the control_register on older
cards. On newer cards (AIO/RayDAT), they have to operate on the
settings_register instead.

This patch augments the existing functions to work with AIO/RayDAT, too.

Signed-off-by: Adrian Knoth <adi@drcomp.erfurt.thur.de>

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index 32a87dc..118d727 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -3092,16 +3092,35 @@ static int snd_hdspm_get_tco_ltc_frames(struct snd_kcontrol *kcontrol,
 
 static int hdspm_toggle_setting(struct hdspm *hdspm, u32 regmask)
 {
-	return (hdspm->control_register & regmask) ? 1 : 0;
+	u32 reg;
+
+	if (hdspm_is_raydat_or_aio(hdspm))
+		reg = hdspm->settings_register;
+	else
+		reg = hdspm->control_register;
+
+	return (reg & regmask) ? 1 : 0;
 }
 
 static int hdspm_set_toggle_setting(struct hdspm *hdspm, u32 regmask, int out)
 {
+	u32 *reg;
+	u32 target_reg;
+
+	if (hdspm_is_raydat_or_aio(hdspm)) {
+		reg = &(hdspm->settings_register);
+		target_reg = HDSPM_WR_SETTINGS;
+	} else {
+		reg = &(hdspm->control_register);
+		target_reg = HDSPM_controlRegister;
+	}
+
 	if (out)
-		hdspm->control_register |= regmask;
+		*reg |= regmask;
 	else
-		hdspm->control_register &= ~regmask;
-	hdspm_write(hdspm, HDSPM_controlRegister, hdspm->control_register);
+		*reg &= ~regmask;
+
+	hdspm_write(hdspm, target_reg, *reg);
 
 	return 0;
 }
-- 
1.8.3.1

[PATCH 04/11] ALSA: hdspm - Drop duplicate code in hdspm_set_system_clock_mode()

[Adrian Knoth]

hdspm_set_system_clock_mode() is almost a one-by-one copy of
hdspm_set_toggle_setting(). To improve code quality, remove the
duplication.

Signed-off-by: Adrian Knoth <adi@drcomp.erfurt.thur.de>

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index 118d727..631c546 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -995,6 +995,7 @@ static inline void snd_hdspm_initialize_midi_flush(struct hdspm *hdspm);
 static inline int hdspm_get_pll_freq(struct hdspm *hdspm);
 static int hdspm_update_simple_mixer_controls(struct hdspm *hdspm);
 static int hdspm_autosync_ref(struct hdspm *hdspm);
+static int hdspm_set_toggle_setting(struct hdspm *hdspm, u32 regmask, int out);
 static int snd_hdspm_set_defaults(struct hdspm *hdspm);
 static int hdspm_system_clock_mode(struct hdspm *hdspm);
 static void hdspm_set_sgbuf(struct hdspm *hdspm,
@@ -2384,26 +2385,10 @@ static int hdspm_system_clock_mode(struct hdspm *hdspm)
  **/
 static void hdspm_set_system_clock_mode(struct hdspm *hdspm, int mode)
 {
-	switch (hdspm->io_type) {
-	case AIO:
-	case RayDAT:
-		if (0 == mode)
-			hdspm->settings_register |= HDSPM_c0Master;
-		else
-			hdspm->settings_register &= ~HDSPM_c0Master;
-
-		hdspm_write(hdspm, HDSPM_WR_SETTINGS, hdspm->settings_register);
-		break;
-
-	default:
-		if (0 == mode)
-			hdspm->control_register |= HDSPM_ClockModeMaster;
-		else
-			hdspm->control_register &= ~HDSPM_ClockModeMaster;
-
-		hdspm_write(hdspm, HDSPM_controlRegister,
-				hdspm->control_register);
-	}
+	hdspm_set_toggle_setting(hdspm,
+			(hdspm_is_raydat_or_aio(hdspm)) ?
+			HDSPM_c0Master : HDSPM_ClockModeMaster,
+			(0 == mode));
 }
 
 
-- 
1.8.3.1

[PATCH 05/11] ALSA: hdspm - Add S/PDIF and WCK48 controls for RME RayDAT

[Adrian Knoth]

This commit adds new ALSA controls to send single-speed WordClock and
S/PDIF-Professional on RME RayDAT cards.

Signed-off-by: Adrian Knoth <adi@drcomp.erfurt.thur.de>

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index 631c546..4a3a822 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -4364,7 +4364,9 @@ static struct snd_kcontrol_new snd_hdspm_controls_raydat[] = {
 	HDSPM_AUTOSYNC_SAMPLE_RATE("ADAT3 Frequency", 5),
 	HDSPM_AUTOSYNC_SAMPLE_RATE("ADAT4 Frequency", 6),
 	HDSPM_AUTOSYNC_SAMPLE_RATE("TCO Frequency", 7),
-	HDSPM_AUTOSYNC_SAMPLE_RATE("SYNC IN Frequency", 8)
+	HDSPM_AUTOSYNC_SAMPLE_RATE("SYNC IN Frequency", 8),
+	HDSPM_TOGGLE_SETTING("S/PDIF Out Professional", HDSPM_c0_Pro),
+	HDSPM_TOGGLE_SETTING("Single Speed WordClock Out", HDSPM_c0_Wck48)
 };
 
 static struct snd_kcontrol_new snd_hdspm_controls_aes32[] = {
-- 
1.8.3.1

[PATCH 06/11] ALSA: hdspm - Add S/PDIF, XLR, WCK48 and ADAT-in controls for RME AIO cards

[Adrian Knoth]

This commit adds the following ALSA controls:

   - S/PDIF Out Optical to switch S/PDIF Out from coaxial to optical
   - S/PDIF Out Professional to send the Pro bit in the output stream
   - ADAT-Internal to enable ADAT/TDIF Expansion Board (AEB/TEB)
   - XLR Breakout Cable if analogue I/O uses the XLR breakout cable
   - WCK48 to force WordClock to the 32-48kHz range (single speed) if
     the card is operating at higher frequencies

Signed-off-by: Adrian Knoth <adi@drcomp.erfurt.thur.de>

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index 4a3a822..15f1e7b 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -4327,7 +4327,12 @@ static struct snd_kcontrol_new snd_hdspm_controls_aio[] = {
 	HDSPM_AUTOSYNC_SAMPLE_RATE("SPDIF Frequency", 2),
 	HDSPM_AUTOSYNC_SAMPLE_RATE("ADAT Frequency", 3),
 	HDSPM_AUTOSYNC_SAMPLE_RATE("TCO Frequency", 4),
-	HDSPM_AUTOSYNC_SAMPLE_RATE("SYNC IN Frequency", 5)
+	HDSPM_AUTOSYNC_SAMPLE_RATE("SYNC IN Frequency", 5),
+	HDSPM_TOGGLE_SETTING("S/PDIF Out Optical", HDSPM_c0_Spdif_Opt),
+	HDSPM_TOGGLE_SETTING("S/PDIF Out Professional", HDSPM_c0_Pro),
+	HDSPM_TOGGLE_SETTING("ADAT internal (AEB/TEB)", HDSPM_c0_AEB1),
+	HDSPM_TOGGLE_SETTING("XLR Breakout Cable", HDSPM_c0_Sym6db),
+	HDSPM_TOGGLE_SETTING("Single Speed WordClock Out", HDSPM_c0_Wck48)
 
 		/*
 		   HDSPM_INPUT_SELECT("Input Select", 0),
-- 
1.8.3.1

[PATCH 07/11] ALSA: hdspm - Refactor ENUMERATED_CTL_INFO into function

[Adrian Knoth]

ENUMERATED_CTL_INFO is a macro, so the binary code is generated multiple
times. To avoid code duplication, refactor the involved functionality
into a function and make ENUMERATED_CTL_INFO a call to this function.

Signed-off-by: Adrian Knoth <adi@drcomp.erfurt.thur.de>

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index 15f1e7b..50453f9 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -2221,16 +2221,22 @@ static int hdspm_get_s1_sample_rate(struct hdspm *hdspm, unsigned int idx)
 	return (status >> (idx*4)) & 0xF;
 }
 
-#define ENUMERATED_CTL_INFO(info, texts) \
-{ \
-	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED; \
-	uinfo->count = 1; \
-	uinfo->value.enumerated.items = ARRAY_SIZE(texts); \
-	if (uinfo->value.enumerated.item >= uinfo->value.enumerated.items) \
-		uinfo->value.enumerated.item =	uinfo->value.enumerated.items - 1; \
-	strcpy(uinfo->value.enumerated.name, texts[uinfo->value.enumerated.item]); \
+static void snd_hdspm_set_infotext(struct snd_ctl_elem_info *uinfo,
+		char **texts, const int count)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
+	uinfo->count = 1;
+	uinfo->value.enumerated.items = count;
+	if (uinfo->value.enumerated.item >= uinfo->value.enumerated.items)
+		uinfo->value.enumerated.item =
+			uinfo->value.enumerated.items - 1;
+	strcpy(uinfo->value.enumerated.name,
+			texts[uinfo->value.enumerated.item]);
 }
 
+#define ENUMERATED_CTL_INFO(info, texts) \
+	snd_hdspm_set_infotext(info, texts, ARRAY_SIZE(texts));
+
 
 
 #define HDSPM_AUTOSYNC_SAMPLE_RATE(xname, xindex) \
-- 
1.8.3.1

[PATCH 08/11] ALSA: hdspm - Introduce generic AIO tristate control

[Adrian Knoth]

AIO cards offer at least four individual settings options with three
states each. Those settings are represented as two bits in the settings
register with the following meaning:

   0*some_base_bit --> Option value 0
   1*some_base_bit --> Option value 1
   2*some_base_bit --> Option value 2
   3*some_base_bit --> mask to select the two involved bits

This patch adds a generic ALSA control macro for such a value-to-bit
pattern mapping. It will be used in a later commit to expose four new
controls.

Signed-off-by: Adrian Knoth <adi@drcomp.erfurt.thur.de>

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index 50453f9..8b9c9f2 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -3348,6 +3348,84 @@ static int snd_hdspm_put_qs_wire(struct snd_kcontrol *kcontrol,
 	return change;
 }
 
+#define HDSPM_CONTROL_TRISTATE(xname, xindex) \
+{	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
+	.name = xname, \
+	.private_value = xindex, \
+	.info = snd_hdspm_info_tristate, \
+	.get = snd_hdspm_get_tristate, \
+	.put = snd_hdspm_put_tristate \
+}
+
+static int hdspm_tristate(struct hdspm *hdspm, u32 regmask)
+{
+	u32 reg = hdspm->settings_register & (regmask * 3);
+	return reg / regmask;
+}
+
+static int hdspm_set_tristate(struct hdspm *hdspm, int mode, u32 regmask)
+{
+	hdspm->settings_register &= ~(regmask * 3);
+	hdspm->settings_register |= (regmask * mode);
+	hdspm_write(hdspm, HDSPM_WR_SETTINGS, hdspm->settings_register);
+
+	return 0;
+}
+
+static int snd_hdspm_info_tristate(struct snd_kcontrol *kcontrol,
+				       struct snd_ctl_elem_info *uinfo)
+{
+	u32 regmask = kcontrol->private_value;
+
+	static char *texts_spdif[] = { "Optical", "Coaxial", "Internal" };
+	static char *texts_levels[] = { "Hi Gain", "+4 dBu", "-10 dBV" };
+
+	switch (regmask) {
+	case HDSPM_c0_Input0:
+		ENUMERATED_CTL_INFO(uinfo, texts_spdif);
+		break;
+	default:
+		ENUMERATED_CTL_INFO(uinfo, texts_levels);
+		break;
+	}
+	return 0;
+}
+
+static int snd_hdspm_get_tristate(struct snd_kcontrol *kcontrol,
+				      struct snd_ctl_elem_value *ucontrol)
+{
+	struct hdspm *hdspm = snd_kcontrol_chip(kcontrol);
+	u32 regmask = kcontrol->private_value;
+
+	spin_lock_irq(&hdspm->lock);
+	ucontrol->value.enumerated.item[0] = hdspm_tristate(hdspm, regmask);
+	spin_unlock_irq(&hdspm->lock);
+	return 0;
+}
+
+static int snd_hdspm_put_tristate(struct snd_kcontrol *kcontrol,
+				      struct snd_ctl_elem_value *ucontrol)
+{
+	struct hdspm *hdspm = snd_kcontrol_chip(kcontrol);
+	u32 regmask = kcontrol->private_value;
+	int change;
+	int val;
+
+	if (!snd_hdspm_use_is_exclusive(hdspm))
+		return -EBUSY;
+	val = ucontrol->value.integer.value[0];
+	if (val < 0)
+		val = 0;
+	if (val > 2)
+		val = 2;
+
+	spin_lock_irq(&hdspm->lock);
+	change = val != hdspm_tristate(hdspm, regmask);
+	hdspm_set_tristate(hdspm, val, regmask);
+	spin_unlock_irq(&hdspm->lock);
+	return change;
+}
+
 #define HDSPM_MADI_SPEEDMODE(xname, xindex) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
 	.name = xname, \
-- 
1.8.3.1

[PATCH 09/11] ALSA: hdspm - Enable AD/DA/PH gains and S/PDIF-Input select on AIO

[Adrian Knoth]

This patch uses the newly introduced HDSPM_CONTROL_TRISTATE functions to
create and expose the following ALSA controls:

   - Gain selection for Input, Output and Phones (HiGain, +4dBu, -10dbV)
   - S/PDIF Input select (Coaxial, Optical, Internal)

Signed-off-by: Adrian Knoth <adi@drcomp.erfurt.thur.de>

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index 8b9c9f2..111713c 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -4412,11 +4412,15 @@ static struct snd_kcontrol_new snd_hdspm_controls_aio[] = {
 	HDSPM_AUTOSYNC_SAMPLE_RATE("ADAT Frequency", 3),
 	HDSPM_AUTOSYNC_SAMPLE_RATE("TCO Frequency", 4),
 	HDSPM_AUTOSYNC_SAMPLE_RATE("SYNC IN Frequency", 5),
+	HDSPM_CONTROL_TRISTATE("S/PDIF Input", HDSPM_c0_Input0),
 	HDSPM_TOGGLE_SETTING("S/PDIF Out Optical", HDSPM_c0_Spdif_Opt),
 	HDSPM_TOGGLE_SETTING("S/PDIF Out Professional", HDSPM_c0_Pro),
 	HDSPM_TOGGLE_SETTING("ADAT internal (AEB/TEB)", HDSPM_c0_AEB1),
 	HDSPM_TOGGLE_SETTING("XLR Breakout Cable", HDSPM_c0_Sym6db),
-	HDSPM_TOGGLE_SETTING("Single Speed WordClock Out", HDSPM_c0_Wck48)
+	HDSPM_TOGGLE_SETTING("Single Speed WordClock Out", HDSPM_c0_Wck48),
+	HDSPM_CONTROL_TRISTATE("Input Level", HDSPM_c0_AD_GAIN0),
+	HDSPM_CONTROL_TRISTATE("Output Level", HDSPM_c0_DA_GAIN0),
+	HDSPM_CONTROL_TRISTATE("Phones Level", HDSPM_c0_PH_GAIN0)
 
 		/*
 		   HDSPM_INPUT_SELECT("Input Select", 0),
-- 
1.8.3.1

Re: [PATCH 07/11] ALSA: hdspm - Refactor ENUMERATED_CTL_INFO into function

[Takashi Iwai]

At Thu,  4 Jul 2013 15:16:34 +0200,
Adrian Knoth wrote:
> 
> ENUMERATED_CTL_INFO is a macro, so the binary code is generated multiple
> times. To avoid code duplication, refactor the involved functionality
> into a function and make ENUMERATED_CTL_INFO a call to this function.
> 
> Signed-off-by: Adrian Knoth <adi@drcomp.erfurt.thur.de>
> 
> diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
> index 15f1e7b..50453f9 100644
> --- a/sound/pci/rme9652/hdspm.c
> +++ b/sound/pci/rme9652/hdspm.c
> @@ -2221,16 +2221,22 @@ static int hdspm_get_s1_sample_rate(struct hdspm *hdspm, unsigned int idx)
>  	return (status >> (idx*4)) & 0xF;
>  }
>  
> -#define ENUMERATED_CTL_INFO(info, texts) \
> -{ \
> -	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED; \
> -	uinfo->count = 1; \
> -	uinfo->value.enumerated.items = ARRAY_SIZE(texts); \
> -	if (uinfo->value.enumerated.item >= uinfo->value.enumerated.items) \
> -		uinfo->value.enumerated.item =	uinfo->value.enumerated.items - 1; \
> -	strcpy(uinfo->value.enumerated.name, texts[uinfo->value.enumerated.item]); \
> +static void snd_hdspm_set_infotext(struct snd_ctl_elem_info *uinfo,
> +		char **texts, const int count)
> +{
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
> +	uinfo->count = 1;
> +	uinfo->value.enumerated.items = count;
> +	if (uinfo->value.enumerated.item >= uinfo->value.enumerated.items)
> +		uinfo->value.enumerated.item =
> +			uinfo->value.enumerated.items - 1;
> +	strcpy(uinfo->value.enumerated.name,
> +			texts[uinfo->value.enumerated.item]);
>  }
>  
> +#define ENUMERATED_CTL_INFO(info, texts) \
> +	snd_hdspm_set_infotext(info, texts, ARRAY_SIZE(texts));

The semicolon is superfluous (rather dangerous) here.
And you can reduce more codes by using the existing
snd_ctl_enum_info() helper function.

#define ENUMERATED_CTL_INFO(info, texts) \
	snd_ctl_enum_info(info, 1, texts, ARRAY_SIZE(texts))


thanks,

Takashi

Re: [PATCH 00/11] HDSPM: RME AIO/RayDAT extensions and cleanups

[Takashi Iwai]

At Thu,  4 Jul 2013 15:16:27 +0200,
Adrian Knoth wrote:
> 
> Hi!
> 
> Here's a series of patches that most importantly add support for AIO's
> Analog Expansion Boards (AEBs) and all the missing controls to adjust
> card settings (output levels, optical/coaxial etc.).
> 
> I also refactored a couple of functions to clean up the code and fixed
> the S/PDIF sync/frequency reporting.
> 
> 
> Cheers
> 
> Adrian Knoth (11):
>   ALSA: hdspm - Add missing defines for RME AIO and RayDAT
>   ALSA: hdspm - Introduce hdspm_is_raydat_or_aio()
>   ALSA: hdspm - Augment HDSPM_TOGGLE_SETTING for AIO/RayDAT
>   ALSA: hdspm - Drop duplicate code in hdspm_set_system_clock_mode()
>   ALSA: hdspm - Add S/PDIF and WCK48 controls for RME RayDAT
>   ALSA: hdspm - Add S/PDIF, XLR, WCK48 and ADAT-in controls for RME AIO
>     cards
>   ALSA: hdspm - Refactor ENUMERATED_CTL_INFO into function
>   ALSA: hdspm - Introduce generic AIO tristate control
>   ALSA: hdspm - Enable AD/DA/PH gains and S/PDIF-Input select on AIO
>   ALSA: hdspm - Add support for AEBs on RME AIO
>   ALSA: hdspm - Fix S/PDIF Sync status and frequency on RME AIO

The patches 10 and 11 are missing by some reason.
Could you resend them?


thanks,

Takashi

Re: [PATCH 07/11] ALSA: hdspm - Refactor ENUMERATED_CTL_INFO into function

[Adrian Knoth]

On Thu, Jul 04, 2013 at 03:28:34PM +0200, Takashi Iwai wrote:

> > ENUMERATED_CTL_INFO is a macro, so the binary code is generated multiple
> > times. To avoid code duplication, refactor the involved functionality
> > into a function and make ENUMERATED_CTL_INFO a call to this function.
> > 
> > Signed-off-by: Adrian Knoth <adi@drcomp.erfurt.thur.de>
> > 
> > diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
> > index 15f1e7b..50453f9 100644
> > --- a/sound/pci/rme9652/hdspm.c
> > +++ b/sound/pci/rme9652/hdspm.c
> > @@ -2221,16 +2221,22 @@ static int hdspm_get_s1_sample_rate(struct hdspm *hdspm, unsigned int idx)
> >  	return (status >> (idx*4)) & 0xF;
> >  }
> >  
> > -#define ENUMERATED_CTL_INFO(info, texts) \
> > -{ \
> > -	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED; \
> > -	uinfo->count = 1; \
> > -	uinfo->value.enumerated.items = ARRAY_SIZE(texts); \
> > -	if (uinfo->value.enumerated.item >= uinfo->value.enumerated.items) \
> > -		uinfo->value.enumerated.item =	uinfo->value.enumerated.items - 1; \
> > -	strcpy(uinfo->value.enumerated.name, texts[uinfo->value.enumerated.item]); \
> > +static void snd_hdspm_set_infotext(struct snd_ctl_elem_info *uinfo,
> > +		char **texts, const int count)
> > +{
> > +	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
> > +	uinfo->count = 1;
> > +	uinfo->value.enumerated.items = count;
> > +	if (uinfo->value.enumerated.item >= uinfo->value.enumerated.items)
> > +		uinfo->value.enumerated.item =
> > +			uinfo->value.enumerated.items - 1;
> > +	strcpy(uinfo->value.enumerated.name,
> > +			texts[uinfo->value.enumerated.item]);
> >  }
> >  
> > +#define ENUMERATED_CTL_INFO(info, texts) \
> > +	snd_hdspm_set_infotext(info, texts, ARRAY_SIZE(texts));
> 
> The semicolon is superfluous (rather dangerous) here.
> And you can reduce more codes by using the existing
> snd_ctl_enum_info() helper function.
> 
> #define ENUMERATED_CTL_INFO(info, texts) \
> 	snd_ctl_enum_info(info, 1, texts, ARRAY_SIZE(texts))

OK, looks useful. The exact function signature is a bit different (first
itemcount, then items).

Suggestion: I'll resend the entire series without the semicolon and add
another patch to convert all the text arrays from static char *texts[]
to static const char *const texts[]. In this patch, I'll also drop the
additional code.


Cheers

-- 
mail: adi@thur.de  	http://adi.thur.de	PGP/GPG: key via keyserver

Re: [PATCH 07/11] ALSA: hdspm - Refactor ENUMERATED_CTL_INFO into function

[Takashi Iwai]

At Thu, 4 Jul 2013 17:05:51 +0200,
Adrian Knoth wrote:
> 
> On Thu, Jul 04, 2013 at 03:28:34PM +0200, Takashi Iwai wrote:
> 
> > > ENUMERATED_CTL_INFO is a macro, so the binary code is generated multiple
> > > times. To avoid code duplication, refactor the involved functionality
> > > into a function and make ENUMERATED_CTL_INFO a call to this function.
> > > 
> > > Signed-off-by: Adrian Knoth <adi@drcomp.erfurt.thur.de>
> > > 
> > > diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
> > > index 15f1e7b..50453f9 100644
> > > --- a/sound/pci/rme9652/hdspm.c
> > > +++ b/sound/pci/rme9652/hdspm.c
> > > @@ -2221,16 +2221,22 @@ static int hdspm_get_s1_sample_rate(struct hdspm *hdspm, unsigned int idx)
> > >  	return (status >> (idx*4)) & 0xF;
> > >  }
> > >  
> > > -#define ENUMERATED_CTL_INFO(info, texts) \
> > > -{ \
> > > -	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED; \
> > > -	uinfo->count = 1; \
> > > -	uinfo->value.enumerated.items = ARRAY_SIZE(texts); \
> > > -	if (uinfo->value.enumerated.item >= uinfo->value.enumerated.items) \
> > > -		uinfo->value.enumerated.item =	uinfo->value.enumerated.items - 1; \
> > > -	strcpy(uinfo->value.enumerated.name, texts[uinfo->value.enumerated.item]); \
> > > +static void snd_hdspm_set_infotext(struct snd_ctl_elem_info *uinfo,
> > > +		char **texts, const int count)
> > > +{
> > > +	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
> > > +	uinfo->count = 1;
> > > +	uinfo->value.enumerated.items = count;
> > > +	if (uinfo->value.enumerated.item >= uinfo->value.enumerated.items)
> > > +		uinfo->value.enumerated.item =
> > > +			uinfo->value.enumerated.items - 1;
> > > +	strcpy(uinfo->value.enumerated.name,
> > > +			texts[uinfo->value.enumerated.item]);
> > >  }
> > >  
> > > +#define ENUMERATED_CTL_INFO(info, texts) \
> > > +	snd_hdspm_set_infotext(info, texts, ARRAY_SIZE(texts));
> > 
> > The semicolon is superfluous (rather dangerous) here.
> > And you can reduce more codes by using the existing
> > snd_ctl_enum_info() helper function.
> > 
> > #define ENUMERATED_CTL_INFO(info, texts) \
> > 	snd_ctl_enum_info(info, 1, texts, ARRAY_SIZE(texts))
> 
> OK, looks useful. The exact function signature is a bit different (first
> itemcount, then items).
> 
> Suggestion: I'll resend the entire series without the semicolon and add
> another patch to convert all the text arrays from static char *texts[]
> to static const char *const texts[]. In this patch, I'll also drop the
> additional code.

That's great.  Thanks.


Takashi