[hdspm: Add RME RayDAT/AIO 0/2] Reworked support for RayDAT/AIO

[hdspm: Add RME RayDAT/AIO 0/2] Reworked support for RayDAT/AIO

[Adrian Knoth]

Hi!

Finally, here's the patch that adds RME RayDAT and AIO support to hdpsm. 
Code was taken from Florian Faber's

   http://wiki.linuxproaudio.org/index.php/Driver:hdspe

revision (2010-12-25) and heavily modified to (almost) comply with the
coding style.

I also enabled S/PDIF input and augmented the code to allow for
all period sizes between 64 and 4096 samples on RayDATs/AIOs.

As pointed out by Takashi earlier this month, this version now doesn't
use typedefs or enums in ioctl structs.

Obviously, the patch is extremely long, it adds MMAP access, support for
the additional TCO hardware module, more MIDI ports and probably a lot
more.

Cheers

PS: This is the third version of the patch, now without conflicting
ioctls (hopefully) and without static arrays in the header file.

Adrian Knoth (2):
  ALSA: Add support for RME RayDAT and AIO
  ALSA: Add RayDAT and AIO strings to Kconfig

 include/sound/hdspm.h     |  349 ++++-
 sound/pci/Kconfig         |    6 +-
 sound/pci/rme9652/hdspm.c | 4231 +++++++++++++++++++++++++++++++++------------
 3 files changed, 3427 insertions(+), 1159 deletions(-)

-- 
1.7.2.3

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH - [hdspm] RayDAT/AIO 2/2] ALSA: Add RayDAT and AIO strings to Kconfig

[Adrian Knoth]

Inform users about the newly added support for RayDAT and AIO.

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

diff --git a/sound/pci/Kconfig b/sound/pci/Kconfig
index 9823d59..1fb3e24 100644
--- a/sound/pci/Kconfig
+++ b/sound/pci/Kconfig
@@ -572,13 +572,13 @@ comment "Don't forget to add built-in firmwares for HDSP driver"
 	depends on SND_HDSP=y
 
 config SND_HDSPM
-	tristate "RME Hammerfall DSP MADI"
+	tristate "RME Hammerfall DSP MADI/RayDAT/AIO"
 	select SND_HWDEP
 	select SND_RAWMIDI
 	select SND_PCM
 	help
-	  Say Y here to include support for RME Hammerfall DSP MADI
-	  soundcards.
+	  Say Y here to include support for RME Hammerfall DSP MADI,
+	  RayDAT and AIO soundcards.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called snd-hdspm.
-- 
1.7.2.3

Re: [PATCH - [hdspm] RayDAT/AIO 1/2] ALSA: Add support for RME RayDAT and AIO

[Takashi Iwai]

