[PATCH 0/2] ASoC: audio_graph_card2: Support variable slot widths

[PATCH 0/2] ASoC: audio_graph_card2: Support variable slot widths

[Richard Fitzgerald]

This adds support for I2S/TDM links where the slot width varies
depending on the sample width, in a way that cannot be guessed by
component hw_params().

A typical example is:

- 16-bit samples use 16-bit slots
- 24-bit samples use 32-bit slots

There is no way for a component hw_params() to deduce from the information
it is passed that 24-bit samples will be in 32-bit slots.

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.

These two patches add support for defining a mapping between sample widths
and sample slots. This allows audio_graph_card2 to be used in these
situations instead of having to write a custom machine driver.

Richard Fitzgerald (2):
  ASoC: dt-bindings: audio-graph-port: Add dai-tdm-slot-width-map
  ASoC: audio_graph_card2: Add support for variable slot widths

 .../bindings/sound/audio-graph-port.yaml      |  7 ++
 include/sound/simple_card_utils.h             | 11 +++
 sound/soc/generic/audio-graph-card2.c         |  4 +
 sound/soc/generic/simple-card-utils.c         | 95 +++++++++++++++++++
 4 files changed, 117 insertions(+)

-- 
2.30.2

[PATCH 1/2] ASoC: dt-bindings: audio-graph-port: Add dai-tdm-slot-width-map

[Richard Fitzgerald]

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.

This property allows setting a mapping of sample widths and the
corresponding tdm slot widths.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 .../devicetree/bindings/sound/audio-graph-port.yaml        | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
index 476dcb49ece6..420adad49382 100644
--- a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
+++ b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
@@ -71,4 +71,11 @@ patternProperties:
         description: CPU to Codec rate channels.
         $ref: /schemas/types.yaml#/definitions/uint32
 
+      dai-tdm-slot-width-map:
+        description: Mapping of sample widths to slot widths. For hardware that
+          cannot support a fixed slot width or a slot width equal to sample
+          width. An array containing one or more pairs of values. Each pair
+          of values is a sample_width and the corresponding slot_width.
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+
 additionalProperties: true
-- 
2.30.2

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

[Richard Fitzgerald]

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(+)

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);
-- 
2.30.2

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

[Kuninori Morimoto]

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

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

[Richard Fitzgerald]

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.