Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 25 May 2021 18:01:16 +0200
Message-ID: <s5hmtsiby5v.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Maxime Ripard <maxime@cerno.tech>
Cc:     Jaroslav Kysela <perex@perex.cz>, Mark Brown <broonie@kernel.org>,
        dri-devel@lists.freedesktop.org,
        Daniel Vetter <daniel.vetter@intel.com>,
        David Airlie <airlied@linux.ie>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Takashi Iwai <tiwai@suse.com>, devicetree@vger.kernel.org,
        Dave Stevenson <dave.stevenson@raspberrypi.com>,
        Phil Elwell <phil@raspberrypi.com>,
        Tim Gover <tim.gover@raspberrypi.com>,
        Dom Cobley <dom@raspberrypi.com>, linux-doc@vger.kernel.org,
        Eric Anholt <eric@anholt.net>,
        Nicolas Saenz Julienne <nsaenzjulienne@kernel.org>,
        linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org,
        Maxime Ripard <mripard@kernel.org>,
        Nicolas Saenz Julienne <nsaenz@kernel.org>,
        bcm-kernel-feedback-list@broadcom.com,
        linux-arm-kernel@lists.infradead.org,
        Daniel Vetter <daniel@ffwll.ch>,
        linux-rpi-kernel@lists.infradead.org,
        Jonathan Corbet <corbet@lwn.net>,
        Rob Herring <robh+dt@kernel.org>
Subject: Re: [PATCH v2 02/12] ALSA: iec958: Split status creation and fill
In-Reply-To: <20210525132354.297468-3-maxime@cerno.tech>
References: <20210525132354.297468-1-maxime@cerno.tech>
        <20210525132354.297468-3-maxime@cerno.tech>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Tue, 25 May 2021 15:23:44 +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@cerno.tech>

Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

> ---
>  include/sound/pcm_iec958.h |   8 ++
>  sound/core/pcm_iec958.c    | 176 ++++++++++++++++++++++++++++---------
>  2 files changed, 141 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..7a1b816f67cc 100644
> --- a/sound/core/pcm_iec958.c
> +++ b/sound/core/pcm_iec958.c
> @@ -9,41 +9,85 @@
>  #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)
> +/**
> + * snd_pcm_create_iec958_consumer_default - create default consumer format IEC958 channel status
> + * @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. When relevant, the configuration-dependant bits will be set as
> + * unspecified.
> + *
> + * Drivers should then call einter snd_pcm_fill_iec958_consumer() or
> + * snd_pcm_fill_iec958_consumer_hw_params() to replace these unspecified
> + * bits by their actual values.
> + *
> + * 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_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_GPL(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 +108,58 @@ 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;
>  }
>  
> +/**
> + * snd_pcm_fill_iec958_consumer - Fill 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
> + *
> + * Fill the unspecified bits in an IEC958 status bits array using the
> + * parameters of the PCM runtime @runtime.
> + *
> + * Drivers may wish to tweak the contents of the buffer after its been
> + * filled.
> + *
> + * Returns: length of buffer, or negative error code if something failed.
> + */
> +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_GPL(snd_pcm_fill_iec958_consumer);
> +
> +/**
> + * snd_pcm_fill_iec958_consumer_hw_params - Fill consumer format IEC958 channel status
> + * @params: the hw_params instance for extracting rate and sample format
> + * @cs: channel status buffer, at least four bytes
> + * @len: length of channel status buffer
> + *
> + * Fill the unspecified bits in an IEC958 status bits array using the
> + * parameters of the PCM hardware parameters @params.
> + *
> + * Drivers may wish to tweak the contents of the buffer after its been
> + * filled..
> + *
> + * Returns: length of buffer, or negative error code if something failed.
> + */
> +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_GPL(snd_pcm_fill_iec958_consumer_hw_params);
> +
>  /**
>   * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
>   * @runtime: pcm runtime structure with ->rate filled in
> @@ -95,9 +176,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 +202,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
>