diff mbox series

[2/2] ASoC: audio_graph_card2: Add support for variable slot widths

Message ID 20220216171408.265605-3-rf@opensource.cirrus.com (mailing list archive)
State Superseded
Headers show
Series ASoC: audio_graph_card2: Support variable slot widths | expand

Commit Message

Richard Fitzgerald Feb. 16, 2022, 5:14 p.m. UTC
Some audio hardware cannot support a fixed slot width or a slot width
equal to the sample width in all cases. This is usually due either to
limitations of the audio serial port or system clocking restrictions.
A typical example would be:

- 16-bit samples in 16-bit slots
- 24-bit samples in 32-bit slots

The new dai-tdm-slot-width-map property allows setting a mapping of
sample widths and the corresponding tdm slot widths. The content is
an array of integers. Each pair of values are the sample_width and
the corresponding slot width.

The property is added to each endpoint node that needs the restriction.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 include/sound/simple_card_utils.h     | 11 ++++
 sound/soc/generic/audio-graph-card2.c |  4 ++
 sound/soc/generic/simple-card-utils.c | 95 +++++++++++++++++++++++++++
 3 files changed, 110 insertions(+)

Comments

Kuninori Morimoto Feb. 17, 2022, 12:23 a.m. UTC | #1
Hi Richard

Thank you for your patch.
One comment from me.

