[RFC,00/34] sun8i-codec fixes and new features
mbox series

Message ID 20200217064250.15516-1-samuel@sholland.org
Headers show
Series
  • sun8i-codec fixes and new features
Related show

Message

Samuel Holland Feb. 17, 2020, 6:42 a.m. UTC
Hello all,

The sun8i-codec driver, as used in the Allwinner A33 and A64, currently
only exposes a small subset of the available hardware features. In order
to use the A64 in a smartphone (the PinePhone), I've added the necessary
functionality to the driver:
  * The full set of supported DAI format options
  * Support for AIF2 and AIF3
  * Additional routing knobs
  * Additional volume controls

Unfortunately, due to preexisting issues with the driver, there are some
breaking changes, as explained further in the commit messages:
  * The LRCK inversion issue means we need a new compatible for the A64.
  * Some controls are named inaccurately, so they are renamed.
  * Likewise, the DAPM widgets used in device trees were either named
    wrong, or the device trees were using the wrong widgets in the first
    place. (Specifically, the links between the analog codec and digital
    codec happen at the ADC and DAC, not AIF1.)

I tended to take the philosophy of "while I'm breaking things, I might
as well do them right", so I've probably made a few more changes than
absolutely necessary. I'm not sure about where all of the policy
boundaries are, about how far I should go to maintain compatibility. For
example, for the DT widget usage, I could:
  * Rename everything and update the DTS files (which is what I did)
  * Keep the old (misleading/wrong) name for the widgets, but repurpose
    them to work correctly
      (i.e. "ADC Left" would be named "AIF1 Slot 0 Left ADC", but it
       would work just like "ADC Left" does in this patchset)
  * Keep the old widgets around as a compatibility layer, but add new
    widgets and update the in-tree DTS files to use them
      (i.e. "ADC Left" would have a path from "AIF1 Slot 0 Left ADC",
       but "AIF1 Slot 0 Left ADC" would be a no-op widget)
  * Something else entirely

There are several trivial fixes in here, and there are several commits
that just add new features without changing any existing behavior, but
there is enough changing that I thought it would be best to send the
whole thing as an RFC. I'm more than happy to reorganize this into one
or several patchsets in future revisions. It doesn't have to all go in
at once.

This has all been tested on the PinePhone. For example, I've
successfully routed a voice call between the modem at AIF2 and a
Bluetooth headset at AIF3, merged with an audio file played from AIF1.
Unfortunately, the PinePhone does not have a DTS upstream yet (partially
due to the audio driver situation), so I don't have an example DTS in
this patchset.

Thanks for any feedback,
Samuel

Samuel Holland (34):
  ASoC: dt-bindings: Add a separate compatible for the A64 codec
  ASoC: sun8i-codec: LRCK is not inverted on A64
  arm64: dts: allwinner: a64: Fix the audio codec compatible
  ASoC: sun8i-codec: Remove unused dev from codec struct
  ASoC: sun8i-codec: Remove incorrect SND_SOC_DAIFMT_DSP_B
  ASoC: sun8i-codec: Fix setting DAI data format
  ASoC: sun8i-codec: Remove extraneous widgets
  ASoC: sun8i-codec: Fix direction of AIF1 outputs
  ASoC: sun8i-codec: Fix broken DAPM routing
  ASoC: sun8i-codec: Advertise only hardware-supported rates
  ASoC: sun8i-codec: Enforce parameter symmetry
  ASoC: sun8i-codec: Fix AIF1 MODCLK widget name
  ASoC: sun8i-codec: Fix AIF1_ADCDAT_CTRL field names
  ASoC: sun8i-codec: Fix AIF1_MXR_SRC field names
  ASoC: sun8i-codec: Fix ADC_DIG_CTRL field name
  ASoC: sun8i-codec: Fix field bit number indentation
  ASoC: sun8i-codec: Sort masks in a consistent order
  ASoC: sun8i-codec: Allow all clock inversion permutations
  ASoC: sun8i-codec: Support mono DAI configurations
  ASoC: sun8i-codec: Support 8/20/24-bit word sizes
  ASoC: sun8i-codec: Clean up module/clock hierarchy
  ASoC: sun8i-codec: Clean up AIF1 Slot 0 widgets
  ASoC: sun8i-codec: Clean up DAC widgets
  ASoC: sun8i-codec: Prepare to support multiple AIFs
  ASoC: sun8i-codec: Add support for AIF2
  ASoC: sun8i-codec: Add support for AIF3
  ASoC: sun8i-codec: Add AIF mono/stereo controls
  ASoC: sun8i-codec: Add AIF loopback controls
  ASoC: sun8i-codec: Add AIF, ADC, and DAC volume controls
  ASoC: dt-bindings: Bump sound-dai-cells on sun8i-codec
  ARM: dts: sun8i-a33: Allow using multiple codec DAIs
  arm64: dts: allwinner: a64: Allow using multiple codec DAIs
  arm64: dts: allwinner: a64: Allow multiple DAI links
  arm64: dts: allwinner: a64: Add pinmux for AIF2/AIF3

 .../sound/allwinner,sun8i-a33-codec.yaml      |    6 +-
 arch/arm/boot/dts/sun8i-a33-olinuxino.dts     |    6 +-
 arch/arm/boot/dts/sun8i-a33.dtsi              |   10 +-
 .../dts/allwinner/sun50i-a64-bananapi-m64.dts |    8 +-
 .../dts/allwinner/sun50i-a64-orangepi-win.dts |    8 +-
 .../boot/dts/allwinner/sun50i-a64-pine64.dts  |    8 +-
 .../dts/allwinner/sun50i-a64-pinebook.dts     |    8 +-
 .../boot/dts/allwinner/sun50i-a64-pinetab.dts |   14 +-
 .../allwinner/sun50i-a64-sopine-baseboard.dts |    8 +-
 .../boot/dts/allwinner/sun50i-a64-teres-i.dts |    8 +-
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |   47 +-
 sound/soc/sunxi/sun8i-codec.c                 | 1056 +++++++++++++----
 12 files changed, 890 insertions(+), 297 deletions(-)

