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

[Jyri Sarha <jsarha@ti.com>]

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);
>>

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