mbox series

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

Message ID 20230811201406.4096210-1-contact@jookia.org
Headers show
Series sun4i-i2s: Support channel remapping | expand

Message

John Watts Aug. 11, 2023, 8:13 p.m. UTC 
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(-)

Comments

Rob Herring (Arm) Aug. 21, 2023, 5:35 p.m. UTC | #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.
Mark Brown Aug. 31, 2023, 12:24 p.m. UTC | #2 
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...