[RFC PATCH 0/7] sun4i-i2s: Support channel remapping

[RFC PATCH 0/7] sun4i-i2s: Support channel remapping

[John Watts]

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

-- 
2.41.0

[RFC PATCH 1/7] ASoC: sunxi: sun4i-i2s: Prepare for runtime DIN pin selection

[John Watts]

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;
-- 
2.41.0

[RFC PATCH 2/7] ASoC: sunxi: sun4i-i2s: Use channel-dins device tree property

[John Watts]

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;
 }
 
-- 
2.41.0

[RFC PATCH 3/7] ASoC: sunxi: sun4i-i2s: Prepare for runtime channel slot selection

[John Watts]

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;
-- 
2.41.0

[RFC PATCH 4/7] ASoC: sunxi: sun4i-i2s: Use channel-slots device tree property

[John Watts]

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;
 }
 
-- 
2.41.0

[RFC PATCH 5/7] ASoC: sunxi: sun4i-i2s: Detect TDM slots based on channel slots

[John Watts]

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;
 }
 
-- 
2.41.0

[RFC PATCH 6/7] dt-bindings: sound: sun4i-i2s: Add channel-dins property

[John Watts]

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>;
+    };
+
 ...
-- 
2.41.0

[RFC PATCH 7/7] dt-bindings: sound: sun4i-i2s: Add channel-slots property

[John Watts]

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>;
     };
 
 ...
-- 
2.41.0

Re: [RFC PATCH 6/7] dt-bindings: sound: sun4i-i2s: Add channel-dins property

[Rob Herring]

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 I2S
> +      channel. 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
> 

Re: [RFC PATCH 7/7] dt-bindings: sound: sun4i-i2s: Add channel-slots property

[Rob Herring]

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.

Re: [RFC PATCH 0/7] sun4i-i2s: Support channel remapping

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1474 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]