At Wed, 26 Jan 2011 19:32:14 +0100,
Adrian Knoth wrote:
> 
> diff --git a/include/sound/hdspm.h b/include/sound/hdspm.h
> index 81990b2..c3f1819 100644
> --- a/include/sound/hdspm.h
> +++ b/include/sound/hdspm.h
> @@ -131,4 +225,175 @@ typedef struct hdspm_version hdspm_version_t;
>  typedef struct hdspm_channelfader snd_hdspm_channelfader_t;
>  typedef struct hdspm_mixer hdspm_mixer_t;
>  
> -#endif				/* __SOUND_HDSPM_H */
> +/* These tables map the ALSA channels 1..N to the channels that we
> +   need to use in order to find the relevant channel buffer. RME
> +   refers to this kind of mapping as between "the ADAT channel and
> +   the DMA channel." We index it using the logical audio channel,
> +   and the value is the DMA channel (i.e. channel buffer number)
> +   where the data for that channel can be read/written from/to.
> +*/
> +
> +char channel_map_unity_ss[HDSPM_MAX_CHANNELS] = {

Err, no, array definitions should be never in a header file.
It doesn't matter whether it's static or not.  The definition should
be in *.c file where it's used.

So, simply move these array definitions to hdspm.c.


thanks,

Takashi

Re: [PATCH - [hdspm] RayDAT/AIO 1/2] ALSA: Add support for RME RayDAT and AIO

[Jaroslav Kysela]

On Thu, 27 Jan 2011, Takashi Iwai wrote:

> At Wed, 26 Jan 2011 19:32:14 +0100,
> Adrian Knoth wrote:
>>
>> diff --git a/include/sound/hdspm.h b/include/sound/hdspm.h
>> index 81990b2..c3f1819 100644
>> --- a/include/sound/hdspm.h
>> +++ b/include/sound/hdspm.h
>> @@ -131,4 +225,175 @@ typedef struct hdspm_version hdspm_version_t;
>>  typedef struct hdspm_channelfader snd_hdspm_channelfader_t;
>>  typedef struct hdspm_mixer hdspm_mixer_t;
>>
>> -#endif				/* __SOUND_HDSPM_H */
>> +/* These tables map the ALSA channels 1..N to the channels that we
>> +   need to use in order to find the relevant channel buffer. RME
>> +   refers to this kind of mapping as between "the ADAT channel and
>> +   the DMA channel." We index it using the logical audio channel,
>> +   and the value is the DMA channel (i.e. channel buffer number)
>> +   where the data for that channel can be read/written from/to.
>> +*/
>> +
>> +char channel_map_unity_ss[HDSPM_MAX_CHANNELS] = {
>
> Err, no, array definitions should be never in a header file.
> It doesn't matter whether it's static or not.  The definition should
> be in *.c file where it's used.
>
> So, simply move these array definitions to hdspm.c.

I think that the reason is to share these arrays with hdspmixer.
But I agree, that it's better to move these static values to .c files.

Note that hdsm/hdspmixer issues are last ones to resolve before 1.0.24 
release. I am waiting for the driver update.

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

[PATCH - hdspm 1/1] ALSA: [hdspm] Move static mapping arrays to .c

[Adrian Knoth]

As requested by Takashi and Jaroslav, these arrays should not be in the
header file.

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

diff --git a/include/sound/hdspm.h b/include/sound/hdspm.h
index c3f1819..1774ff5 100644
--- a/include/sound/hdspm.h
+++ b/include/sound/hdspm.h
@@ -225,175 +225,5 @@ typedef struct hdspm_version hdspm_version_t;
 typedef struct hdspm_channelfader snd_hdspm_channelfader_t;
 typedef struct hdspm_mixer hdspm_mixer_t;
 
-/* These tables map the ALSA channels 1..N to the channels that we
-   need to use in order to find the relevant channel buffer. RME
-   refers to this kind of mapping as between "the ADAT channel and
-   the DMA channel." We index it using the logical audio channel,
-   and the value is the DMA channel (i.e. channel buffer number)
-   where the data for that channel can be read/written from/to.
-*/
-
-char channel_map_unity_ss[HDSPM_MAX_CHANNELS] = {
-	0, 1, 2, 3, 4, 5, 6, 7,
-	8, 9, 10, 11, 12, 13, 14, 15,
-	16, 17, 18, 19, 20, 21, 22, 23,
-	24, 25, 26, 27, 28, 29, 30, 31,
-	32, 33, 34, 35, 36, 37, 38, 39,
-	40, 41, 42, 43, 44, 45, 46, 47,
-	48, 49, 50, 51, 52, 53, 54, 55,
-	56, 57, 58, 59, 60, 61, 62, 63
-};
-
-char channel_map_unity_ds[HDSPM_MAX_CHANNELS] = {
-	0, 2, 4, 6, 8, 10, 12, 14,
-	16, 18, 20, 22, 24, 26, 28, 30,
-	32, 34, 36, 38, 40, 42, 44, 46,
-	48, 50, 52, 54, 56, 58, 60, 62,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-};
-
-char channel_map_unity_qs[HDSPM_MAX_CHANNELS] = {
-	0, 4, 8, 12, 16, 20, 24, 28,
-	32, 36, 40, 44, 48, 52, 56, 60,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-};
-
-char channel_map_raydat_ss[HDSPM_MAX_CHANNELS] = {
-	4, 5, 6, 7, 8, 9, 10, 11,	/* ADAT 1 */
-	12, 13, 14, 15, 16, 17, 18, 19,	/* ADAT 2 */
-	20, 21, 22, 23, 24, 25, 26, 27,	/* ADAT 3 */
-	28, 29, 30, 31, 32, 33, 34, 35,	/* ADAT 4 */
-	0, 1,			/* AES */
-	2, 3,			/* SPDIF */
-	-1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-};
-
-char channel_map_raydat_ds[HDSPM_MAX_CHANNELS] = {
-	4, 5, 6, 7,		/* ADAT 1 */
-	8, 9, 10, 11,		/* ADAT 2 */
-	12, 13, 14, 15,		/* ADAT 3 */
-	16, 17, 18, 19,		/* ADAT 4 */
-	0, 1,			/* AES */
-	2, 3,			/* SPDIF */
-	-1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-};
-
-char channel_map_raydat_qs[HDSPM_MAX_CHANNELS] = {
-	4, 5,			/* ADAT 1 */
-	6, 7,			/* ADAT 2 */
-	8, 9,			/* ADAT 3 */
-	10, 11,			/* ADAT 4 */
-	0, 1,			/* AES */
-	2, 3,			/* SPDIF */
-	-1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-};
-
-char channel_map_aio_in_ss[HDSPM_MAX_CHANNELS] = {
-	0, 1,			/* line in */
-	8, 9,			/* aes in, */
-	10, 11,			/* spdif in */
-	12, 13, 14, 15, 16, 17, 18, 19,	/* ADAT in */
-	-1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-};
-
-char channel_map_aio_out_ss[HDSPM_MAX_CHANNELS] = {
-	0, 1,			/* line out */
-	8, 9,			/* aes out */
-	10, 11,			/* spdif out */
-	12, 13, 14, 15, 16, 17, 18, 19,	/* ADAT out */
-	6, 7,			/* phone out */
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-};
-
-char channel_map_aio_in_ds[HDSPM_MAX_CHANNELS] = {
-	0, 1,			/* line in */
-	8, 9,			/* aes in */
-	10, 11,			/* spdif in */
-	12, 14, 16, 18,		/* adat in */
-	-1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1
-};
-
-char channel_map_aio_out_ds[HDSPM_MAX_CHANNELS] = {
-	0, 1,			/* line out */
-	8, 9,			/* aes out */
-	10, 11,			/* spdif out */
-	12, 14, 16, 18,		/* adat out */
-	6, 7,			/* phone out */
-	-1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1
-};
-
-char channel_map_aio_in_qs[HDSPM_MAX_CHANNELS] = {
-	0, 1,			/* line in */
-	8, 9,			/* aes in */
-	10, 11,			/* spdif in */
-	12, 16,			/* adat in */
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1
-};
-
-char channel_map_aio_out_qs[HDSPM_MAX_CHANNELS] = {
-	0, 1,			/* line out */
-	8, 9,			/* aes out */
-	10, 11,			/* spdif out */
-	12, 16,			/* adat out */
-	6, 7,			/* phone out */
-	-1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1,
-	-1, -1, -1, -1, -1, -1, -1, -1
-};
 
 #endif
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index 2db871d..28a1eb3 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -673,6 +673,177 @@ static char *texts_ports_aio_out_qs[] = {
 	"Phone.L", "Phone.R"
 };
 
+/* These tables map the ALSA channels 1..N to the channels that we
+   need to use in order to find the relevant channel buffer. RME
+   refers to this kind of mapping as between "the ADAT channel and
+   the DMA channel." We index it using the logical audio channel,
+   and the value is the DMA channel (i.e. channel buffer number)
+   where the data for that channel can be read/written from/to.
+*/
+
+static char channel_map_unity_ss[HDSPM_MAX_CHANNELS] = {
+	0, 1, 2, 3, 4, 5, 6, 7,
+	8, 9, 10, 11, 12, 13, 14, 15,
+	16, 17, 18, 19, 20, 21, 22, 23,
+	24, 25, 26, 27, 28, 29, 30, 31,
+	32, 33, 34, 35, 36, 37, 38, 39,
+	40, 41, 42, 43, 44, 45, 46, 47,
+	48, 49, 50, 51, 52, 53, 54, 55,
+	56, 57, 58, 59, 60, 61, 62, 63
+};
+
+static char channel_map_unity_ds[HDSPM_MAX_CHANNELS] = {
+	0, 2, 4, 6, 8, 10, 12, 14,
+	16, 18, 20, 22, 24, 26, 28, 30,
+	32, 34, 36, 38, 40, 42, 44, 46,
+	48, 50, 52, 54, 56, 58, 60, 62,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+};
+
+static char channel_map_unity_qs[HDSPM_MAX_CHANNELS] = {
+	0, 4, 8, 12, 16, 20, 24, 28,
+	32, 36, 40, 44, 48, 52, 56, 60,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+};
+
+static char channel_map_raydat_ss[HDSPM_MAX_CHANNELS] = {
+	4, 5, 6, 7, 8, 9, 10, 11,	/* ADAT 1 */
+	12, 13, 14, 15, 16, 17, 18, 19,	/* ADAT 2 */
+	20, 21, 22, 23, 24, 25, 26, 27,	/* ADAT 3 */
+	28, 29, 30, 31, 32, 33, 34, 35,	/* ADAT 4 */
+	0, 1,			/* AES */
+	2, 3,			/* SPDIF */
+	-1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+};
+
+static char channel_map_raydat_ds[HDSPM_MAX_CHANNELS] = {
+	4, 5, 6, 7,		/* ADAT 1 */
+	8, 9, 10, 11,		/* ADAT 2 */
+	12, 13, 14, 15,		/* ADAT 3 */
+	16, 17, 18, 19,		/* ADAT 4 */
+	0, 1,			/* AES */
+	2, 3,			/* SPDIF */
+	-1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+};
+
+static char channel_map_raydat_qs[HDSPM_MAX_CHANNELS] = {
+	4, 5,			/* ADAT 1 */
+	6, 7,			/* ADAT 2 */
+	8, 9,			/* ADAT 3 */
+	10, 11,			/* ADAT 4 */
+	0, 1,			/* AES */
+	2, 3,			/* SPDIF */
+	-1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+};
+
+static char channel_map_aio_in_ss[HDSPM_MAX_CHANNELS] = {
+	0, 1,			/* line in */
+	8, 9,			/* aes in, */
+	10, 11,			/* spdif in */
+	12, 13, 14, 15, 16, 17, 18, 19,	/* ADAT in */
+	-1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+};
+
+static char channel_map_aio_out_ss[HDSPM_MAX_CHANNELS] = {
+	0, 1,			/* line out */
+	8, 9,			/* aes out */
+	10, 11,			/* spdif out */
+	12, 13, 14, 15, 16, 17, 18, 19,	/* ADAT out */
+	6, 7,			/* phone out */
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+};
+
+static char channel_map_aio_in_ds[HDSPM_MAX_CHANNELS] = {
+	0, 1,			/* line in */
+	8, 9,			/* aes in */
+	10, 11,			/* spdif in */
+	12, 14, 16, 18,		/* adat in */
+	-1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1
+};
+
+static char channel_map_aio_out_ds[HDSPM_MAX_CHANNELS] = {
+	0, 1,			/* line out */
+	8, 9,			/* aes out */
+	10, 11,			/* spdif out */
+	12, 14, 16, 18,		/* adat out */
+	6, 7,			/* phone out */
+	-1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1
+};
+
+static char channel_map_aio_in_qs[HDSPM_MAX_CHANNELS] = {
+	0, 1,			/* line in */
+	8, 9,			/* aes in */
+	10, 11,			/* spdif in */
+	12, 16,			/* adat in */
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1
+};
+
+static char channel_map_aio_out_qs[HDSPM_MAX_CHANNELS] = {
+	0, 1,			/* line out */
+	8, 9,			/* aes out */
+	10, 11,			/* spdif out */
+	12, 16,			/* adat out */
+	6, 7,			/* phone out */
+	-1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1,
+	-1, -1, -1, -1, -1, -1, -1, -1
+};
+
 struct hdspm_midi {
 	struct hdspm *hdspm;
 	int id;
-- 
1.7.2.3

Re: [PATCH - [hdspm] RayDAT/AIO 1/2] ALSA: Add support for RME RayDAT and AIO

[Adrian Knoth]

On Thu, Jan 27, 2011 at 09:09:05AM +0100, Jaroslav Kysela wrote:

>>> +char channel_map_unity_ss[HDSPM_MAX_CHANNELS] = {
>>
>> Err, no, array definitions should be never in a header file.
>> It doesn't matter whether it's static or not.  The definition should
>> be in *.c file where it's used.

We have more than one C file using this information...

>> So, simply move these array definitions to hdspm.c.
>
> I think that the reason is to share these arrays with hdspmixer.

Exactly. hdspmixer needs this information, too. If we move it to the C
file, we would need to duplicate the information in hdspmixer. And then
we forget about it next time hdspm.h needs to be updated.


> But I agree, that it's better to move these static values to .c files.

Ok, I've sent a follow-up patch to avoid to send the whole 180k patch
for the 4th time.

I'll now duplicate this information and send a patch against hdspmixer.
I feel that there should be a better solution like getting the channel
mapping from the driver (sysfs, proc, ioctl or whatever), but that's for
some other time.


HTH

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

Re: [PATCH - [hdspm] RayDAT/AIO 1/2] ALSA: Add support for RME RayDAT and AIO

[Jaroslav Kysela]

On Thu, 27 Jan 2011, Adrian Knoth wrote:

> On Thu, Jan 27, 2011 at 09:09:05AM +0100, Jaroslav Kysela wrote:
>
>>>> +char channel_map_unity_ss[HDSPM_MAX_CHANNELS] = {
>>>
>>> Err, no, array definitions should be never in a header file.
>>> It doesn't matter whether it's static or not.  The definition should
>>> be in *.c file where it's used.
>
> We have more than one C file using this information...
>
>>> So, simply move these array definitions to hdspm.c.
>>
>> I think that the reason is to share these arrays with hdspmixer.
>
> Exactly. hdspmixer needs this information, too. If we move it to the C
> file, we would need to duplicate the information in hdspmixer. And then
> we forget about it next time hdspm.h needs to be updated.
>
>
>> But I agree, that it's better to move these static values to .c files.
>
> Ok, I've sent a follow-up patch to avoid to send the whole 180k patch
> for the 4th time.
>
> I'll now duplicate this information and send a patch against hdspmixer.
> I feel that there should be a better solution like getting the channel
> mapping from the driver (sysfs, proc, ioctl or whatever), but that's for
> some other time.

Thanks. I applied your patches to my git tree with some slight subject 
modifications. Please, Takashi, do a cherry-pick. Thanks.

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [PATCH - [hdspm] RayDAT/AIO 1/2] ALSA: Add support for RME RayDAT and AIO

[Takashi Iwai]

At Thu, 27 Jan 2011 11:27:57 +0100,
Adrian Knoth wrote:
> 
> On Thu, Jan 27, 2011 at 09:09:05AM +0100, Jaroslav Kysela wrote:
> 
> >>> +char channel_map_unity_ss[HDSPM_MAX_CHANNELS] = {
> >>
> >> Err, no, array definitions should be never in a header file.
> >> It doesn't matter whether it's static or not.  The definition should
> >> be in *.c file where it's used.
> 
> We have more than one C file using this information...

Still it doesn't rectify.

> >> So, simply move these array definitions to hdspm.c.
> >
> > I think that the reason is to share these arrays with hdspmixer.
> 
> Exactly. hdspmixer needs this information, too. If we move it to the C
> file, we would need to duplicate the information in hdspmixer. And then
> we forget about it next time hdspm.h needs to be updated.

Then such info isn't appropriate to be static.  If it's dynamic,
this has to be provided via other interface.


> > But I agree, that it's better to move these static values to .c files.
> 
> Ok, I've sent a follow-up patch to avoid to send the whole 180k patch
> for the 4th time.
> 
> I'll now duplicate this information and send a patch against hdspmixer.
> I feel that there should be a better solution like getting the channel
> mapping from the driver (sysfs, proc, ioctl or whatever), but that's for
> some other time.

Exactly.

BTW, now I get the following warnings:
  sound/pci/rme9652/hdspm.c:578:14: warning: ‘texts_sync_status’ defined but not used
  sound/pci/rme9652/hdspm.c:695:13: warning: ‘channel_map_unity_ds’ defined but not used
  sound/pci/rme9652/hdspm.c:706:13: warning: ‘channel_map_unity_qs’ defined but not used
  sound/pci/rme9652/hdspm.c: In function ‘snd_hdspm_hwdep_ioctl’:
  sound/pci/rme9652/hdspm.c:6141:1: warning: the frame size of 2464 bytes is larger than 1024 bytes

Care to fix it?


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH - [hdspm] RayDAT/AIO 1/2] ALSA: Add support for RME RayDAT and AIO

[Takashi Iwai]

At Thu, 27 Jan 2011 11:44:34 +0100 (CET),
Jaroslav Kysela wrote:
> 
> On Thu, 27 Jan 2011, Adrian Knoth wrote:
> 
> > On Thu, Jan 27, 2011 at 09:09:05AM +0100, Jaroslav Kysela wrote:
> >
> >>>> +char channel_map_unity_ss[HDSPM_MAX_CHANNELS] = {
> >>>
> >>> Err, no, array definitions should be never in a header file.
> >>> It doesn't matter whether it's static or not.  The definition should
> >>> be in *.c file where it's used.
> >
> > We have more than one C file using this information...
> >
> >>> So, simply move these array definitions to hdspm.c.
> >>
> >> I think that the reason is to share these arrays with hdspmixer.
> >
> > Exactly. hdspmixer needs this information, too. If we move it to the C
> > file, we would need to duplicate the information in hdspmixer. And then
> > we forget about it next time hdspm.h needs to be updated.
> >
> >
> >> But I agree, that it's better to move these static values to .c files.
> >
> > Ok, I've sent a follow-up patch to avoid to send the whole 180k patch
> > for the 4th time.
> >
> > I'll now duplicate this information and send a patch against hdspmixer.
> > I feel that there should be a better solution like getting the channel
> > mapping from the driver (sysfs, proc, ioctl or whatever), but that's for
> > some other time.
> 
> Thanks. I applied your patches to my git tree with some slight subject 
> modifications. Please, Takashi, do a cherry-pick. Thanks.

Done.  But there are compile warnings.  They must be fixed before your
release.

Also, sync again hdspm.h in alsa-lib and etc.


thanks,

Takashi

Re: [PATCH - [hdspm] RayDAT/AIO 1/2] ALSA: Add support for RME RayDAT and AIO

[Jaroslav Kysela]

On Thu, 27 Jan 2011, Takashi Iwai wrote:

> At Thu, 27 Jan 2011 11:44:34 +0100 (CET),
> Jaroslav Kysela wrote:
>>
>> On Thu, 27 Jan 2011, Adrian Knoth wrote:
>>
>>> On Thu, Jan 27, 2011 at 09:09:05AM +0100, Jaroslav Kysela wrote:
>>>
>>>>>> +char channel_map_unity_ss[HDSPM_MAX_CHANNELS] = {
>>>>>
>>>>> Err, no, array definitions should be never in a header file.
>>>>> It doesn't matter whether it's static or not.  The definition should
>>>>> be in *.c file where it's used.
>>>
>>> We have more than one C file using this information...
>>>
>>>>> So, simply move these array definitions to hdspm.c.
>>>>
>>>> I think that the reason is to share these arrays with hdspmixer.
>>>
>>> Exactly. hdspmixer needs this information, too. If we move it to the C
>>> file, we would need to duplicate the information in hdspmixer. And then
>>> we forget about it next time hdspm.h needs to be updated.
>>>
>>>
>>>> But I agree, that it's better to move these static values to .c files.
>>>
>>> Ok, I've sent a follow-up patch to avoid to send the whole 180k patch
>>> for the 4th time.
>>>
>>> I'll now duplicate this information and send a patch against hdspmixer.
>>> I feel that there should be a better solution like getting the channel
>>> mapping from the driver (sysfs, proc, ioctl or whatever), but that's for
>>> some other time.
>>
>> Thanks. I applied your patches to my git tree with some slight subject
>> modifications. Please, Takashi, do a cherry-pick. Thanks.
>
> Done.  But there are compile warnings.  They must be fixed before your
> release.

I fixed all warning in commit "ALSA: hdspm - remove unused arrays, reduce 
stack usage in hwdep_ioctl" now.

> Also, sync again hdspm.h in alsa-lib and etc.

Sure.

 					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [PATCH - [hdspm] RayDAT/AIO 1/2] ALSA: Add support for RME RayDAT and AIO

[Florian Faber]

On 01/27/11 09:09, Jaroslav Kysela wrote:

>> So, simply move these array definitions to hdspm.c.
> I think that the reason is to share these arrays with hdspmixer.

There's more than hdspmixer out there that needs this data.

> But I agree, that it's better to move these static values to .c files.

The solution is to make them non-static (which was an error on my side).
Replicating information is not a bright idea.


Flo
-- 
Machines can do the work, so people have time to think.
public key 6C002249          x-hkp://wwwkeys.eu.pgp.net