diff mbox

[6/7] ASoC: max9867: Fix BSEL value in master mode.

Message ID 20180130111033.GG18123@lenoch (mailing list archive)
State New, archived
Headers show

Commit Message

Ladislav Michl Jan. 30, 2018, 11:10 a.m. UTC 
Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 Note: It should be reconsidered where BSEL should be set.

 sound/soc/codecs/max9867.c | 43 +++----------------------------------------
 1 file changed, 3 insertions(+), 40 deletions(-)

Comments

Alexandre Belloni Feb. 27, 2018, 5:23 p.m. UTC | #1 
This also needs a proper commit message.

On 30/01/2018 at 12:10:33 +0100, Ladislav Michl wrote:
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> ---
>  Note: It should be reconsidered where BSEL should be set.
> 
>  sound/soc/codecs/max9867.c | 43 +++----------------------------------------
>  1 file changed, 3 insertions(+), 40 deletions(-)
> 
> diff --git a/sound/soc/codecs/max9867.c b/sound/soc/codecs/max9867.c
> index 026b7cf94910..6272cf5df4a9 100644
> --- a/sound/soc/codecs/max9867.c
> +++ b/sound/soc/codecs/max9867.c
> @@ -148,46 +148,6 @@ static int max9867_dai_hw_params(struct snd_pcm_substream *substream,
>  			MAX9867_RAPID_LOCK, MAX9867_RAPID_LOCK);
>  		regmap_update_bits(max9867->regmap, MAX9867_AUDIOCLKHIGH,
>  			MAX9867_PLL, MAX9867_PLL);
> -	} else {
> -		unsigned long int bclk_rate, pclk_bclk_ratio;
> -		int bclk_value;
> -
> -		bclk_rate = params_rate(params) * 2 * params_width(params);
> -		pclk_bclk_ratio = max9867->pclk/bclk_rate;
> -		switch (params_width(params)) {
> -		case 8:
> -		case 16:
> -			switch (pclk_bclk_ratio) {
> -			case 2:
> -				bclk_value = MAX9867_IFC1B_PCLK_2;
> -				break;
> -			case 4:
> -				bclk_value = MAX9867_IFC1B_PCLK_4;
> -				break;
> -			case 8:
> -				bclk_value = MAX9867_IFC1B_PCLK_8;
> -				break;
> -			case 16:
> -				bclk_value = MAX9867_IFC1B_PCLK_16;
> -				break;
> -			default:
> -				dev_err(codec->dev,
> -					"unsupported sampling rate\n");
> -				return -EINVAL;
> -			}
> -			break;
> -		case 24:
> -			bclk_value = MAX9867_IFC1B_24BIT;
> -			break;
> -		case 32:
> -			bclk_value = MAX9867_IFC1B_32BIT;
> -			break;
> -		default:
> -			dev_err(codec->dev, "unsupported sampling rate\n");
> -			return -EINVAL;
> -		}
> -		regmap_update_bits(max9867->regmap, MAX9867_IFC1B,
> -			MAX9867_IFC1B_BCLK_MASK, bclk_value);
>  	}
>  	return 0;
>  }
> @@ -244,6 +204,9 @@ static int max9867_set_dai_sysclk(struct snd_soc_dai *codec_dai,
>  	value &= ~MAX9867_FREQ_MASK;
>  	regmap_update_bits(max9867->regmap, MAX9867_SYSCLK,
>  			MAX9867_PSCLK_MASK, value);
> +	regmap_update_bits(max9867->regmap, MAX9867_IFC1B,
> +			MAX9867_IFC1B_BCLK_MASK, 0x010);

This magic value has to be defined somewhere.

> +
>  	return 0;
>  }
>  
> -- 
> 2.15.1
>
Ladislav Michl Feb. 27, 2018, 7:03 p.m. UTC | #2 
On Tue, Feb 27, 2018 at 06:23:09PM +0100, Alexandre Belloni wrote:
> This also needs a proper commit message.

