[PATCH - [hdspm] 0/3] Support for RME RayDAT and AIO

[PATCH - [hdspm] 0/3] Support for RME RayDAT and AIO

[Adrian Knoth]

Hi!

As announced a few days ago, I've integrated the offspring hdspm driver
by Florian Faber. The new code now also supports the 36in/36out RME
RayDAT cards and RME AIOs.

I tried to follow the kernel's coding style as much as possible,
however, there are still a few overlong lines left. For example, I
didn't want to break "if read_reg(...) & 0x3f" at "& 0x3f", as it
worsens readability. If it's required to comply, feel free to add the
missing line breaks.

Right now, RayDATs and AIOs can only operate on a single period size
of 1024 samples, because these cards always use a fixed 16k buffer per
channel and only vary interrupt frequency. I'm going to try to fix this
after I've learned more about the PCM interface. ;) Any suggestions
welcome.

Cheers

Adrian Knoth (3):
  Add headers for RayDAT and AIO support.
  Enable RME RayDAT and AIO
  Add RayDAT and AIO strings to Kconfig

 include/hdspm.h     |  442 +++++-
 pci/Kconfig         |    6 +-
 pci/rme9652/hdspm.c | 4135 +++++++++++++++++++++++++++++++++++++--------------
 3 files changed, 3442 insertions(+), 1141 deletions(-)

-- 
1.7.2.3

[PATCH - [hdspm] 1/3] Add headers for RayDAT and AIO support.

[Adrian Knoth]

Merged the changes by Florian Faber to the hdspm driver. Code taken from

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

Whitespace fixes, also added missing commas to the texts_ports_* arrays.
Changed card_clock to unsigned long long in struct hdspm_status.

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

diff --git a/include/hdspm.h b/include/hdspm.h
index 81990b2..5b69694 100644
--- a/include/hdspm.h
+++ b/include/hdspm.h
@@ -3,8 +3,8 @@
 /*
  *   Copyright (C) 2003 Winfried Ritsch (IEM)
  *   based on hdsp.h from Thomas Charbonnel (thomas@undata.org)
- *                      
- *    
+ *
+ *
  *   This program is free software; you can redistribute it and/or modify
  *   it under the terms of the GNU General Public License as published by
  *   the Free Software Foundation; either version 2 of the License, or
@@ -23,50 +23,37 @@
 /* Maximum channels is 64 even on 56Mode you have 64playbacks to matrix */
 #define HDSPM_MAX_CHANNELS      64
 
