Re: [PATCH 1/8] ALSA: pcm: add a helper function to constrain mask-type parameters

[Takashi Iwai <tiwai@suse.de>]

On Thu, 08 Jun 2017 01:10:19 +0200,
Takashi Sakamoto wrote:
> 
> Application of constraints to mask-type parameters for PCM substream is
> done in a call of snd_pcm_hw_refine(), while the function includes much
> codes and is not enough friendly to readers.
> 
> This commit splits the codes to a separated function so that readers can
> get it easily. I leave desicion into compilers to merge the function into
> its callee.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/core/pcm_native.c | 57 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 2bde07a4a87f..b3e8aab3915e 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -253,6 +253,40 @@ static bool hw_support_mmap(struct snd_pcm_substream *substream)
>  	return true;
>  }
>  
> +static int constrain_mask_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *params)
> +{
> +	struct snd_pcm_hw_constraints *constrs =
> +					&substream->runtime->hw_constraints;
> +	struct snd_mask *m;
> +	unsigned int k;
> +	int changed;
> +

A strange blank line is here.

> +	struct snd_mask __maybe_unused old_mask;

Do we really need __maybe_unused?  Drop it as much as possible.
IMO, it can be uglier than ifdef, since you don't know why it's
unused.  With ifdef, at least, you have an idea about the condition.


thanks,

Takashi

> +
> +	for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) {
> +		m = hw_param_mask(params, k);
> +		if (snd_mask_empty(m))
> +			return -EINVAL;
> +		if (!(params->rmask & (1 << k)))
> +			continue;
> +
> +		if (trace_hw_mask_param_enabled())
> +			old_mask = *m;
> +
> +		changed = snd_mask_refine(m, constrs_mask(constrs, k));
> +
> +		trace_hw_mask_param(substream, k, 0, &old_mask, m);
> +
> +		if (changed)
> +			params->cmask |= 1 << k;
> +		if (changed < 0)
> +			return changed;
> +	}
> +
> +	return 0;
> +}
> +
>  int snd_pcm_hw_refine(struct snd_pcm_substream *substream, 
>  		      struct snd_pcm_hw_params *params)
>  {
> @@ -265,6 +299,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
>  	unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1];
>  	unsigned int stamp = 2;
>  	int changed, again;
> +	int err;
>  
>  	struct snd_mask __maybe_unused old_mask;
>  	struct snd_interval __maybe_unused old_interval;
> @@ -278,25 +313,9 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
>  		params->rate_den = 0;
>  	}
>  
> -	for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) {
> -		m = hw_param_mask(params, k);
> -		if (snd_mask_empty(m))
> -			return -EINVAL;
> -		if (!(params->rmask & (1 << k)))
> -			continue;
> -
> -		if (trace_hw_mask_param_enabled())
> -			old_mask = *m;
> -
> -		changed = snd_mask_refine(m, constrs_mask(constrs, k));
> -
> -		trace_hw_mask_param(substream, k, 0, &old_mask, m);
> -
> -		if (changed)
> -			params->cmask |= 1 << k;
> -		if (changed < 0)
> -			return changed;
> -	}
> +	err = constrain_mask_params(substream, params);
> +	if (err < 0)
> +		return err;
>  
>  	for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) {
>  		i = hw_param_interval(params, k);
> -- 
> 2.11.0
>