Meanwhile more patches for this driver accumulated, so I'll send simple ones
first and those I'm uncertain of later.

Now few questions:
- should hw_params emit any error message when asked to set for example
  unsupported sample rate? Many drivers do so, but it seems strange.
- table used in this driver is overkill, frequencies can be calculated
  directly (patch ready), but that brings question about
  SNDRV_PCM_RATE_CONTINUOUS: what does it exactly mean? What if codec is
  able to set "any" rate, but there are rounding errors?

> On 30/01/2018 at 12:10:33 +0100, Ladislav Michl wrote:
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > ---
> >  Note: It should be reconsidered where BSEL should be set.
> > 
> >  sound/soc/codecs/max9867.c | 43 +++----------------------------------------
> >  1 file changed, 3 insertions(+), 40 deletions(-)
> > 
> > diff --git a/sound/soc/codecs/max9867.c b/sound/soc/codecs/max9867.c
> > index 026b7cf94910..6272cf5df4a9 100644
> > --- a/sound/soc/codecs/max9867.c
> > +++ b/sound/soc/codecs/max9867.c
> > @@ -148,46 +148,6 @@ static int max9867_dai_hw_params(struct snd_pcm_substream *substream,
> >  			MAX9867_RAPID_LOCK, MAX9867_RAPID_LOCK);
> >  		regmap_update_bits(max9867->regmap, MAX9867_AUDIOCLKHIGH,
> >  			MAX9867_PLL, MAX9867_PLL);
> > -	} else {
> > -		unsigned long int bclk_rate, pclk_bclk_ratio;
> > -		int bclk_value;
> > -
> > -		bclk_rate = params_rate(params) * 2 * params_width(params);
> > -		pclk_bclk_ratio = max9867->pclk/bclk_rate;
> > -		switch (params_width(params)) {
> > -		case 8:
> > -		case 16:
> > -			switch (pclk_bclk_ratio) {
> > -			case 2:
> > -				bclk_value = MAX9867_IFC1B_PCLK_2;
> > -				break;
> > -			case 4:
> > -				bclk_value = MAX9867_IFC1B_PCLK_4;
> > -				break;
> > -			case 8:
> > -				bclk_value = MAX9867_IFC1B_PCLK_8;
> > -				break;
> > -			case 16:
> > -				bclk_value = MAX9867_IFC1B_PCLK_16;
> > -				break;
> > -			default:
> > -				dev_err(codec->dev,
> > -					"unsupported sampling rate\n");
> > -				return -EINVAL;
> > -			}
> > -			break;
> > -		case 24:
> > -			bclk_value = MAX9867_IFC1B_24BIT;
> > -			break;
> > -		case 32:
> > -			bclk_value = MAX9867_IFC1B_32BIT;
> > -			break;
> > -		default:
> > -			dev_err(codec->dev, "unsupported sampling rate\n");
> > -			return -EINVAL;
> > -		}
> > -		regmap_update_bits(max9867->regmap, MAX9867_IFC1B,
> > -			MAX9867_IFC1B_BCLK_MASK, bclk_value);
> >  	}
> >  	return 0;
> >  }
> > @@ -244,6 +204,9 @@ static int max9867_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> >  	value &= ~MAX9867_FREQ_MASK;
> >  	regmap_update_bits(max9867->regmap, MAX9867_SYSCLK,
> >  			MAX9867_PSCLK_MASK, value);
> > +	regmap_update_bits(max9867->regmap, MAX9867_IFC1B,
> > +			MAX9867_IFC1B_BCLK_MASK, 0x010);
> 
> This magic value has to be defined somewhere.
> 
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.15.1
> > 
> 
> -- 
> Alexandre Belloni, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com
Alexandre Belloni Feb. 28, 2018, 10 a.m. UTC | #3 
On 27/02/2018 at 20:03:38 +0100, Ladislav Michl wrote:
> On Tue, Feb 27, 2018 at 06:23:09PM +0100, Alexandre Belloni wrote:
> > This also needs a proper commit message.
> 
> Meanwhile more patches for this driver accumulated, so I'll send simple ones
> first and those I'm uncertain of later.
> 
> Now few questions:
> - should hw_params emit any error message when asked to set for example
>   unsupported sample rate? Many drivers do so, but it seems strange.

I would thin that is correct because then the eror goes up to userspace
and it knows it has to resample.

> - table used in this driver is overkill, frequencies can be calculated
>   directly (patch ready), but that brings question about
>   SNDRV_PCM_RATE_CONTINUOUS: what does it exactly mean? What if codec is
>   able to set "any" rate, but there are rounding errors?
> 

That is a good question I can't answer.

> > On 30/01/2018 at 12:10:33 +0100, Ladislav Michl wrote:
> > > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > > ---
> > >  Note: It should be reconsidered where BSEL should be set.
> > > 
> > >  sound/soc/codecs/max9867.c | 43 +++----------------------------------------
> > >  1 file changed, 3 insertions(+), 40 deletions(-)
> > > 
> > > diff --git a/sound/soc/codecs/max9867.c b/sound/soc/codecs/max9867.c
> > > index 026b7cf94910..6272cf5df4a9 100644
> > > --- a/sound/soc/codecs/max9867.c
> > > +++ b/sound/soc/codecs/max9867.c
> > > @@ -148,46 +148,6 @@ static int max9867_dai_hw_params(struct snd_pcm_substream *substream,
> > >  			MAX9867_RAPID_LOCK, MAX9867_RAPID_LOCK);
> > >  		regmap_update_bits(max9867->regmap, MAX9867_AUDIOCLKHIGH,
> > >  			MAX9867_PLL, MAX9867_PLL);
> > > -	} else {
> > > -		unsigned long int bclk_rate, pclk_bclk_ratio;
> > > -		int bclk_value;
> > > -
> > > -		bclk_rate = params_rate(params) * 2 * params_width(params);
> > > -		pclk_bclk_ratio = max9867->pclk/bclk_rate;
> > > -		switch (params_width(params)) {
> > > -		case 8:
> > > -		case 16:
> > > -			switch (pclk_bclk_ratio) {
> > > -			case 2:
> > > -				bclk_value = MAX9867_IFC1B_PCLK_2;
> > > -				break;
> > > -			case 4:
> > > -				bclk_value = MAX9867_IFC1B_PCLK_4;
> > > -				break;
> > > -			case 8:
> > > -				bclk_value = MAX9867_IFC1B_PCLK_8;
> > > -				break;
> > > -			case 16:
> > > -				bclk_value = MAX9867_IFC1B_PCLK_16;
> > > -				break;
> > > -			default:
> > > -				dev_err(codec->dev,
> > > -					"unsupported sampling rate\n");
> > > -				return -EINVAL;
> > > -			}
> > > -			break;
> > > -		case 24:
> > > -			bclk_value = MAX9867_IFC1B_24BIT;
> > > -			break;
> > > -		case 32:
> > > -			bclk_value = MAX9867_IFC1B_32BIT;
> > > -			break;
> > > -		default:
> > > -			dev_err(codec->dev, "unsupported sampling rate\n");
> > > -			return -EINVAL;
> > > -		}
> > > -		regmap_update_bits(max9867->regmap, MAX9867_IFC1B,
> > > -			MAX9867_IFC1B_BCLK_MASK, bclk_value);
> > >  	}
> > >  	return 0;
> > >  }
> > > @@ -244,6 +204,9 @@ static int max9867_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> > >  	value &= ~MAX9867_FREQ_MASK;
> > >  	regmap_update_bits(max9867->regmap, MAX9867_SYSCLK,
> > >  			MAX9867_PSCLK_MASK, value);
> > > +	regmap_update_bits(max9867->regmap, MAX9867_IFC1B,
> > > +			MAX9867_IFC1B_BCLK_MASK, 0x010);
> > 
> > This magic value has to be defined somewhere.
> > 
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > -- 
> > > 2.15.1
> > > 
> > 
> > -- 
> > Alexandre Belloni, Bootlin (formerly Free Electrons)
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
Alexandre Belloni Feb. 28, 2018, 10:28 a.m. UTC | #4 
On 28/02/2018 at 11:23:13 +0100, Ladislav Michl wrote:
> On Wed, Feb 28, 2018 at 11:00:54AM +0100, Alexandre Belloni wrote:
> > On 27/02/2018 at 20:03:38 +0100, Ladislav Michl wrote:
> > > On Tue, Feb 27, 2018 at 06:23:09PM +0100, Alexandre Belloni wrote:
> > > > This also needs a proper commit message.
> > > 
> > > Meanwhile more patches for this driver accumulated, so I'll send simple ones
> > > first and those I'm uncertain of later.
> > > 
> > > Now few questions:
> > > - should hw_params emit any error message when asked to set for example
> > >   unsupported sample rate? Many drivers do so, but it seems strange.
> > 
> > I would thin that is correct because then the eror goes up to userspace
> > and it knows it has to resample.
> 
> Perhaps I didn't express it clearly enough. I mean all those
> dev_err(dev, "sunsupported rate\n");
> Here only human sitting in realspace is able to react somehow, but I'd say
> he's only annoyed.
> 
> I would expect driver is able to set any rate it is advertising, so returned
> -EINVAL is considered to be a programming error (which shouldn't clutter
> kernel log).
> 

