[PATCH] ASoC: fsl_asrc: add protection for the asrc of older version

There is a constraint for the channel number setting on the
asrc of older version (e.g. imx35), the channel number should
be even, odd number isn't valid.

So add protection when the asrc of older version is used.

Signed-off-by: Shengjiu Wang <redacted>
---
 sound/soc/fsl/fsl_asrc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 528e8b108422..b3b3c5e15ef1 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c

@@ -109,7 +109,8 @@ static int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair)
 	if (index == ASRC_INVALID_PAIR) {
 		dev_err(dev, "all pairs are busy now\n");
 		ret = -EBUSY;
-	} else if (asrc_priv->channel_avail < channels) {
+	} else if (asrc_priv->channel_avail < channels ||
+		(asrc_priv->channel_bits < 4 && channels % 2 != 0)) {
 		dev_err(dev, "can't afford required channels: %d\n", channels);
 		ret = -EINVAL;
 	} else {

-- 
1.9.1

Hi Shengjiu,

On Fri, Mar 01, 2019 at 02:32:38AM +0000, S.j. Wang wrote:

I feel it'd be better to have another else-if, since the existing
error message is against something else.

+	} else if (asrc_priv->channel_bits < 4 && channels & 1) {
+		/* old version of ASRC has channel_bits = 3 */
+  		dev_err(dev, "does not support odd channel number\n");
+  		ret = -EINVAL;

Alternatively, I feel instead of error-out at here, should we add
a HW constraint or at least fence it off at the beginning of the
hw_params()? This is actually nothing specific to the pair-request
function but a hardware constraint.

Hi

Hi Shengjiu,

On Fri, Mar 01, 2019 at 02:32:38AM +0000, S.j. Wang wrote:

quoted

There is a constraint for the channel number setting on the asrc of
older version (e.g. imx35), the channel number should be even, odd
number isn't valid.

So add protection when the asrc of older version is used.

Signed-off-by: Shengjiu Wang <redacted>
---
 sound/soc/fsl/fsl_asrc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index

528e8b108422..b3b3c5e15ef1 100644

--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c

@@ -109,7 +109,8 @@ static int fsl_asrc_request_pair(int channels, struct

fsl_asrc_pair *pair)

quoted

 	if (index == ASRC_INVALID_PAIR) {
 		dev_err(dev, "all pairs are busy now\n");
 		ret = -EBUSY;
-	} else if (asrc_priv->channel_avail < channels) {
+	} else if (asrc_priv->channel_avail < channels ||
+		(asrc_priv->channel_bits < 4 && channels % 2 != 0)) {
 		dev_err(dev, "can't afford required channels: %d\n",

channels);

I feel it'd be better to have another else-if, since the existing error message
is against something else.

+	} else if (asrc_priv->channel_bits < 4 && channels & 1) {
+		/* old version of ASRC has channel_bits = 3 */
+  		dev_err(dev, "does not support odd channel number\n");
+  		ret = -EINVAL;

Alternatively, I feel instead of error-out at here, should we add a HW
constraint or at least fence it off at the beginning of the hw_params()? This
is actually nothing specific to the pair-request function but a hardware
constraint.

How about add constraint in startup?
static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream,
                               struct snd_soc_dai *dai)
{
       struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);

       if (asrc_priv->channel_bits == 3) {
               snd_pcm_hw_constraint_step(substream->runtime, 0,
                                          SNDRV_PCM_HW_PARAM_CHANNELS, 2);
       }

       return 0;
}

Best regards
Wang shengjiu

On Fri, Mar 01, 2019 at 06:55:25AM +0000, S.j. Wang wrote:

quoted

Alternatively, I feel instead of error-out at here, should we add a HW
constraint or at least fence it off at the beginning of the hw_params()? This
is actually nothing specific to the pair-request function but a hardware
constraint.

How about add constraint in startup?
static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream,
                               struct snd_soc_dai *dai)
{
       struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);

       if (asrc_priv->channel_bits == 3) {
               snd_pcm_hw_constraint_step(substream->runtime, 0,
                                          SNDRV_PCM_HW_PARAM_CHANNELS, 2);
       }

       return 0;
}

Yea, that looks good to me. Better to have a line of comments
to tell that "bits==3" means older version -- maybe we should
have something much more clear than using channel_bits but it
is fine for now since they only differ here.

Hi

On Fri, Mar 01, 2019 at 06:55:25AM +0000, S.j. Wang wrote:

quoted

quoted

Alternatively, I feel instead of error-out at here, should we add a
HW constraint or at least fence it off at the beginning of the
hw_params()? This is actually nothing specific to the pair-request
function but a hardware constraint.

How about add constraint in startup?
static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream,
                               struct snd_soc_dai *dai) {
       struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);

       if (asrc_priv->channel_bits == 3) {
               snd_pcm_hw_constraint_step(substream->runtime, 0,
                                          SNDRV_PCM_HW_PARAM_CHANNELS, 2);
       }

       return 0;
}

Yea, that looks good to me. Better to have a line of comments to tell that
"bits==3" means older version -- maybe we should have something much
more clear than using channel_bits but it is fine for now since they only
differ here.

Ok , will send v2.

On Fri, Mar 01, 2019 at 06:55:25AM +0000, S.j. Wang wrote:

quoted

Alternatively, I feel instead of error-out at here, should we add a HW
constraint or at least fence it off at the beginning of the hw_params()? This
is actually nothing specific to the pair-request function but a hardware
constraint.

How about add constraint in startup?
static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream,
                               struct snd_soc_dai *dai)
{
       struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);

       if (asrc_priv->channel_bits == 3) {
               snd_pcm_hw_constraint_step(substream->runtime, 0,
                                          SNDRV_PCM_HW_PARAM_CHANNELS, 2);
       }

       return 0;
}

Yes, that's definitely good - the general idea is that we should never
need to return an error from hw_params() as the constraints code will
have filtered out any invalid configurations before they get that far.