-/* -------------------- IOCTL Peak/RMS Meters -------------------- */
-
-/* peam rms level structure like we get from hardware 
-  
-   maybe in future we can memory map it so I just copy it
-   to user on ioctl call now an dont change anything
-   rms are made out of low and high values
-   where (long) ????_rms = (????_rms_l >> 8) + ((????_rms_h & 0xFFFFFF00)<<24)
-   (i asume so from the code)
-*/
-
-struct hdspm_peak_rms {
+typedef enum {
+	MADI,
+	MADIface,
+	AIO,
+	AES32,
+	RayDAT
+} hdspm_io_type;
 
-	unsigned int level_offset[1024];
 
-	unsigned int input_peak[64];
-	unsigned int playback_peak[64];
-	unsigned int output_peak[64];
-	unsigned int xxx_peak[64];	/* not used */
+/* -------------------- IOCTL Peak/RMS Meters -------------------- */
 
-	unsigned int reserved[256];	/* not used */
+struct hdspm_peak_rms {
+		uint32_t input_peaks[64];
+		uint32_t playback_peaks[64];
+		uint32_t output_peaks[64];
 
-	unsigned int input_rms_l[64];
-	unsigned int playback_rms_l[64];
-	unsigned int output_rms_l[64];
-	unsigned int xxx_rms_l[64];	/* not used */
+		uint64_t input_rms[64];
+		uint64_t playback_rms[64];
+		uint64_t output_rms[64];
 
-	unsigned int input_rms_h[64];
-	unsigned int playback_rms_h[64];
-	unsigned int output_rms_h[64];
-	unsigned int xxx_rms_h[64];	/* not used */
+		enum {ss, ds, qs} speed;
+		int status2;
 };
 
-struct hdspm_peak_rms_ioctl {
-	struct hdspm_peak_rms *peak;
-};
 
-/* use indirect access due to the limit of ioctl bit size */
 #define SNDRV_HDSPM_IOCTL_GET_PEAK_RMS \
-	_IOR('H', 0x40, struct hdspm_peak_rms_ioctl)
+	_IOR('H', 0x42, struct hdspm_peak_rms)
 
 /* ------------ CONFIG block IOCTL ---------------------- */
 
-struct hdspm_config_info {
+struct hdspm_config {
 	unsigned char pref_sync_ref;
 	unsigned char wordclock_sync_check;
 	unsigned char madi_sync_check;
@@ -80,14 +67,118 @@ struct hdspm_config_info {
 	unsigned int analog_out;
 };
 
-#define SNDRV_HDSPM_IOCTL_GET_CONFIG_INFO \
-	_IOR('H', 0x41, struct hdspm_config_info)
 
 
-/* get Soundcard Version */
+#define SNDRV_HDSPM_IOCTL_GET_CONFIG \
+	_IOR('H', 0x41, struct hdspm_config)
+
+
+
+/**
+ * If there's a TCO (TimeCode Option) board installed,
+ * there are further options and status data available.
+ * The hdspm_ltc structure contains the current SMPTE
+ * timecode and some status information and can be
+ * optained via SNDRV_HDSPM_IOCTL_GET_LTC or in the
+ * hdspm_status struct.
+ **/
+
+typedef enum {
+	format_invalid,
+	fps_24,
+	fps_25,
+	fps_2997,
+	fps_30
+} hdspm_ltc_format;
+
+typedef enum {
+	frame_invalid,
+	drop_frame,
+	full_frame
+} hdspm_ltc_frame;
+
+typedef enum {
+	ntsc,
+	pal,
+	no_video
+} hdspm_ltc_input_format;
+
+struct hdspm_ltc {
+	unsigned int ltc;
+
+	hdspm_ltc_format format;
+	hdspm_ltc_frame frame;
+	hdspm_ltc_input_format input_format;
+};
+
+#define SNDRV_HDSPM_IOCTL_GET_LTC _IOR('H', 0x46, struct hdspm_mixer_ioctl)
+
+
+/**
+ * The status data reflects the device's current state
+ * as determined by the card's configuration and
+ * connection status.
+ **/
+
+typedef enum {
+	hdspm_sync_no_lock = 0,
+	hdspm_sync_lock = 1,
+	hdspm_sync_sync = 2
+} hdspm_sync;
+
+typedef enum {hdspm_input_optical = 0, hdspm_input_coax = 1} hdspm_madi_input;
+
+typedef enum {hdspm_format_ch_64 = 0,
+	hdspm_format_ch_56 = 1
+} hdspm_madi_channel_format;
+
+typedef enum {hdspm_frame_48 = 0,
+	hdspm_frame_96 = 1
+} hdspm_madi_frame_format;
+
+typedef enum {syncsource_wc = 0,
+	syncsource_madi = 1,
+	syncsource_tco = 2,
+	syncsource_sync = 3,
+	syncsource_none = 4
+} hdspm_syncsource;
+
+struct hdspm_status {
+	hdspm_io_type card_type;
+	hdspm_syncsource autosync_source;
+
+	unsigned long long card_clock;
+	unsigned int master_period;
+
+	union {
+		struct {
+			hdspm_sync sync_wc;
+			hdspm_sync sync_madi;
+			hdspm_sync sync_tco;
+			hdspm_sync sync_in;
+			hdspm_madi_input madi_input;
+			hdspm_madi_channel_format channel_format;
+			hdspm_madi_frame_format frame_format;
+		} madi;
+	} card_specific;
+};
+
+#define SNDRV_HDSPM_IOCTL_GET_STATUS \
+	_IOR('H', 0x47, struct hdspm_status)
+
+
+/**
+ * Get information about the card and its add-ons.
+ **/
+
+#define HDSPM_ADDON_TCO 1
 
 struct hdspm_version {
+	hdspm_io_type card_type;
+	char cardname[20];
+	unsigned int serial;
 	unsigned short firmware_rev;
+	int addons;
 };
 
 #define SNDRV_HDSPM_IOCTL_GET_VERSION _IOR('H', 0x43, struct hdspm_version)
@@ -103,7 +194,7 @@ struct hdspm_version {
 /* equivalent to hardware definition, maybe for future feature of mmap of
  * them
  */
-/* each of 64 outputs has 64 infader and 64 outfader: 
+/* each of 64 outputs has 64 infader and 64 outfader:
    Ins to Outs mixer[out].in[in], Outstreams to Outs mixer[out].pb[pb] */
 
 #define HDSPM_MIXER_CHANNELS HDSPM_MAX_CHANNELS
@@ -131,4 +222,277 @@ 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 */
+
+static char *texts_sync_status[] = {
+	"no lock",
+	"lock",
+	"sync"
+};
+
+static char *texts_ports_madi[] = {
+	"MADI.1", "MADI.2", "MADI.3", "MADI.4", "MADI.5", "MADI.6",
+	"MADI.7", "MADI.8", "MADI.9", "MADI.10", "MADI.11", "MADI.12",
+	"MADI.13", "MADI.14", "MADI.15", "MADI.16", "MADI.17", "MADI.18",
+	"MADI.19", "MADI.20", "MADI.21", "MADI.22", "MADI.23", "MADI.24",
+	"MADI.25", "MADI.26", "MADI.27", "MADI.28", "MADI.29", "MADI.30",
+	"MADI.31", "MADI.32", "MADI.33", "MADI.34", "MADI.35", "MADI.36",
+	"MADI.37", "MADI.38", "MADI.39", "MADI.40", "MADI.41", "MADI.42",
+	"MADI.43", "MADI.44", "MADI.45", "MADI.46", "MADI.47", "MADI.48",
+	"MADI.49", "MADI.50", "MADI.51", "MADI.52", "MADI.53", "MADI.54",
+	"MADI.55", "MADI.56", "MADI.57", "MADI.58", "MADI.59", "MADI.60",
+	"MADI.61", "MADI.62", "MADI.63", "MADI.64",
+};
+
+
+static char *texts_ports_raydat_ss[] = {
+	"ADAT1.1", "ADAT1.2", "ADAT1.3", "ADAT1.4", "ADAT1.5", "ADAT1.6",
+	"ADAT1.7", "ADAT1.8", "ADAT2.1", "ADAT2.2", "ADAT2.3", "ADAT2.4",
+	"ADAT2.5", "ADAT2.6", "ADAT2.7", "ADAT2.8", "ADAT3.1", "ADAT3.2",
+	"ADAT3.3", "ADAT3.4", "ADAT3.5", "ADAT3.6", "ADAT3.7", "ADAT3.8",
+	"ADAT4.1", "ADAT4.2", "ADAT4.3", "ADAT4.4", "ADAT4.5", "ADAT4.6",
+	"ADAT4.7", "ADAT4.8",
+	"AES.L", "AES.R",
+	"SPDIF.L", "SPDIF.R"
+};
+
+static char *texts_ports_raydat_ds[] = {
+	"ADAT1.1", "ADAT1.2", "ADAT1.3", "ADAT1.4",
+	"ADAT2.1", "ADAT2.2", "ADAT2.3", "ADAT2.4",
+	"ADAT3.1", "ADAT3.2", "ADAT3.3", "ADAT3.4",
+	"ADAT4.1", "ADAT4.2", "ADAT4.3", "ADAT4.4",
+	"AES.L", "AES.R",
+	"SPDIF.L", "SPDIF.R"
+};
+
+static char *texts_ports_raydat_qs[] = {
+	"ADAT1.1", "ADAT1.2",
+	"ADAT2.1", "ADAT2.2",
+	"ADAT3.1", "ADAT3.2",
+	"ADAT4.1", "ADAT4.2",
+	"AES.L", "AES.R",
+	"SPDIF.L", "SPDIF.R"
+};
+
+
+static char *texts_ports_aio_in_ss[] = {
+	"Analogue.L", "Analogue.R",
+	"AES.L", "AES.R",
+	"SPDIF.L", "SPDIF.R",
+	"ADAT.1", "ADAT.2", "ADAT.3", "ADAT.4", "ADAT.5", "ADAT.6",
+	"ADAT.7", "ADAT.8"
+};
+
+static char *texts_ports_aio_out_ss[] = {
+	"Analogue.L", "Analogue.R",
+	"AES.L", "AES.R",
+	"SPDIF.L", "SPDIF.R",
+	"ADAT.1", "ADAT.2", "ADAT.3", "ADAT.4", "ADAT.5", "ADAT.6",
+	"ADAT.7", "ADAT.8",
+	"Phone.L", "Phone.R"
+};
+
+static char *texts_ports_aio_in_ds[] = {
+	"Analogue.L", "Analogue.R",
+	"AES.L", "AES.R",
+	"SPDIF.L", "SPDIF.R",
+	"ADAT.1", "ADAT.2", "ADAT.3", "ADAT.4"
+};
+
+static char *texts_ports_aio_out_ds[] = {
+	"Analogue.L", "Analogue.R",
+	"AES.L", "AES.R",
+	"SPDIF.L", "SPDIF.R",
+	"ADAT.1", "ADAT.2", "ADAT.3", "ADAT.4",
+	"Phone.L", "Phone.R"
+};
+
+static char *texts_ports_aio_in_qs[] = {
+	"Analogue.L", "Analogue.R",
+	"AES.L", "AES.R",
+	"SPDIF.L", "SPDIF.R",
+	"ADAT.1", "ADAT.2", "ADAT.3", "ADAT.4"
+};
+
+static char *texts_ports_aio_out_qs[] = {
+	"Analogue.L", "Analogue.R",
+	"AES.L", "AES.R",
+	"SPDIF.L", "SPDIF.R",
+	"ADAT.1", "ADAT.2", "ADAT.3", "ADAT.4",
+	"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
+};
+
+
+#endif
-- 
1.7.2.3

[PATCH - [hdspm] 3/3] Add RayDAT and AIO strings to Kconfig

[Adrian Knoth]

Inform users about the newly added capabilities.

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

diff --git a/pci/Kconfig b/pci/Kconfig
index 7b2678a..5df9844 100644
--- a/pci/Kconfig
+++ b/pci/Kconfig
@@ -570,13 +570,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] 0/3] Support for RME RayDAT and AIO

[Takashi Iwai]

At Tue, 18 Jan 2011 15:12:16 +0100,
Adrian Knoth wrote:
> 
> Hi!
> 
> As announced a few days ago, I've integrated the offspring hdspm driver
> by Florian Faber. The new code now also supports the 36in/36out RME
> RayDAT cards and RME AIOs.

Thanks for the patch!

> I tried to follow the kernel's coding style as much as possible,
> however, there are still a few overlong lines left. For example, I
> didn't want to break "if read_reg(...) & 0x3f" at "& 0x3f", as it
> worsens readability. If it's required to comply, feel free to add the
> missing line breaks.

This is no big issue.  Would be better if the line fits in 80 chars,
but it's no must.

> Right now, RayDATs and AIOs can only operate on a single period size
> of 1024 samples, because these cards always use a fixed 16k buffer per
> channel and only vary interrupt frequency. I'm going to try to fix this
> after I've learned more about the PCM interface. ;) Any suggestions
> welcome.

Basically one patch should contain the changes that are build-clean,
i.e. you can build cleanly after patching.  Thus changing a header
and *.c in separate patches doesn't work well.

Also, please use the generic variable types as much as possible
for ioctl structs.  Avoid use of enum or such, especially ones that
are typedef'ed.  In general, avoid typedefs in the kernel code as much
as possible.  Defining each enum type is no-go.

Second, don't define arrays in the header file.  They must be in the
body, not included headers.

Also, remember checkpatch.pl is your friend.


thanks,

Takashi

Re: [PATCH - [hdspm] 1/3] Add headers for RayDAT and AIO support.

[Takashi Iwai]

At Tue, 18 Jan 2011 15:12:17 +0100,
Adrian Knoth wrote:
> 
> +struct hdspm_peak_rms {
> +		uint32_t input_peaks[64];
> +		uint32_t playback_peaks[64];
> +		uint32_t output_peaks[64];

Strange indentation level?

> +struct hdspm_status {
> +	hdspm_io_type card_type;
> +	hdspm_syncsource autosync_source;
> +
> +	unsigned long long card_clock;

Use more explicit int type with bit width.

> +static char *texts_sync_status[] = {
> +	"no lock",
> +	"lock",
> +	"sync"
> +};

Put these to *.c.


Takashi

Re: [PATCH - [hdspm] 2/3] Enable RME RayDAT and AIO

[Takashi Iwai]

At Tue, 18 Jan 2011 15:12:18 +0100,
Adrian Knoth wrote:
> 
> @@ -29,12 +42,13 @@
>  #include <linux/moduleparam.h>
>  #include <linux/slab.h>
>  #include <linux/pci.h>
> -#include <linux/math64.h>

No math64 stuff is necessary?

> +#include <linux/sysfs.h>

It's not implemented yet, so don't include it.

> -static DEFINE_PCI_DEVICE_TABLE(snd_hdspm_ids) = {
> +static struct pci_device_id snd_hdspm_ids[] __devinitdata = {

Why open-coded again?


> @@ -796,6 +1009,8 @@ static void hdspm_silence_playback(struct hdspm *hdspm)
>  	int n = hdspm->period_bytes;
>  	void *buf = hdspm->playback_buffer;
>  
> +	snd_printk(KERN_INFO "hdspm_silence_playback\n");

Don't put such a verbose debug message in frequently called callbacks.

>  static void hdspm_set_dds_value(struct hdspm *hdspm, int rate)
...
>  	n = div_u64(n, rate);
>  	/* n should be less than 2^32 for being written to FREQ register */
> -	snd_BUG_ON(n >> 32);
> +	snd_BUG_ON((n >> 32) == 0);

This check is consistent with the comment above...

The rest is too long to review.  Would be nice if you can split the
whole patch in logical orders...


thanks,

Takashi

Re: [PATCH - [hdspm] 1/3] Add headers for RayDAT and AIO support.

[Adrian Knoth]

On Tue, Jan 18, 2011 at 03:40:21PM +0100, Takashi Iwai wrote:

> Adrian Knoth wrote:
> > 
> > +struct hdspm_peak_rms {
> > +		uint32_t input_peaks[64];
> > +		uint32_t playback_peaks[64];
> > +		uint32_t output_peaks[64];
> Strange indentation level?

Indeed. Fixed.

> > +struct hdspm_status {
> > +	hdspm_io_type card_type;
> > +	hdspm_syncsource autosync_source;
> > +
> > +	unsigned long long card_clock;
> Use more explicit int type with bit width.

Done.

> > +static char *texts_sync_status[] = {
> > +	"no lock",
> > +	"lock",
> > +	"sync"
> > +};
> 
> Put these to *.c.

Looks like this array is actually unused. Let's see which of them can be
dropped or moved to *.c.

I'll have to cross-check with hdspmixer, some parts are referenced
there.


Cheers

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

Re: [PATCH - [hdspm] 2/3] Enable RME RayDAT and AIO

[Adrian Knoth]

On Tue, Jan 18, 2011 at 03:55:56PM +0100, Takashi Iwai wrote:

> > -#include <linux/math64.h>
> No math64 stuff is necessary?

Probably not. At least it works and compiles without. Perhaps
cross-included from somewhere else? I see div_u64 down there, so I guess
linux/math64.h should be included. Fixed.

> > +#include <linux/sysfs.h>
> It's not implemented yet, so don't include it.

ACK.

> > -static DEFINE_PCI_DEVICE_TABLE(snd_hdspm_ids) = {
> > +static struct pci_device_id snd_hdspm_ids[] __devinitdata = {
> Why open-coded again?

No particular reason. Now you mention it, I'll change it back.

How did it end up there?: I took the code from Florian's tarball. Looks
like there was some divergent development in the meantime (between his
fork and mainline kernel)

> > @@ -796,6 +1009,8 @@ static void hdspm_silence_playback(struct hdspm *hdspm)
> >  	int n = hdspm->period_bytes;
> >  	void *buf = hdspm->playback_buffer;
> >  
> > +	snd_printk(KERN_INFO "hdspm_silence_playback\n");
> Don't put such a verbose debug message in frequently called callbacks.

ACK. Removed.

> >  	n = div_u64(n, rate);
> >  	/* n should be less than 2^32 for being written to FREQ register */
> > -	snd_BUG_ON(n >> 32);
> > +	snd_BUG_ON((n >> 32) == 0);
> This check is consistent with the comment above...

Inconsistent I guess...?

I suggest to remove the line. We know n, we control rate, this should
never be an issue.


> The rest is too long to review. Would be nice if you can split the
> whole patch in logical orders...

I'm afraid I can't, because there's no logical order. There's just this
single hdspm.c (and .h) by Florian and my style fixes. No VCS.

So whatever is wrong needs to be spotted by some experienced ALSA dev. I
guess you'd be faster fixing it instead of mailing me all the things
that are wrong, but I'm happy to change things you point out.


Cheers

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

Re: [PATCH - [hdspm] 0/3] Support for RME RayDAT and AIO

[Adrian Knoth]

On 01/18/11 15:36, Takashi Iwai wrote:

Hi!

> Basically one patch should contain the changes that are build-clean,
> i.e. you can build cleanly after patching.  Thus changing a header
> and *.c in separate patches doesn't work well.

Ok. Better luck next time. ;) Speaking of which: should I resend
everything as a whole-in-one patch?

I could also provide a download link (given we're talking 170+kb here)

> Also, please use the generic variable types as much as possible
> for ioctl structs. Avoid use of enum or such, especially ones that
> are typedef'ed. In general, avoid typedefs in the kernel code as much
> as possible. Defining each enum type is no-go.

If've dropped all the new typedefs and only kept the existing ones now.
However, it still reads "enum foo something" in the structs for some
ioctls.

Is this acceptable or do I have to change this to int? If so, would I
manually add a couple of #defines? I somehow feel losing the semantic
connection to the enum is bad, but this might OTOH be intended, e.g. for
the sake of a stable interface.

> Second, don't define arrays in the header file. They must be in the
> body, not included headers.

Cross-checked with hdspmixer and moved to .c, except for the channel
mappings as they're included from userspace. (channel_map*)

> Also, remember checkpatch.pl is your friend.

It already was for the very last days. But I'm more excited about my new
friend Lindent. ;)


Cheers

Re: [PATCH - [hdspm] 0/3] Support for RME RayDAT and AIO

[Takashi Iwai]

At Tue, 18 Jan 2011 18:06:00 +0100,
Adrian Knoth wrote:
> 
> On 01/18/11 15:36, Takashi Iwai wrote:
> 
> Hi!
> 
> > Basically one patch should contain the changes that are build-clean,
> > i.e. you can build cleanly after patching.  Thus changing a header
> > and *.c in separate patches doesn't work well.
> 
> Ok. Better luck next time. ;) Speaking of which: should I resend
> everything as a whole-in-one patch?

That's OK.  At best, split a patch into some logical pieces
(i.e. MIDI handling changes, addition of RayDAT/AIO support,
code-refactoring, etc.)  But if this is hard to achieve, just "the big
one" is enough.

> I could also provide a download link (given we're talking 170+kb here)

This should be avoided.  The link content isn't archived, so we'll
loose the code history.  And it makes difficult to review.

> > Also, please use the generic variable types as much as possible
> > for ioctl structs. Avoid use of enum or such, especially ones that
> > are typedef'ed. In general, avoid typedefs in the kernel code as much
> > as possible. Defining each enum type is no-go.
> 
> If've dropped all the new typedefs and only kept the existing ones now.
> However, it still reads "enum foo something" in the structs for some
> ioctls.
> 
> Is this acceptable or do I have to change this to int? If so, would I
> manually add a couple of #defines?

You can use int (short or whatever) and keep enum as its values.

> I somehow feel losing the semantic
> connection to the enum is bad, but this might OTOH be intended, e.g. for
> the sake of a stable interface.

Exactly.

enum itself is nice to use, but the problem is that its size isn't
defined strictly, rather depending on its contents.  So, for ioctl,
types with fixed size is better.

> > Second, don't define arrays in the header file. They must be in the
> > body, not included headers.
> 
> Cross-checked with hdspmixer and moved to .c, except for the channel
> mappings as they're included from userspace. (channel_map*)

OK.

> > Also, remember checkpatch.pl is your friend.
> 
> It already was for the very last days. But I'm more excited about my new
> friend Lindent. ;)

Heh, but sometimes the sense of beauty conflicts between Lindent and
human being ;)  I recommend you to re-read the code after running it.


thanks,

Takashi

Re: Process of sumit the Audio driver code to ALSA

[Reddy, MR Swami]

Thanks for quick response!
Yes, it  ASOC based and currently, the audio drivers supported for Samsung 6410 target.

Thanks
Swami

-----Original Message-----
From: Takashi Iwai [mailto:tiwai@suse.de] 
Sent: Wednesday, January 19, 2011 12:39 PM
To: Reddy, MR Swami
Subject: Re: Process of sumit the Audio driver code to ALSA

At Tue, 18 Jan 2011 20:58:28 -0800,
Reddy, MR Swami wrote:
> 
> Hello,
> We (National Semi) have developed ALSA based audio drivers for our Audio subsystem. Could you please let me know the process to submit and include as part of ALSA project.
> 
> The ALSA audio driver version info as follows:
>   - Linux - 2.6.24 base
>   - Andorid- 2.1   base

Is this based on ASoC?  If not, I recommend you to port the code to ASoC, thus it's the standard for embedded devices.
If you have any questions, just post to alsa-devel ML.


thanks,

Takashi

Re: Process of sumit the Audio driver code to ALSA

[Takashi Iwai]

At Tue, 18 Jan 2011 23:20:15 -0800,
Reddy, MR Swami wrote:
> 
> Thanks for quick response!
> Yes, it  ASOC based and currently, the audio drivers supported for Samsung 6410 target.

OK, then just post your patches to alsa-devel ML, Cc'ed to ASoC
maintainers (Liam Girdwood <lrg@slimlogic.co.uk> and
 Mark Brown <broonie@opensource.wolfsonmicro.com>).

But I guess there have been quite many changes in ASoC core
(e.g. multi-core support) that are no longer compatible with
2.6.24-based code.  These have to be fixed, of course, before
merging.

It's up to you -- you can first pot the original patch and ask some
hints for forward-porting, or first start reading the latest ASoC API
changes, do port your driver then post the revised patch.


thanks,

Takashi

> 
> Thanks
> Swami
> 
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de] 
> Sent: Wednesday, January 19, 2011 12:39 PM
> To: Reddy, MR Swami
> Subject: Re: Process of sumit the Audio driver code to ALSA
> 
> At Tue, 18 Jan 2011 20:58:28 -0800,
> Reddy, MR Swami wrote:
> > 
> > Hello,
> > We (National Semi) have developed ALSA based audio drivers for our Audio subsystem. Could you please let me know the process to submit and include as part of ALSA project.
> > 
> > The ALSA audio driver version info as follows:
> >   - Linux - 2.6.24 base
> >   - Andorid- 2.1   base
> 
> Is this based on ASoC?  If not, I recommend you to port the code to ASoC, thus it's the standard for embedded devices.
> If you have any questions, just post to alsa-devel ML.
> 
> 
> thanks,
> 
> Takashi
> 
> 

Re: Process of sumit the Audio driver code to ALSA

[Seungwhan Youn]

Hi,

On Wed, Jan 19, 2011 at 4:20 PM, Reddy, MR Swami <MR.Swami.Reddy@nsc.com> wrote:
> Thanks for quick response!
> Yes, it  ASOC based and currently, the audio drivers supported for Samsung 6410 target.

If you are trying to submit your new ASoC driver, you may work based
on Mr. Mark Brown's ASoC git repository
(git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6.git).
And also, if your driver works on Samsung 6410 target,
'sound/soc/samsung/smdk_wm8580.c' and
'sound/soc/samsung/smdk_wm9713.c' on 'for-2.6.39' (or 'for-next')
branch will give you some hints. ;)

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

Re: [PATCH - [hdspm] 2/3] Enable RME RayDAT and AIO

[Takashi Iwai]

At Tue, 18 Jan 2011 16:24:08 +0100,
Adrian Knoth wrote:
> 
> > > -static DEFINE_PCI_DEVICE_TABLE(snd_hdspm_ids) = {
> > > +static struct pci_device_id snd_hdspm_ids[] __devinitdata = {
> > Why open-coded again?
> 
> No particular reason. Now you mention it, I'll change it back.

I guess this was changed in the upstream from the version Florian's
code based on.

> How did it end up there?: I took the code from Florian's tarball. Looks
> like there was some divergent development in the meantime (between his
> fork and mainline kernel)

Yes, it's always bad to fork in such a way.  If forking, it should track
also the upstream changes (which can be better done via git).


> > >  	n = div_u64(n, rate);
> > >  	/* n should be less than 2^32 for being written to FREQ register */
> > > -	snd_BUG_ON(n >> 32);
> > > +	snd_BUG_ON((n >> 32) == 0);
> > This check is consistent with the comment above...
> 
> Inconsistent I guess...?

Yep.

> I suggest to remove the line. We know n, we control rate, this should
> never be an issue.

Fair enough.

> > The rest is too long to review. Would be nice if you can split the
> > whole patch in logical orders...
> 
> I'm afraid I can't, because there's no logical order. There's just this
> single hdspm.c (and .h) by Florian and my style fixes. No VCS.

Too bad.

> So whatever is wrong needs to be spotted by some experienced ALSA dev. I
> guess you'd be faster fixing it instead of mailing me all the things
> that are wrong, but I'm happy to change things you point out.

Thanks.

Could you repost the fixed patches?  Don't forget to put me to Cc
(or send me and put alsa-devel ML to Cc), so that it reaches to me
directly.  I don't look through ML so regularly, and alsa-devel ML
blocks the posts from non-subscribers or ones with too large contents
for approvals.


Takashi

Re: Process of sumit the Audio driver code to ALSA

[Reddy, MR Swami]

Hi Takashi,

OK. I will submit the patches for National's (NSC) LM49352 and LM49350 audio cards (with older version of ALSA and Linux 2.6.24 tested). And request for the forward porting guidelines.

Thanks
Swami

-----Original Message-----
From: Takashi Iwai [mailto:tiwai@suse.de] 
Sent: Wednesday, January 19, 2011 1:07 PM
To: Reddy, MR Swami
Cc: alsa-devel@alsa-project.org
Subject: Re: Process of sumit the Audio driver code to ALSA

At Tue, 18 Jan 2011 23:20:15 -0800,
Reddy, MR Swami wrote:
> 
> Thanks for quick response!
> Yes, it  ASOC based and currently, the audio drivers supported for Samsung 6410 target.

OK, then just post your patches to alsa-devel ML, Cc'ed to ASoC maintainers (Liam Girdwood <lrg@slimlogic.co.uk> and  Mark Brown <broonie@opensource.wolfsonmicro.com>).

But I guess there have been quite many changes in ASoC core (e.g. multi-core support) that are no longer compatible with 2.6.24-based code.  These have to be fixed, of course, before merging.

It's up to you -- you can first pot the original patch and ask some hints for forward-porting, or first start reading the latest ASoC API changes, do port your driver then post the revised patch.


thanks,

Takashi

> 
> Thanks
> Swami
> 
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, January 19, 2011 12:39 PM
> To: Reddy, MR Swami
> Subject: Re: Process of sumit the Audio driver code to ALSA
> 
> At Tue, 18 Jan 2011 20:58:28 -0800,
> Reddy, MR Swami wrote:
> > 
> > Hello,
> > We (National Semi) have developed ALSA based audio drivers for our Audio subsystem. Could you please let me know the process to submit and include as part of ALSA project.
> > 
> > The ALSA audio driver version info as follows:
> >   - Linux - 2.6.24 base
> >   - Andorid- 2.1   base
> 
> Is this based on ASoC?  If not, I recommend you to port the code to ASoC, thus it's the standard for embedded devices.
> If you have any questions, just post to alsa-devel ML.
> 
> 
> thanks,
> 
> Takashi
> 
>