Re: [PATCH v6 1/6] ALSA: pcm: add IEC958 channel status helper for hw_params

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/09/16 12:08, Arnaud Pouliquen wrote:
Hello Jyri,

Acked-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxx>

With few nitpicking remarks on form.

Regards
Arnaud

On 03/08/2016 09:14 PM, Jyri Sarha wrote:
Add IEC958 channel status helper that gets the audio properties from
snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to
produce the channel status bits already in audio stream configuration
phase.

Signed-off-by: Jyri Sarha <jsarha@xxxxxx>
---
  include/sound/pcm_iec958.h |  2 ++
  sound/core/pcm_iec958.c    | 53 +++++++++++++++++++++++++++++++---------------
  2 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h
index 0eed397..36f023a 100644
--- a/include/sound/pcm_iec958.h
+++ b/include/sound/pcm_iec958.h
@@ -6,4 +6,6 @@
  int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
  	size_t len);

+int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
+					     u8 *cs, size_t len);
  #endif
diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
index 36b2d7a..27e0981 100644
--- a/sound/core/pcm_iec958.c
+++ b/sound/core/pcm_iec958.c
@@ -9,30 +9,18 @@
  #include <linux/types.h>
  #include <sound/asoundef.h>
  #include <sound/pcm.h>
+#include <sound/pcm_params.h>
  #include <sound/pcm_iec958.h>

-/**
- * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
- * @runtime: pcm runtime structure with ->rate filled in
- * @cs: channel status buffer, at least four bytes
- * @len: length of channel status buffer
- *
- * Create the consumer format channel status data in @cs of maximum size
- * @len corresponding to the parameters of the PCM runtime @runtime.
- *
- * Drivers may wish to tweak the contents of the buffer after creation.
- *
- * Returns: length of buffer, or negative error code if something failed.
- */
-int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
-	size_t len)
+static int create_iec958_consumer(uint rate, uint sample_width,
+				  u8 *cs, size_t len)
  {
  	unsigned int fs, ws;

  	if (len < 4)
  		return -EINVAL;

-	switch (runtime->rate) {
+	switch (rate) {
  	case 32000:
  		fs = IEC958_AES3_CON_FS_32000;
  		break;
@@ -59,7 +47,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
  	}

  	if (len > 4) {
-		switch (snd_pcm_format_width(runtime->format)) {
+		switch (sample_width) {
  		case 16:
  			ws = IEC958_AES4_CON_WORDLEN_20_16;
  			break;
@@ -71,6 +59,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
  			     IEC958_AES4_CON_MAX_WORDLEN_24;
  			break;
  		case 24:
+		case 32: /* Assume 24-bit width for 32-bit samples. */
  			ws = IEC958_AES4_CON_WORDLEN_24_20 |
  			     IEC958_AES4_CON_MAX_WORDLEN_24;
  			break;
@@ -92,4 +81,34 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,

  	return len;
  }
+
+/**
+ * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
+ * @runtime: pcm runtime structure with ->rate filled in
+ * @cs: channel status buffer, at least four bytes
+ * @len: length of channel status buffer
+ *
+ * Create the consumer format channel status data in @cs of maximum size
+ * @len corresponding to the parameters of the PCM runtime @runtime.
+ *
+ * Drivers may wish to tweak the contents of the buffer after creation.
+ *
+ * Returns: length of buffer, or negative error code if something failed.
+ */
+int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
+	size_t len)
should be aligned on parenthesis
+{
+	return create_iec958_consumer(runtime->rate,
+				      snd_pcm_format_width(runtime->format),
+				      cs, len);
+}
  EXPORT_SYMBOL(snd_pcm_create_iec958_consumer);
+
+
No kernel-doc comment?

Oh yes. It'll be mostly redundant with snd_pcm_create_iec958_consumer() but still it should be there.

+int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
Could be snd_pcm_hw_params_create_iec958_consumer to be coherent with
functions declared in pcm.h

I feel that iec958 should be earlier in the name because it is the common denominator in this c-file. Then again I have no strong opinions about this.

+					     u8 *cs, size_t len)
should be aligned on parenthesis

But it is. It just that the diff '+'-sign pushes the line above one char right, but the line bellow is not pushed because it indented with tabs (like it should be).

+{
+	return create_iec958_consumer(params_rate(params), params_width(params),
+				      cs, len);
+}
+EXPORT_SYMBOL(snd_pcm_create_iec958_consumer_hw_params);


_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux