Let Atmel use simple-audio-card

There is an error message within devm_ioremap_resource already, so remove the redundant dev_err() call.

Signed-off-by Ladislav Michl <[email protected]>

---
 sound/soc/atmel/atmel-classd.c | 7 ++-----
 sound/soc/atmel/atmel-pdmic.c  | 7 ++-----
 2 files changed, 4 insertions(+), 10 deletions(-)

sound/soc/atmel/atmel-classd.c +2 -5

index ebabed69f0e6..540f0b6a95aa 100644
@@ -580,11 +580,8 @@ static int atmel_classd_probe(struct platform_device *pdev)
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	io_base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(io_base)) {
-		ret =  PTR_ERR(io_base);
-		dev_err(dev, "failed to remap register memory: %d\n", ret);
-		return ret;
-	}
+	if (IS_ERR(io_base))
+		return PTR_ERR(io_base);
 
 	dd->phy_base = res->start;
 

sound/soc/atmel/atmel-pdmic.c +2 -5

index 8e3d34be9e69..0729f656f7eb 100644
@@ -629,11 +629,8 @@ static int atmel_pdmic_probe(struct platform_device *pdev)
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	io_base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(io_base)) {
-		ret = PTR_ERR(io_base);
-		dev_err(dev, "failed to remap register memory: %d\n", ret);
-		return ret;
-	}
+	if (IS_ERR(io_base))
+		return PTR_ERR(io_base);
 
 	dd->phy_base = res->start;
 

Signature · 1 line

2.15.1

Hi,

Quoted 3 lines · Ladislav Michl

There is an error message within devm_ioremap_resource already, so remove the redundant dev_err() call.

That one can be sent as-is, don't forget to send to Mark Brown as he is the asoc maintainer.

Quoted 45 lines

Signed-off-by Ladislav Michl <[email protected]>

---
  sound/soc/atmel/atmel-classd.c | 7 ++-----
  sound/soc/atmel/atmel-pdmic.c  | 7 ++-----
  2 files changed, 4 insertions(+), 10 deletions(-)

sound/soc/atmel/atmel-classd.c +2 -5

index ebabed69f0e6..540f0b6a95aa 100644
@@ -580,11 +580,8 @@ static int atmel_classd_probe(struct platform_device *pdev)
 
  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	io_base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(io_base)) {
-		ret =  PTR_ERR(io_base);
-		dev_err(dev, "failed to remap register memory: %d\n", ret);
-		return ret;
-	}
+	if (IS_ERR(io_base))
+		return PTR_ERR(io_base);
 
  	dd->phy_base = res->start;
 

sound/soc/atmel/atmel-pdmic.c +2 -5

index 8e3d34be9e69..0729f656f7eb 100644
@@ -629,11 +629,8 @@ static int atmel_pdmic_probe(struct platform_device *pdev)
 
  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	io_base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(io_base)) {
-		ret = PTR_ERR(io_base);
-		dev_err(dev, "failed to remap register memory: %d\n", ret);
-		return ret;
-	}
+	if (IS_ERR(io_base))
+		return PTR_ERR(io_base);
 
  	dd->phy_base = res->start;
 

Signature · 1 line

2.15.1

From 1f598e6861ab1d3cc46d6eff155c0a0a6736de38 Mon Sep 17 00:00:00 2001

Signed-off-by Ladislav Michl <[email protected]>

---
 sound/soc/atmel/atmel_ssc_dai.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

sound/soc/atmel/atmel_ssc_dai.c +1 -1

index a1e2c5682dcd..8e145487cb18 100644
@@ -647,7 +647,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 
 		tcmr =	  SSC_BF(TCMR_PERIOD, 0)
 			| SSC_BF(TCMR_STTDLY, START_DELAY)
-			| SSC_BF(TCMR_START, SSC_START_FALLING_RF)
+			| SSC_BF(TCMR_START, SSC_START_TX_RX)
 			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
 			| SSC_BF(TCMR_CKO, SSC_CKO_NONE)
 			| SSC_BF(TCMR_CKS, ssc->clk_from_rk_pin ?

Signature · 1 line

2.15.1

This one needs a proper git message because it is not obvious what this is fixing.

Also, this doesn't exactly fix salve mode. Instead it is enabled synchronous mode. Maybe we need a new DT property to handle sychronous RX/TX. Probably something like what is done here:

https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/sound/fsl-sai.txt#L39

But our default would be asynchronous.

Quoted 21 lines · Ladislav Michl

Signed-off-by Ladislav Michl <[email protected]>

---
 sound/soc/atmel/atmel_ssc_dai.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

sound/soc/atmel/atmel_ssc_dai.c +1 -1

index a1e2c5682dcd..8e145487cb18 100644
@@ -647,7 +647,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 
 		tcmr =	  SSC_BF(TCMR_PERIOD, 0)
 			| SSC_BF(TCMR_STTDLY, START_DELAY)
-			| SSC_BF(TCMR_START, SSC_START_FALLING_RF)
+			| SSC_BF(TCMR_START, SSC_START_TX_RX)
 			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
 			| SSC_BF(TCMR_CKO, SSC_CKO_NONE)
 			| SSC_BF(TCMR_CKS, ssc->clk_from_rk_pin ?

Signature · 1 line

2.15.1

Signature · 3 lines

Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Signed-off-by Ladislav Michl <[email protected]>

---
 sound/soc/generic/simple-card-utils.c | 4 ++++
 1 file changed, 4 insertions(+)

sound/soc/generic/simple-card-utils.c +0 -0

index 3751a07de6aa..6b0cadc56933 100644

From 3ac9ce8858ae3c6fef308692dc4580c164adc96b Mon Sep 17 00:00:00 2001

Signed-off-by Ladislav Michl <[email protected]>

---
Note: It should be reconsidered where BSEL should be set.

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

sound/soc/codecs/max9867.c +0 -40

index 026b7cf94910..6272cf5df4a9 100644
@@ -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;
 }

sound/soc/codecs/max9867.c +1 -0

+
 	return 0;
 }
 

Signature · 1 line

2.15.1

This also needs a proper commit message.

Quoted 59 lines

Signed-off-by Ladislav Michl <[email protected]>

---
Note: It should be reconsidered where BSEL should be set.

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

sound/soc/codecs/max9867.c +0 -40

index 026b7cf94910..6272cf5df4a9 100644
@@ -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;
 }

sound/soc/codecs/max9867.c static int max9867_set_dai_sysclk(struct snd_soc_dai *codec_dai, · line 208

Quoted 5 lines

sound/soc/codecs/max9867.c +2 -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.

Quoted 7 lines

sound/soc/codecs/max9867.c +1 -0

+
 	return 0;
 }
 

Signature · 1 line

2.15.1

Signature · 3 lines

Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Quoted 1 line · Alexandre Belloni

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?

Quoted 80 lines · Ladislav Michl

Quoted 59 lines

Signed-off-by Ladislav Michl <[email protected]>

---
Note: It should be reconsidered where BSEL should be set.

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

sound/soc/codecs/max9867.c +0 -40

index 026b7cf94910..6272cf5df4a9 100644
@@ -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;
 }

sound/soc/codecs/max9867.c static int max9867_set_dai_sysclk(struct snd_soc_dai *codec_dai, · line 208

Quoted 5 lines

sound/soc/codecs/max9867.c +2 -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.

Quoted 7 lines

sound/soc/codecs/max9867.c +1 -0

+
 	return 0;
 }
 

Signature · 1 line

2.15.1

Signature · 3 lines

Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Quoted 6 lines · Ladislav Michl

Quoted 1 line · Alexandre Belloni

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.

Quoted 3 lines

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.

Quoted 4 lines

- 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.

Quoted 7 lines · Alexandre Belloni

Quoted 6 lines · Ladislav Michl

Quoted 1 line · Alexandre Belloni

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.

Quoted 6 lines

Quoted 3 lines

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).

Quoted 6 lines

Quoted 4 lines

- 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.

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 <[email protected]>

sound/soc/codecs/max9867.c +15 -71

index 0a6bd9f4d00a..d9b81a95a200 100644
@@ -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,
 	}