Ah yes, please feel free to remove the dev_err messages. I also find
that they clutter the kernel output needlessly.

> > > - table used in this driver is overkill, frequencies can be calculated
> > >   directly (patch ready), but that brings question about
> > >   SNDRV_PCM_RATE_CONTINUOUS: what does it exactly mean? What if codec is
> > >   able to set "any" rate, but there are rounding errors?
> > 
> > That is a good question I can't answer.
> 
> Thank you anyway, hopefully someone else would be able to answer.
> 

Well, Mark is not Cced, you'd have to get his attention somehow ;)

> Consider patch bellow - with introduced change codec is able to set any rate
> in master mode (it is an example only, it needs more work), but not any rate
> will be matched exactly. Of course, for each master clock rate it is possible
> to compute exact supported rates and advertise them. Is that way forward or
> is there any trick with SNDRV_PCM_RATE_CONTINUOUS?
> 
> Subject: ASoC: max9867: Use continuous sample rate
> 
> Drop "Common NI Values Table" and calculate LRCLK divider.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> 
> diff --git a/sound/soc/codecs/max9867.c b/sound/soc/codecs/max9867.c
> index 0a6bd9f4d00a..d9b81a95a200 100644
> --- a/sound/soc/codecs/max9867.c
> +++ b/sound/soc/codecs/max9867.c
> @@ -126,75 +126,19 @@ static const struct snd_soc_dapm_route max9867_audio_map[] = {
>  	{"LINE_IN", NULL, "Right Line"},
>  };
>  
> -enum rates {
> -	pcm_rate_8, pcm_rate_16, pcm_rate_24,
> -	pcm_rate_32, pcm_rate_44,
> -	pcm_rate_48, max_pcm_rate,
> -};
> -
> -static const struct ni_div_rates {
> -	u32 mclk;
> -	u16 ni[max_pcm_rate];
> -} ni_div[] = {
> -	{11289600, {0x116A, 0x22D4, 0x343F, 0x45A9, 0x6000, 0x687D} },
> -	{12000000, {0x1062, 0x20C5, 0x3127, 0x4189, 0x5A51, 0x624E} },
> -	{12288000, {0x1000, 0x2000, 0x3000, 0x4000, 0x5833, 0x6000} },
> -	{13000000, {0x0F20, 0x1E3F, 0x2D5F, 0x3C7F, 0x535F, 0x5ABE} },
> -	{19200000, {0x0A3D, 0x147B, 0x1EB8, 0x28F6, 0x3873, 0x3D71} },
> -	{24000000, {0x1062, 0x20C5, 0x1893, 0x4189, 0x5A51, 0x624E} },
> -	{26000000, {0x0F20, 0x1E3F, 0x16AF, 0x3C7F, 0x535F, 0x5ABE} },
> -	{27000000, {0x0E90, 0x1D21, 0x15D8, 0x3A41, 0x5048, 0x5762} },
> -};
> -
> -static inline int get_ni_value(int mclk, int rate)
> -{
> -	int i, ret = 0;
> -
> -	/* find the closest rate index*/
> -	for (i = 0; i < ARRAY_SIZE(ni_div); i++) {
> -		if (ni_div[i].mclk >= mclk)
> -			break;
> -	}
> -	if (i == ARRAY_SIZE(ni_div))
> -		return -EINVAL;
> -
> -	switch (rate) {
> -	case 8000:
> -		return ni_div[i].ni[pcm_rate_8];
> -	case 16000:
> -		return ni_div[i].ni[pcm_rate_16];
> -	case 32000:
> -		return ni_div[i].ni[pcm_rate_32];
> -	case 44100:
> -		return ni_div[i].ni[pcm_rate_44];
> -	case 48000:
> -		return ni_div[i].ni[pcm_rate_48];
> -	default:
> -		pr_err("%s wrong rate %d\n", __func__, rate);
> -		ret = -EINVAL;
> -	}
> -	return ret;
> -}
> -
>  static int max9867_dai_hw_params(struct snd_pcm_substream *substream,
>  		struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
>  {
>  	struct snd_soc_component *component = dai->component;
>  	struct max9867_priv *max9867 = snd_soc_component_get_drvdata(component);
> -	unsigned int ni_h, ni_l;
> -	int value;
> +	unsigned int ni = DIV_ROUND_CLOSEST_ULL(96ULL * 0x10000 * params_rate(params),
> +						max9867->pclk);
>  
> -	value = get_ni_value(max9867->sysclk, params_rate(params));
> -	if (value < 0)
> -		return value;
> -
> -	ni_h = (0xFF00 & value) >> 8;
> -	ni_l = 0x00FF & value;
>  	/* set up the ni value */
>  	regmap_update_bits(max9867->regmap, MAX9867_AUDIOCLKHIGH,
> -		MAX9867_NI_HIGH_MASK, ni_h);
> +		MAX9867_NI_HIGH_MASK, (0xFF00 & ni) >> 8);
>  	regmap_update_bits(max9867->regmap, MAX9867_AUDIOCLKLOW,
> -		MAX9867_NI_LOW_MASK, ni_l);
> +		MAX9867_NI_LOW_MASK, 0x00FF & ni);
>  	if (!max9867->master) {
>  		/*
>  		 * digital pll locks on to any externally supplied LRCLK signal
> @@ -241,13 +185,13 @@ static int max9867_set_dai_sysclk(struct snd_soc_dai *codec_dai,
>  	/* Set the prescaler based on the master clock frequency*/
>  	if (freq >= 10000000 && freq <= 20000000) {
>  		value |= MAX9867_PSCLK_10_20;
> -		max9867->pclk =  freq;
> +		max9867->pclk = freq;
>  	} else if (freq >= 20000000 && freq <= 40000000) {
>  		value |= MAX9867_PSCLK_20_40;
> -		max9867->pclk =  freq/2;
> +		max9867->pclk = freq / 2;
>  	} else if (freq >= 40000000 && freq <= 60000000) {
>  		value |= MAX9867_PSCLK_40_60;
> -		max9867->pclk =  freq/4;
> +		max9867->pclk = freq / 4;
>  	} else {
>  		dev_err(component->dev,
>  			"Invalid clock frequency %uHz (required 10-60MHz)\n",
> @@ -322,10 +266,6 @@ static const struct snd_soc_dai_ops max9867_dai_ops = {
>  	.hw_params = max9867_dai_hw_params,
>  };
>  
> -#define MAX9867_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |\
> -	SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000)
> -#define MAX9867_FORMATS (SNDRV_PCM_FMTBIT_S16_LE)
> -
>  static struct snd_soc_dai_driver max9867_dai[] = {
>  	{
>  	.name = "max9867-aif1",
> @@ -333,15 +273,19 @@ static struct snd_soc_dai_driver max9867_dai[] = {
>  		.stream_name = "HiFi Playback",
>  		.channels_min = 1,
>  		.channels_max = 2,
> -		.rates = MAX9867_RATES,
> -		.formats = MAX9867_FORMATS,
> +		.rates = SNDRV_PCM_RATE_CONTINUOUS,
> +		.rate_min = 8000,
> +		.rate_max = 48000,
> +		.formats = SNDRV_PCM_FMTBIT_S16_LE,
>  	},
>  	.capture = {
>  		.stream_name = "HiFi Capture",
>  		.channels_min = 1,
>  		.channels_max = 2,
> -		.rates = MAX9867_RATES,
> -		.formats = MAX9867_FORMATS,
> +		.rates = SNDRV_PCM_RATE_CONTINUOUS,
> +		.rate_min = 8000,
> +		.rate_max = 48000,
> +		.formats = SNDRV_PCM_FMTBIT_S16_LE,
>  	},
>  	.ops = &max9867_dai_ops,
>  	}
diff mbox

Patch

diff --git a/sound/soc/codecs/max9867.c b/sound/soc/codecs/max9867.c
index 026b7cf94910..6272cf5df4a9 100644
--- a/sound/soc/codecs/max9867.c
+++ b/sound/soc/codecs/max9867.c
@@ -148,46 +148,6 @@  static int max9867_dai_hw_params(struct snd_pcm_substream *substream,
 			MAX9867_RAPID_LOCK, MAX9867_RAPID_LOCK);
 		regmap_update_bits(max9867->regmap, MAX9867_AUDIOCLKHIGH,
 			MAX9867_PLL, MAX9867_PLL);
-	} else {
-		unsigned long int bclk_rate, pclk_bclk_ratio;
-		int bclk_value;
-
-		bclk_rate = params_rate(params) * 2 * params_width(params);
-		pclk_bclk_ratio = max9867->pclk/bclk_rate;
-		switch (params_width(params)) {
-		case 8:
-		case 16:
-			switch (pclk_bclk_ratio) {
-			case 2:
-				bclk_value = MAX9867_IFC1B_PCLK_2;
-				break;
-			case 4:
-				bclk_value = MAX9867_IFC1B_PCLK_4;
-				break;
-			case 8:
-				bclk_value = MAX9867_IFC1B_PCLK_8;
-				break;
-			case 16:
-				bclk_value = MAX9867_IFC1B_PCLK_16;
-				break;
-			default:
-				dev_err(codec->dev,
-					"unsupported sampling rate\n");
-				return -EINVAL;
-			}
-			break;
-		case 24:
-			bclk_value = MAX9867_IFC1B_24BIT;
-			break;
-		case 32:
-			bclk_value = MAX9867_IFC1B_32BIT;
-			break;
-		default:
-			dev_err(codec->dev, "unsupported sampling rate\n");
-			return -EINVAL;
-		}
-		regmap_update_bits(max9867->regmap, MAX9867_IFC1B,
-			MAX9867_IFC1B_BCLK_MASK, bclk_value);
 	}
 	return 0;
 }
@@ -244,6 +204,9 @@  static int max9867_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 	value &= ~MAX9867_FREQ_MASK;
 	regmap_update_bits(max9867->regmap, MAX9867_SYSCLK,
 			MAX9867_PSCLK_MASK, value);
+	regmap_update_bits(max9867->regmap, MAX9867_IFC1B,
+			MAX9867_IFC1B_BCLK_MASK, 0x010);
+
 	return 0;
 }