diff mbox

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

Message ID 3d9fbdac702b93742841221e3e8d2468a3d18f12.1457465383.git.jsarha@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jyri Sarha March 8, 2016, 8:14 p.m. UTC
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@ti.com>
---
 include/sound/pcm_iec958.h |  2 ++
 sound/core/pcm_iec958.c    | 53 +++++++++++++++++++++++++++++++---------------
 2 files changed, 38 insertions(+), 17 deletions(-)

Comments

Arnaud POULIQUEN March 9, 2016, 10:08 a.m. UTC | #1
Hello Jyri,

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

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@ti.com>
> ---
>  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?
> +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
> +					     u8 *cs, size_t len)
should be aligned on parenthesis
> +{
> +	return create_iec958_consumer(params_rate(params), params_width(params),
> +				      cs, len);
> +}
> +EXPORT_SYMBOL(snd_pcm_create_iec958_consumer_hw_params);
>
Jyri Sarha March 9, 2016, 2:38 p.m. UTC | #2
On 03/09/16 12:08, Arnaud Pouliquen wrote:
> Hello Jyri,
>
> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>
> 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@ti.com>
>> ---
>>   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);
>>
Mark Brown March 10, 2016, 5:24 a.m. UTC | #3
On Tue, Mar 08, 2016 at 10:14:25PM +0200, 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.

Takashi, are you OK with this one?
diff mbox

Patch

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)
+{
+	return create_iec958_consumer(runtime->rate,
+				      snd_pcm_format_width(runtime->format),
+				      cs, len);
+}
 EXPORT_SYMBOL(snd_pcm_create_iec958_consumer);
+
+
+int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
+					     u8 *cs, size_t len)
+{
+	return create_iec958_consumer(params_rate(params), params_width(params),
+				      cs, len);
+}
+EXPORT_SYMBOL(snd_pcm_create_iec958_consumer_hw_params);