Comments

Maxime Ripard Feb. 17, 2020, 9:14 a.m. UTC | #1
Hi,

On Mon, Feb 17, 2020 at 12:42:16AM -0600, Samuel Holland wrote:
> The sun8i-codec driver, as used in the Allwinner A33 and A64, currently
> only exposes a small subset of the available hardware features. In order
> to use the A64 in a smartphone (the PinePhone), I've added the necessary
> functionality to the driver:
>   * The full set of supported DAI format options
>   * Support for AIF2 and AIF3
>   * Additional routing knobs
>   * Additional volume controls
>
> Unfortunately, due to preexisting issues with the driver, there are some
> breaking changes, as explained further in the commit messages:
>   * The LRCK inversion issue means we need a new compatible for the A64.
>   * Some controls are named inaccurately, so they are renamed.
>   * Likewise, the DAPM widgets used in device trees were either named
>     wrong, or the device trees were using the wrong widgets in the first
>     place. (Specifically, the links between the analog codec and digital
>     codec happen at the ADC and DAC, not AIF1.)
>
> I tended to take the philosophy of "while I'm breaking things, I might
> as well do them right", so I've probably made a few more changes than
> absolutely necessary. I'm not sure about where all of the policy
> boundaries are, about how far I should go to maintain compatibility. For
> example, for the DT widget usage, I could:
>   * Rename everything and update the DTS files (which is what I did)
>   * Keep the old (misleading/wrong) name for the widgets, but repurpose
>     them to work correctly
>       (i.e. "ADC Left" would be named "AIF1 Slot 0 Left ADC", but it
>        would work just like "ADC Left" does in this patchset)
>   * Keep the old widgets around as a compatibility layer, but add new
>     widgets and update the in-tree DTS files to use them
>       (i.e. "ADC Left" would have a path from "AIF1 Slot 0 Left ADC",
>        but "AIF1 Slot 0 Left ADC" would be a no-op widget)
>   * Something else entirely

I'm not sure this is really a concern here. We need to maintain the
compatibility with old DT's, but those will have an A33 compatible
too, and as far as I can see, you're not changing anything for that
compatible, so we're in the clear?

If not, then the third option would probably be the best, especially
since it's only a couple of them.

Maxime
Chen-Yu Tsai Feb. 17, 2020, 9:44 a.m. UTC | #2
On Mon, Feb 17, 2020 at 5:14 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> On Mon, Feb 17, 2020 at 12:42:16AM -0600, Samuel Holland wrote:
> > The sun8i-codec driver, as used in the Allwinner A33 and A64, currently
> > only exposes a small subset of the available hardware features. In order
> > to use the A64 in a smartphone (the PinePhone), I've added the necessary
> > functionality to the driver:
> >   * The full set of supported DAI format options
> >   * Support for AIF2 and AIF3
> >   * Additional routing knobs
> >   * Additional volume controls
> >
> > Unfortunately, due to preexisting issues with the driver, there are some
> > breaking changes, as explained further in the commit messages:
> >   * The LRCK inversion issue means we need a new compatible for the A64.
> >   * Some controls are named inaccurately, so they are renamed.
> >   * Likewise, the DAPM widgets used in device trees were either named
> >     wrong, or the device trees were using the wrong widgets in the first
> >     place. (Specifically, the links between the analog codec and digital
> >     codec happen at the ADC and DAC, not AIF1.)
> >
> > I tended to take the philosophy of "while I'm breaking things, I might
> > as well do them right", so I've probably made a few more changes than
> > absolutely necessary. I'm not sure about where all of the policy
> > boundaries are, about how far I should go to maintain compatibility. For
> > example, for the DT widget usage, I could:
> >   * Rename everything and update the DTS files (which is what I did)
> >   * Keep the old (misleading/wrong) name for the widgets, but repurpose
> >     them to work correctly
> >       (i.e. "ADC Left" would be named "AIF1 Slot 0 Left ADC", but it
> >        would work just like "ADC Left" does in this patchset)
> >   * Keep the old widgets around as a compatibility layer, but add new
> >     widgets and update the in-tree DTS files to use them
> >       (i.e. "ADC Left" would have a path from "AIF1 Slot 0 Left ADC",
> >        but "AIF1 Slot 0 Left ADC" would be a no-op widget)
> >   * Something else entirely
>
> I'm not sure this is really a concern here. We need to maintain the
> compatibility with old DT's, but those will have an A33 compatible
> too, and as far as I can see, you're not changing anything for that
> compatible, so we're in the clear?
>
> If not, then the third option would probably be the best, especially
> since it's only a couple of them.

