Re: [PATCH 01/11] snd: iec958: split status creation and fill

From: Takashi Iwai
Date: Tue May 25 2021 - 03:34:07 EST

On Fri, 07 May 2021 16:03:24 +0200,
Maxime Ripard wrote:
>
> In some situations, like a codec probe, we need to provide an IEC status
> default but don't have access to the sampling rate and width yet since
> no stream has been configured yet.
>
> Each and every driver has its own default, whereas the core iec958 code
> also has some buried in the snd_pcm_create_iec958_consumer functions.
>
> Let's split these functions in two to provide a default that doesn't
> rely on the sampling rate and width, and another function to fill them
> when available.
>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>

The changes look almost good, but please use EXPORT_SYMBOL_GPL() for
newly introduced symbols. Also, it'd be worth to mention that some
bits update are done only for the default values; if a rate value has
been already set, it won't be overridden by this *_fill_*() call,
that's the intentional behavior, right?

Last but not least, the subject prefix should be "ALSA:" in general :)


thanks,

Takashi

> ---
> include/sound/pcm_iec958.h | 8 +++
> sound/core/pcm_iec958.c | 131 +++++++++++++++++++++++++------------
> 2 files changed, 96 insertions(+), 43 deletions(-)
>
> diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h
> index 0939aa45e2fe..64e84441cde1 100644
> --- a/include/sound/pcm_iec958.h
> +++ b/include/sound/pcm_iec958.h
> @@ -4,6 +4,14 @@
>
> #include <linux/types.h>
>
> +int snd_pcm_create_iec958_consumer_default(u8 *cs, size_t len);
> +
> +int snd_pcm_fill_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> + size_t len);
> +
> +int snd_pcm_fill_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
> + u8 *cs, size_t len);
> +
> int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> size_t len);
>
> diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
> index f9a211cc1f2c..a60908efe159 100644
> --- a/sound/core/pcm_iec958.c
> +++ b/sound/core/pcm_iec958.c
> @@ -9,41 +9,68 @@
> #include <sound/pcm_params.h>
> #include <sound/pcm_iec958.h>
>
> -static int create_iec958_consumer(uint rate, uint sample_width,
> - u8 *cs, size_t len)
> +int snd_pcm_create_iec958_consumer_default(u8 *cs, size_t len)
> {
> - unsigned int fs, ws;
> -
> if (len < 4)
> return -EINVAL;
>
> - switch (rate) {
> - case 32000:
> - fs = IEC958_AES3_CON_FS_32000;
> - break;
> - case 44100:
> - fs = IEC958_AES3_CON_FS_44100;
> - break;
> - case 48000:
> - fs = IEC958_AES3_CON_FS_48000;
> - break;
> - case 88200:
> - fs = IEC958_AES3_CON_FS_88200;
> - break;
> - case 96000:
> - fs = IEC958_AES3_CON_FS_96000;
> - break;
> - case 176400:
> - fs = IEC958_AES3_CON_FS_176400;
> - break;
> - case 192000:
> - fs = IEC958_AES3_CON_FS_192000;
> - break;
> - default:
> + memset(cs, 0, len);
> +
> + cs[0] = IEC958_AES0_CON_NOT_COPYRIGHT | IEC958_AES0_CON_EMPHASIS_NONE;
> + cs[1] = IEC958_AES1_CON_GENERAL;
> + cs[2] = IEC958_AES2_CON_SOURCE_UNSPEC | IEC958_AES2_CON_CHANNEL_UNSPEC;
> + cs[3] = IEC958_AES3_CON_CLOCK_1000PPM | IEC958_AES3_CON_FS_NOTID;
> +
> + if (len > 4)
> + cs[4] = IEC958_AES4_CON_WORDLEN_NOTID;
> +
> + return len;
> +}
> +EXPORT_SYMBOL(snd_pcm_create_iec958_consumer_default);
> +
> +static int fill_iec958_consumer(uint rate, uint sample_width,
> + u8 *cs, size_t len)
> +{
> + if (len < 4)
> return -EINVAL;
> +
> + if ((cs[3] & IEC958_AES3_CON_FS) == IEC958_AES3_CON_FS_NOTID) {
> + unsigned int fs;
> +
> + switch (rate) {
> + case 32000:
> + fs = IEC958_AES3_CON_FS_32000;
> + break;
> + case 44100:
> + fs = IEC958_AES3_CON_FS_44100;
> + break;
> + case 48000:
> + fs = IEC958_AES3_CON_FS_48000;
> + break;
> + case 88200:
> + fs = IEC958_AES3_CON_FS_88200;
> + break;
> + case 96000:
> + fs = IEC958_AES3_CON_FS_96000;
> + break;
> + case 176400:
> + fs = IEC958_AES3_CON_FS_176400;
> + break;
> + case 192000:
> + fs = IEC958_AES3_CON_FS_192000;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + cs[3] &= ~IEC958_AES3_CON_FS;
> + cs[3] |= fs;
> }
>
> - if (len > 4) {
> + if (len > 4 &&
> + (cs[4] & IEC958_AES4_CON_WORDLEN) == IEC958_AES4_CON_WORDLEN_NOTID) {
> + unsigned int ws;
> +
> switch (sample_width) {
> case 16:
> ws = IEC958_AES4_CON_WORDLEN_20_16;
> @@ -64,21 +91,30 @@ static int create_iec958_consumer(uint rate, uint sample_width,
> default:
> return -EINVAL;
> }
> +
> + cs[4] &= ~IEC958_AES4_CON_WORDLEN;
> + cs[4] |= ws;
> }
>
> - memset(cs, 0, len);
> -
> - cs[0] = IEC958_AES0_CON_NOT_COPYRIGHT | IEC958_AES0_CON_EMPHASIS_NONE;
> - cs[1] = IEC958_AES1_CON_GENERAL;
> - cs[2] = IEC958_AES2_CON_SOURCE_UNSPEC | IEC958_AES2_CON_CHANNEL_UNSPEC;
> - cs[3] = IEC958_AES3_CON_CLOCK_1000PPM | fs;
> -
> - if (len > 4)
> - cs[4] = ws;
> -
> return len;
> }
>
> +int snd_pcm_fill_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
> + u8 *cs, size_t len)
> +{
> + return fill_iec958_consumer(params_rate(params), params_width(params), cs, len);
> +}
> +EXPORT_SYMBOL(snd_pcm_fill_iec958_consumer_hw_params);
> +
> +int snd_pcm_fill_iec958_consumer(struct snd_pcm_runtime *runtime,
> + u8 *cs, size_t len)
> +{
> + return fill_iec958_consumer(runtime->rate,
> + snd_pcm_format_width(runtime->format),
> + cs, len);
> +}
> +EXPORT_SYMBOL(snd_pcm_fill_iec958_consumer);
> +
> /**
> * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
> * @runtime: pcm runtime structure with ->rate filled in
> @@ -95,9 +131,13 @@ static int create_iec958_consumer(uint rate, uint sample_width,
> 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);
> + int ret;
> +
> + ret = snd_pcm_create_iec958_consumer_default(cs, len);
> + if (ret < 0)
> + return ret;
> +
> + return snd_pcm_fill_iec958_consumer(runtime, cs, len);
> }
> EXPORT_SYMBOL(snd_pcm_create_iec958_consumer);
>
> @@ -117,7 +157,12 @@ 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);
> + int ret;
> +
> + ret = snd_pcm_create_iec958_consumer_default(cs, len);
> + if (ret < 0)
> + return ret;
> +
> + return fill_iec958_consumer(params_rate(params), params_width(params), cs, len);
> }
> EXPORT_SYMBOL(snd_pcm_create_iec958_consumer_hw_params);
> --
> 2.31.1
>