Date:   Mon, 17 Feb 2020 13:07:19 +0100
From:   Maxime Ripard <maxime@...no.tech>
To:     Chen-Yu Tsai <wens@...e.org>
Cc:     Samuel Holland <samuel@...lland.org>,
        Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Vasily Khoruzhick <anarsoul@...il.com>,
        Mylène Josserand 
        <mylene.josserand@...e-electrons.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Linux-ALSA <alsa-devel@...a-project.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 00/34] sun8i-codec fixes and new features

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

Powered by blists - more mailing lists