Unfortunately the description for both chips are shared, and they're wrong.
So we probably need a new compatible (or a new driver)... or like options
2 or 3, keep the DT visible endpoints (but deprecate them), and route them
to a new set of proper widgets.

ChenYu
Maxime Ripard Feb. 17, 2020, 12:07 p.m. UTC | #3
On Mon, Feb 17, 2020 at 05:44:36PM +0800, Chen-Yu Tsai wrote:
> On Mon, Feb 17, 2020 at 5:14 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi,
> >
> > On Mon, Feb 17, 2020 at 12:42:16AM -0600, Samuel Holland wrote:
> > > The sun8i-codec driver, as used in the Allwinner A33 and A64, currently
> > > only exposes a small subset of the available hardware features. In order
> > > to use the A64 in a smartphone (the PinePhone), I've added the necessary
> > > functionality to the driver:
> > >   * The full set of supported DAI format options
> > >   * Support for AIF2 and AIF3
> > >   * Additional routing knobs
> > >   * Additional volume controls
> > >
> > > Unfortunately, due to preexisting issues with the driver, there are some
> > > breaking changes, as explained further in the commit messages:
> > >   * The LRCK inversion issue means we need a new compatible for the A64.
> > >   * Some controls are named inaccurately, so they are renamed.
> > >   * Likewise, the DAPM widgets used in device trees were either named
> > >     wrong, or the device trees were using the wrong widgets in the first
> > >     place. (Specifically, the links between the analog codec and digital
> > >     codec happen at the ADC and DAC, not AIF1.)
> > >
> > > I tended to take the philosophy of "while I'm breaking things, I might
> > > as well do them right", so I've probably made a few more changes than
> > > absolutely necessary. I'm not sure about where all of the policy
> > > boundaries are, about how far I should go to maintain compatibility. For
> > > example, for the DT widget usage, I could:
> > >   * Rename everything and update the DTS files (which is what I did)
> > >   * Keep the old (misleading/wrong) name for the widgets, but repurpose
> > >     them to work correctly
> > >       (i.e. "ADC Left" would be named "AIF1 Slot 0 Left ADC", but it
> > >        would work just like "ADC Left" does in this patchset)
> > >   * Keep the old widgets around as a compatibility layer, but add new
> > >     widgets and update the in-tree DTS files to use them
> > >       (i.e. "ADC Left" would have a path from "AIF1 Slot 0 Left ADC",
> > >        but "AIF1 Slot 0 Left ADC" would be a no-op widget)
> > >   * Something else entirely
> >
> > I'm not sure this is really a concern here. We need to maintain the
> > compatibility with old DT's, but those will have an A33 compatible
> > too, and as far as I can see, you're not changing anything for that
> > compatible, so we're in the clear?
> >
> > If not, then the third option would probably be the best, especially
> > since it's only a couple of them.
>
> Unfortunately the description for both chips are shared, and they're wrong.
> So we probably need a new compatible (or a new driver)... or like options
> 2 or 3, keep the DT visible endpoints (but deprecate them), and route them
> to a new set of proper widgets.

And hmm, it might be a bit wild, but since it's basically just a sed
on a string in DT, can't we leverage the dynamic DT stuff to rewrite
the property if we find the old one at probe? That would keep the
driver clean.

Maxime
Mark Brown Feb. 17, 2020, 4:26 p.m. UTC | #4
On Mon, Feb 17, 2020 at 12:42:16AM -0600, Samuel Holland wrote:

> There are several trivial fixes in here, and there are several commits
> that just add new features without changing any existing behavior, but
> there is enough changing that I thought it would be best to send the
> whole thing as an RFC. I'm more than happy to reorganize this into one
> or several patchsets in future revisions. It doesn't have to all go in
> at once.

This could definitely use being both split up and reordered, it's a 34
patch series as things stand which is just far too big and I don't
understand the ordering within the series - there's a mix of fixes,
cleanups and new features which should come in that order but don't.
This makes it difficult to get a handle on what's going on because what
the series is doing jumps about a lot.  There's also a lot of overuse of
fixes tags and stable tags which also makes things less clear.  I'd
suggest first sending all the clear fixes as a separate series with the
cleanups and new functionality separate.

With regard to the ABI breaks they *may* be OK for mainline if we're
confident that there's not going to be anyone broken by them but we
should be looking to maintain compatibility if we can even if that's the
case.