>  struct asoc_simple_dai {
>  	const char *name;
>  	unsigned int sysclk;
> @@ -26,6 +31,9 @@ struct asoc_simple_dai {
>  	unsigned int rx_slot_mask;
>  	struct clk *clk;
>  	bool clk_fixed;
> +	struct asoc_simple_tdm_width_map *tdm_width_map;
> +	int n_tdm_widths;
> +	struct snd_soc_dai *dai;
>  };
(snip)
> +	ret = snd_soc_dai_set_tdm_slot(simple_dai->dai,
> +				       simple_dai->tx_slot_mask,
> +				       simple_dai->rx_slot_mask,
> +				       simple_dai->slots,
> +				       slot_width);
(snip)
> +	for_each_prop_dai_codec(props, i, pdai) {
> +		ret = asoc_simple_set_tdm(pdai, params);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	for_each_prop_dai_cpu(props, i, pdai) {
> +		ret = asoc_simple_set_tdm(pdai, params);
> +		if (ret < 0)
> +			return ret;
> +	}
(snip)
> @@ -386,6 +479,8 @@ static int asoc_simple_init_dai(struct snd_soc_dai *dai,
>  	if (!simple_dai)
>  		return 0;
>  
> +	simple_dai->dai = dai;

Indeed the relationship between asoc_simple_dai and snd_soc_dai are
very mystery, and current utils is using confusable naming.
We want to have some better solution about there.

Having snd_soc_dai pointer inside asoc_simple_dai itself is not bad idea.
But we can get snd_soc_dai pointer without it.

Please check asoc_simple_dai_init().
Not tested, but we can replace the code like this ?

=>	struct snd_soc_dai *dai;

	for_each_prop_dai_codec(props, i, pdai) {
=>		dai = asoc_rtd_to_codec(rtd, i);
		ret = asoc_simple_set_tdm(dai, pdai, params);
		if (ret < 0)
			return ret;
	}


Thank you for your help !!

Best regards
---
Kuninori Morimoto
Richard Fitzgerald Feb. 17, 2022, 12:51 p.m. UTC | #2
On 17/02/2022 00:23, Kuninori Morimoto wrote:
> 
> Hi Richard
> 
> Thank you for your patch.
> One comment from me.
> 
>>   struct asoc_simple_dai {
>>   	const char *name;
>>   	unsigned int sysclk;
>> @@ -26,6 +31,9 @@ struct asoc_simple_dai {
>>   	unsigned int rx_slot_mask;
>>   	struct clk *clk;
>>   	bool clk_fixed;
>> +	struct asoc_simple_tdm_width_map *tdm_width_map;
>> +	int n_tdm_widths;
>> +	struct snd_soc_dai *dai;
>>   };
> (snip)

(snip)

> (snip)
>> @@ -386,6 +479,8 @@ static int asoc_simple_init_dai(struct snd_soc_dai *dai,
>>   	if (!simple_dai)
>>   		return 0;
>>   
>> +	simple_dai->dai = dai;
> 
> Indeed the relationship between asoc_simple_dai and snd_soc_dai are
> very mystery, and current utils is using confusable naming.
> We want to have some better solution about there.
> 
> Having snd_soc_dai pointer inside asoc_simple_dai itself is not bad idea.
> But we can get snd_soc_dai pointer without it.
> 
> Please check asoc_simple_dai_init().
> Not tested, but we can replace the code like this ?
> 
> =>	struct snd_soc_dai *dai;
> 
> 	for_each_prop_dai_codec(props, i, pdai) {
> =>		dai = asoc_rtd_to_codec(rtd, i);
> 		ret = asoc_simple_set_tdm(dai, pdai, params);
> 		if (ret < 0)
> 			return ret;
> 	}
> 
> 
I first thought about doing it like that. But I was not sure whether it
is safe to assume [i] is the same entry for both arrays. If it is ok,
then I can use that and do not need to add the snd_soc_dai * to struct
asoc_simple_dai.

I will look at this and send a V2 set if it is ok.
diff mbox series

Patch

diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h
index 5ee269c59aac..1c966efe0187 100644
--- a/include/sound/simple_card_utils.h
+++ b/include/sound/simple_card_utils.h
@@ -16,6 +16,11 @@ 
 #define asoc_simple_init_mic(card, sjack, prefix) \
 	asoc_simple_init_jack(card, sjack, 0, prefix, NULL)
 
+struct asoc_simple_tdm_width_map {
+	int sample_bits;
+	int slot_width;
+};
+
 struct asoc_simple_dai {
 	const char *name;
 	unsigned int sysclk;
@@ -26,6 +31,9 @@  struct asoc_simple_dai {
 	unsigned int rx_slot_mask;
 	struct clk *clk;
 	bool clk_fixed;
+	struct asoc_simple_tdm_width_map *tdm_width_map;
+	int n_tdm_widths;
+	struct snd_soc_dai *dai;
 };
 
 struct asoc_simple_data {
@@ -132,6 +140,9 @@  int asoc_simple_parse_daifmt(struct device *dev,
 			     struct device_node *codec,
 			     char *prefix,
 			     unsigned int *retfmt);
+int asoc_simple_parse_tdm_width_map(struct device *dev, struct device_node *np,
+				    struct asoc_simple_dai *dai);
+
 __printf(3, 4)
 int asoc_simple_set_dailink_name(struct device *dev,
 				 struct snd_soc_dai_link *dai_link,
diff --git a/sound/soc/generic/audio-graph-card2.c b/sound/soc/generic/audio-graph-card2.c
index c3947347dda3..c0f3907a01fd 100644
--- a/sound/soc/generic/audio-graph-card2.c
+++ b/sound/soc/generic/audio-graph-card2.c
@@ -503,6 +503,10 @@  static int __graph_parse_node(struct asoc_simple_priv *priv,
 	if (ret < 0)
 		return ret;
 
+	ret = asoc_simple_parse_tdm_width_map(dev, ep, dai);
+	if (ret < 0)
+		return ret;
+
 	ret = asoc_simple_parse_clk(dev, ep, dai, dlc);
 	if (ret < 0)
 		return ret;
diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index a4babfb63175..60653d7d7ae7 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -12,6 +12,7 @@ 
 #include <linux/of_gpio.h>
 #include <linux/of_graph.h>
 #include <sound/jack.h>
+#include <sound/pcm_params.h>
 #include <sound/simple_card_utils.h>
 
 void asoc_simple_convert_fixup(struct asoc_simple_data *data,
@@ -87,6 +88,49 @@  int asoc_simple_parse_daifmt(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(asoc_simple_parse_daifmt);
 
+int asoc_simple_parse_tdm_width_map(struct device *dev, struct device_node *np,
+				    struct asoc_simple_dai *dai)
+{
+	u32 *array_values;
+	int n, i, ret;
+
+	if (!of_property_read_bool(np, "dai-tdm-slot-width-map"))
+		return 0;
+
+	n = of_property_count_elems_of_size(np, "dai-tdm-slot-width-map", sizeof(u32));
+	if (n % 1) {
+		dev_err(dev, "Invalid number of cells for dai-tdm-slot-width-map\n");
+		return -EINVAL;
+	}
+
+	dai->tdm_width_map = devm_kcalloc(dev, n, sizeof(*dai->tdm_width_map), GFP_KERNEL);
+	if (!dai->tdm_width_map)
+		return -ENOMEM;
+
+	array_values = kcalloc(n, sizeof(*array_values), GFP_KERNEL);
+	if (!array_values)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(np, "dai-tdm-slot-width-map", array_values, n);
+	if (ret < 0) {
+		dev_err(dev, "Could not read dai-tdm-slot-width-map: %d\n", ret);
+		goto out;
+	}
+
+	for (i = 0; i < n; i += 2) {
+		dai->tdm_width_map[i / 2].sample_bits = array_values[i];
+		dai->tdm_width_map[i / 2].slot_width = array_values[i + 1];
+	}
+
+	dai->n_tdm_widths = n / 2;
+	ret = 0;
+out:
+	kfree(array_values);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(asoc_simple_parse_tdm_width_map);
+
 int asoc_simple_set_dailink_name(struct device *dev,
 				 struct snd_soc_dai_link *dai_link,
 				 const char *fmt, ...)
@@ -309,6 +353,42 @@  static int asoc_simple_set_clk_rate(struct device *dev,
 	return clk_set_rate(simple_dai->clk, rate);
 }
 
+static int asoc_simple_set_tdm(struct asoc_simple_dai *simple_dai,
+				struct snd_pcm_hw_params *params)
+{
+	int slot_width = params_width(params);
+	int sample_bits = params_width(params);
+	int i, ret;
+
+	if (!simple_dai)
+		return 0;
+
+	if (!simple_dai->dai || !simple_dai->tdm_width_map)
+		return 0;
+
+	if (simple_dai->slot_width)
+		slot_width = simple_dai->slot_width;
+
+	for (i = 0; i < simple_dai->n_tdm_widths; ++i) {
+		if (simple_dai->tdm_width_map[i].sample_bits == sample_bits) {
+			slot_width = simple_dai->tdm_width_map[i].slot_width;
+			break;
+		}
+	}
+
+	ret = snd_soc_dai_set_tdm_slot(simple_dai->dai,
+				       simple_dai->tx_slot_mask,
+				       simple_dai->rx_slot_mask,
+				       simple_dai->slots,
+				       slot_width);
+	if (ret && ret != -ENOTSUPP) {
+		dev_err(simple_dai->dai->dev, "simple-card: set_tdm_slot error: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 int asoc_simple_hw_params(struct snd_pcm_substream *substream,
 			  struct snd_pcm_hw_params *params)
 {
@@ -362,6 +442,19 @@  int asoc_simple_hw_params(struct snd_pcm_substream *substream,
 				return ret;
 		}
 	}
+
+	for_each_prop_dai_codec(props, i, pdai) {
+		ret = asoc_simple_set_tdm(pdai, params);
+		if (ret < 0)
+			return ret;
+	}
+
+	for_each_prop_dai_cpu(props, i, pdai) {
+		ret = asoc_simple_set_tdm(pdai, params);
+		if (ret < 0)
+			return ret;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(asoc_simple_hw_params);
@@ -386,6 +479,8 @@  static int asoc_simple_init_dai(struct snd_soc_dai *dai,
 	if (!simple_dai)
 		return 0;
 
+	simple_dai->dai = dai;
+
 	if (simple_dai->sysclk) {
 		ret = snd_soc_dai_set_sysclk(dai, 0, simple_dai->sysclk,
 					     simple_dai->clk_direction);