[RFC PATCH 0/7] sun4i-i2s: Support channel remapping
Hi there,
A while back I put out this thread: How do I set up multiple codecs on one I2S - without TDM?" https://lore.kernel.org/all/ZMBRMnv0GQF4wyfQ@titan/ This is my specific use case and motivation for this patch series.
I posted how I managed to configure the audio-graph-card2 to allow doing this in conjunction with some register hacking, but I now have an initial patch series to support this in the sun4i-i2s driver.
Now I've tested this on the T113-s3 with 3 wm8782s and it works well, but I'm marking this as RFC as I think I'm out of my depth here and I'd like to know the direction I need to take this: I'm new to kernel development and I'm genuinely unsure what are best practices and which are bad practices design-wise.
My main concerns are the following:
First, I split up channel-dins and channel-slots. This is mainly because I implemented one first but both of them only make sense together. The registers themselves use a format of a byte per channel with the upper nibble being the din and the lower being the slot. Perhaps this is a better format to copy?
Second, I use u8 arrays on the device tree. This is done so I can just read the array easily, but it also means I can't make this property contingent on a compatible string in the schema as $ref doesn't seem to be allowed there.
Third, channel-slots is available on all sun4i-i2s controllers, but I only have it implemented on the R329 variant for now when there are multiple din pins. I could add support for this on older controllers but there's not really a use case for manual configuration as there's no DIN and I don't have hardware to test it on.
Fourth, I don't limit the readable channels to the channels listed. Reading more channels than you have currently results in the controller assuming you want to use TDM, but that's not the case here and you can have strange duplicate channels show up.
Fifth, it might be a good idea to increase the maximum channels from 8 to 16, especially if people are going to be running multiple TDM streams on one controller.
Sixth, the channel-slots only apply to capture, not playback. This is something I just realized now when writing and forgot to document in the patch set.
Thanks for your time, John.
John Watts (7): ASoC: sunxi: sun4i-i2s: Prepare for runtime DIN pin selection ASoC: sunxi: sun4i-i2s: Use channel-dins device tree property ASoC: sunxi: sun4i-i2s: Prepare for runtime channel slot selection ASoC: sunxi: sun4i-i2s: Use channel-slots device tree property ASoC: sunxi: sun4i-i2s: Detect TDM slots based on channel slots dt-bindings: sound: sun4i-i2s: Add channel-dins property dt-bindings: sound: sun4i-i2s: Add channel-slots property
.../sound/allwinner,sun4i-a10-i2s.yaml | 30 +++++ sound/soc/sunxi/sun4i-i2s.c | 106 +++++++++++++++++- 2 files changed, 132 insertions(+), 4 deletions(-)
The R329 and D1 support multiple I2S data input pins, but to use these they require explicit mapping to channels.
Add a mapping of DIN pins to the 16 channels in the sun4i_i2s struct then use that to write the channel mapping register.
For now the mapping is always to DIN0, identical to the previous behaviour.
Signed-off-by: John Watts contact@jookia.org --- sound/soc/sunxi/sun4i-i2s.c | 40 +++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 669d712bbe9f..d8f999ecaf05 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -216,6 +216,7 @@ struct sun4i_i2s { unsigned int mclk_freq; unsigned int slots; unsigned int slot_width; + u8 channel_dins[16];
struct snd_dmaengine_dai_dma_data capture_dma_data; struct snd_dmaengine_dai_dma_data playback_dma_data; @@ -233,6 +234,13 @@ struct sun4i_i2s_clk_div { u8 val; };
+static int sun4i_i2s_read_channel_dins(struct device *dev, struct sun4i_i2s *i2s) +{ + /* Use DIN pin 0 by default */ + memset(i2s->channel_dins, 0, sizeof(i2s->channel_dins)); + return 0; +} + static const struct sun4i_i2s_clk_div sun4i_i2s_bclk_div[] = { { .div = 2, .val = 0 }, { .div = 4, .val = 1 }, @@ -527,6 +535,25 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, return 0; }
+static void sun50i_h6_write_channel_map(const struct sun4i_i2s *i2s, + unsigned int reg, + unsigned int channel_start) +{ + unsigned int reg_value = 0; + + /* Loop backwards so we can shift values in */ + for (int i = 3; i >= 0; i--) { + int channel = channel_start + i; + u8 din = i2s->channel_dins[channel]; + u8 slot = channel; /* Map slot to channel */ + + reg_value <<= 8; + reg_value |= (din << 4) | slot; + } + + regmap_write(i2s->regmap, reg, reg_value); +} + static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, unsigned int channels, unsigned int slots, unsigned int slot_width) @@ -537,10 +564,10 @@ static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG(0), 0xFEDCBA98); regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG(0), 0x76543210); if (i2s->variant->num_din_pins > 1) { - regmap_write(i2s->regmap, SUN50I_R329_I2S_RX_CHAN_MAP0_REG, 0x0F0E0D0C); - regmap_write(i2s->regmap, SUN50I_R329_I2S_RX_CHAN_MAP1_REG, 0x0B0A0908); - regmap_write(i2s->regmap, SUN50I_R329_I2S_RX_CHAN_MAP2_REG, 0x07060504); - regmap_write(i2s->regmap, SUN50I_R329_I2S_RX_CHAN_MAP3_REG, 0x03020100); + sun50i_h6_write_channel_map(i2s, SUN50I_R329_I2S_RX_CHAN_MAP0_REG, 12); + sun50i_h6_write_channel_map(i2s, SUN50I_R329_I2S_RX_CHAN_MAP1_REG, 8); + sun50i_h6_write_channel_map(i2s, SUN50I_R329_I2S_RX_CHAN_MAP2_REG, 4); + sun50i_h6_write_channel_map(i2s, SUN50I_R329_I2S_RX_CHAN_MAP3_REG, 0); } else { regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP0_REG, 0xFEDCBA98); regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x76543210); @@ -1559,6 +1586,11 @@ static int sun4i_i2s_probe(struct platform_device *pdev) } }
+ if (sun4i_i2s_read_channel_dins(&pdev->dev, i2s)) { + dev_err(&pdev->dev, "Invalid channel DINs\n"); + return -EINVAL; + } + i2s->playback_dma_data.addr = res->start + i2s->variant->reg_offset_txdata; i2s->playback_dma_data.maxburst = 8;
Instead of using DIN pin 0 for all pins, allow changing this using the device tree property.
As an example:
&i2s2 { channel-dins = /bits/ 8 <0 0 1 1 2 2>; };
This sets channels 0 and 1 to DIN pin 0, channels 1 and 2 to DIN pin 1, and channels 3 and 4 to DIN pin 3 respectively.
Signed-off-by: John Watts contact@jookia.org --- sound/soc/sunxi/sun4i-i2s.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index d8f999ecaf05..cf66f21646a4 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -236,8 +236,33 @@ struct sun4i_i2s_clk_div {
static int sun4i_i2s_read_channel_dins(struct device *dev, struct sun4i_i2s *i2s) { + struct device_node *np = dev->of_node; + int max_channels = ARRAY_SIZE(i2s->channel_dins); + int ret; + /* Use DIN pin 0 by default */ memset(i2s->channel_dins, 0, sizeof(i2s->channel_dins)); + + if (!np) + return 0; + + ret = of_property_read_variable_u8_array(np, "channel-dins", + i2s->channel_dins, + 1, max_channels); + + if (ret == -EINVAL) + return 0; + + if (ret < 0) + return ret; + + for (int i = 0; i < ret; ++i) { + u8 din = i2s->channel_dins[i]; + + if (din >= i2s->variant->num_din_pins) + return -EINVAL; + } + return 0; }
The sun4i I2S controller supports mapping channels to specific TDM slots. This is usually done in a 1:1 mapping because there's not really a reason to re-order the TDM slots in software.
On the R329 and D1 multiplexing can be done using pins as well as TDM. Here's an example of using 3 pins and 2 slots (left and right):
Channel 0 -> DIN0 -> ADC0, TDM slot 0 Channel 1 -> DIN0 -> ADC0, TDM slot 1 Channel 2 -> DIN1 -> ADC1, TDM slot 0 Channel 3 -> DIN1 -> ADC1, TDM slot 1 Channel 4 -> DIN2 -> ADC2, TDM slot 0 Channel 5 -> DIN2 -> ADC2, TDM slot 1
This connects 3 separate TDM-unaware ADCs to the I2S controller. Likewise, multiple TDM slots could be used to run two sets of TDM-aware ADCs on one I2S controller.
Prepare for configurable slot selection by adding a channel to slot mapping array and using that in the R329 code if we multiple DIN pins.
Signed-off-by: John Watts contact@jookia.org --- sound/soc/sunxi/sun4i-i2s.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index cf66f21646a4..627bf319e1cc 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -217,6 +217,7 @@ struct sun4i_i2s { unsigned int slots; unsigned int slot_width; u8 channel_dins[16]; + u8 channel_slots[16];
struct snd_dmaengine_dai_dma_data capture_dma_data; struct snd_dmaengine_dai_dma_data playback_dma_data; @@ -266,6 +267,17 @@ static int sun4i_i2s_read_channel_dins(struct device *dev, struct sun4i_i2s *i2s return 0; }
+static int sun4i_i2s_read_channel_slots(struct device *dev, struct sun4i_i2s *i2s) +{ + int max_channels = ARRAY_SIZE(i2s->channel_dins); + + /* Use a 1:1 mapping by default */ + for (int i = 0; i < max_channels; ++i) + i2s->channel_slots[i] = i; + + return 0; +} + static const struct sun4i_i2s_clk_div sun4i_i2s_bclk_div[] = { { .div = 2, .val = 0 }, { .div = 4, .val = 1 }, @@ -570,7 +582,7 @@ static void sun50i_h6_write_channel_map(const struct sun4i_i2s *i2s, for (int i = 3; i >= 0; i--) { int channel = channel_start + i; u8 din = i2s->channel_dins[channel]; - u8 slot = channel; /* Map slot to channel */ + u8 slot = i2s->channel_slots[channel];
reg_value <<= 8; reg_value |= (din << 4) | slot; @@ -1616,6 +1628,11 @@ static int sun4i_i2s_probe(struct platform_device *pdev) return -EINVAL; }
+ if (sun4i_i2s_read_channel_slots(&pdev->dev, i2s)) { + dev_err(&pdev->dev, "Invalid channel slots\n"); + return -EINVAL; + } + i2s->playback_dma_data.addr = res->start + i2s->variant->reg_offset_txdata; i2s->playback_dma_data.maxburst = 8;
On R329 I2S variants with multiple pins it's useful to read from multiple devices within a single TDM slot. Allow changing the assignment of slots through a device tree property.
As an example:
&i2s2 { channel-dins = /bits/ 8 <0 0 1 1 2 2>; channel-slots = /bits/ 8 <0 1 0 1 0 1>; };
In addition to configuring the first 6 channels to use different DIN pins for three separate ADCs, the addition of channel-slots allows all three ADCs to be sampled within the first two TDM slots.
Signed-off-by: John Watts contact@jookia.org --- sound/soc/sunxi/sun4i-i2s.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 627bf319e1cc..019a4856c90b 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -269,12 +269,27 @@ static int sun4i_i2s_read_channel_dins(struct device *dev, struct sun4i_i2s *i2s
static int sun4i_i2s_read_channel_slots(struct device *dev, struct sun4i_i2s *i2s) { + struct device_node *np = dev->of_node; int max_channels = ARRAY_SIZE(i2s->channel_dins); + int ret;
/* Use a 1:1 mapping by default */ for (int i = 0; i < max_channels; ++i) i2s->channel_slots[i] = i;
+ if (!np) + return 0; + + ret = of_property_read_variable_u8_array(np, "channel-slots", + i2s->channel_slots, + 1, max_channels); + + if (ret == -EINVAL) + return 0; + + if (ret < 0) + return ret; + return 0; }
The current controller code assumes a 1:1 relationship between audio channel and TDM slot. This may not be the case when slots are set explicitly. Instead figure out how many slots we need based on the number of slots used in the channel map.
This allows the case of reading multiple data pins on a single slot.
Signed-off-by: John Watts contact@jookia.org --- sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 019a4856c90b..6347aaaed016 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -271,6 +271,7 @@ static int sun4i_i2s_read_channel_slots(struct device *dev, struct sun4i_i2s *i2 { struct device_node *np = dev->of_node; int max_channels = ARRAY_SIZE(i2s->channel_dins); + int slot_max; int ret;
/* Use a 1:1 mapping by default */ @@ -290,6 +291,16 @@ static int sun4i_i2s_read_channel_slots(struct device *dev, struct sun4i_i2s *i2 if (ret < 0) return ret;
+ for (int i = 0; i < ret; ++i) { + int slot = i2s->channel_slots[i]; + + if (slot_max < slot) + slot_max = slot; + } + + /* Add 1 to be inclusive of slot 0 */ + i2s->slots = slot_max + 1; + return 0; }
The R329 variant of the sun4i I2S controller supports multiple data input pins (din pins) for receiving data. Each channel can have its data input pin configured.
Allow this to be configured using a new channel-dins property.
Signed-off-by: John Watts contact@jookia.org --- .../sound/allwinner,sun4i-a10-i2s.yaml | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml index 739114fb6549..402549f9941c 100644 --- a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml +++ b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml @@ -52,6 +52,13 @@ properties: - const: apb - const: mod
+ channel-dins: + $ref: /schemas/types.yaml#/definitions/uint8-array + description: + This is a list of DIN pin numbers, each used for a receiving I2S + channel. Pins are mapped to channels based on array index. + Channel 0 is the first number, then channel 1, and so on. + # Even though it only applies to subschemas under the conditionals, # not listing them here will trigger a warning because of the # additionalsProperties set to false. @@ -144,4 +151,19 @@ examples: dma-names = "rx", "tx"; };
+ - | + i2s0_d1: i2s@2032000 { + #sound-dai-cells = <0>; + compatible = "allwinner,sun20i-d1-i2s", + "allwinner,sun50i-r329-i2s"; + reg = <0x2032000 0x1000>; + interrupts = <0 26 0>; + clocks = <&ccu 86>, <&ccu 82>; + clock-names = "apb", "mod"; + resets = <&ccu 34>; + dmas = <&dma 3>, <&dma 3>; + dma-names = "rx", "tx"; + channel-dins = /bits/ 8 <0 0 1 1 2 2>; + }; + ...
On Sat, Aug 12, 2023 at 06:14:05AM +1000, John Watts wrote:
The R329 variant of the sun4i I2S controller supports multiple data input pins (din pins) for receiving data. Each channel can have its data input pin configured.
Allow this to be configured using a new channel-dins property.
Signed-off-by: John Watts contact@jookia.org
.../sound/allwinner,sun4i-a10-i2s.yaml | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml index 739114fb6549..402549f9941c 100644 --- a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml +++ b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml @@ -52,6 +52,13 @@ properties: - const: apb - const: mod
- channel-dins:
Needs a vendor prefix.
- $ref: /schemas/types.yaml#/definitions/uint8-array
- description:
This is a list of DIN pin numbers, each used for a receiving I2Schannel. Pins are mapped to channels based on array index.Channel 0 is the first number, then channel 1, and so on.
Constraints on the values? 255 is valid pin number?
- # Even though it only applies to subschemas under the conditionals, # not listing them here will trigger a warning because of the # additionalsProperties set to false.
@@ -144,4 +151,19 @@ examples: dma-names = "rx", "tx"; };
- |
- i2s0_d1: i2s@2032000 {
Drop unused labels.
However, I don't really think adding 1 property justifies a whole other example.
#sound-dai-cells = <0>;compatible = "allwinner,sun20i-d1-i2s","allwinner,sun50i-r329-i2s";reg = <0x2032000 0x1000>;interrupts = <0 26 0>;clocks = <&ccu 86>, <&ccu 82>;clock-names = "apb", "mod";resets = <&ccu 34>;dmas = <&dma 3>, <&dma 3>;dma-names = "rx", "tx";channel-dins = /bits/ 8 <0 0 1 1 2 2>;- };
...
2.41.0
The sun4i I2S controller supports mapping arbitrary TDM slots to each channel. Allow this to be expressed in the device tree.
This is currently only implemented in the R329 I2S variant.
Allow this to be configured using a new channel-dins property.
Signed-off-by: John Watts contact@jookia.org --- .../bindings/sound/allwinner,sun4i-a10-i2s.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml index 402549f9941c..a74b02387d8a 100644 --- a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml +++ b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml @@ -59,6 +59,13 @@ properties: channel. Pins are mapped to channels based on array index. Channel 0 is the first number, then channel 1, and so on.
+ channel-slots: + $ref: /schemas/types.yaml#/definitions/uint8-array + description: + This is a list of channel I2S TDM slot numbers. Slots are mapped + to channels based on array index. + Channel 0 is the first number, then channel 1, and so on. + # Even though it only applies to subschemas under the conditionals, # not listing them here will trigger a warning because of the # additionalsProperties set to false. @@ -164,6 +171,7 @@ examples: dmas = <&dma 3>, <&dma 3>; dma-names = "rx", "tx"; channel-dins = /bits/ 8 <0 0 1 1 2 2>; + channel-slots = /bits/ 8 <0 1 0 1 0 1>; };
...
On Sat, Aug 12, 2023 at 06:14:06AM +1000, John Watts wrote:
The sun4i I2S controller supports mapping arbitrary TDM slots to each channel. Allow this to be expressed in the device tree.
This is currently only implemented in the R329 I2S variant.
Allow this to be configured using a new channel-dins property.
Signed-off-by: John Watts contact@jookia.org
.../bindings/sound/allwinner,sun4i-a10-i2s.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)
Same comments on this one.
On Sat, Aug 12, 2023 at 06:13:59AM +1000, John Watts wrote:
First, I split up channel-dins and channel-slots. This is mainly because I implemented one first but both of them only make sense together. The registers themselves use a format of a byte per channel with the upper nibble being the din and the lower being the slot. Perhaps this is a better format to copy?
I think this is fine.
Third, channel-slots is available on all sun4i-i2s controllers, but I only have it implemented on the R329 variant for now when there are multiple din pins. I could add support for this on older controllers but there's not really a use case for manual configuration as there's no DIN and I don't have hardware to test it on.
It's fine to leave this for someone who cares about that hardware to implement, might be nice to add a warning if the properties are set but not supported but it's not essential.
Fourth, I don't limit the readable channels to the channels listed. Reading more channels than you have currently results in the controller assuming you want to use TDM, but that's not the case here and you can have strange duplicate channels show up.
It would be better to have constraints which prevent userspace doing the wrong thing here.
Fifth, it might be a good idea to increase the maximum channels from 8 to 16, especially if people are going to be running multiple TDM streams on one controller.
If there's no reason not to...
participants (3)
-
John Watts -
Mark Brown -
Rob Herring