On Wed, Dec 08, 2021 at 12:12:24PM +0100, Cezary Rojewski wrote:

> A continuation of cleanup work of Intel SST solutions found in
> sound/soc/intel/. With two major chapters released last year catpt [1]
> and removal of haswell solution [2], time has come for Skylake-driver.

...

> Note: this series does not add fully functional driver as its size would
> get out of control. Focus is put on adding new code. A

So, I've spent some time looking at this but I think there's just
too much in this patch set for me to get through in a timely
fashion even with the efforts you've noted above in that
direction and that the best thing to do is to look at how to make
things a bit more managable.  It's a big series and the time of
year does mean time for review is a bit more limited.  From that
point of view I think the big thing to do is to reduce the amount
of interesting or new things that are being done and make the
series a simple as possible.  That'd be a limited but hopefully
routine driver which should be much easier to review and would
allow the more interesting bits to be focused on separately
without getting lost in the bulk of code that's more routine.
This applies more to bits at the top of the stack that interface
with the framework than DSP/hardware facing bits (eg, stuff like
the tracing is not really getting in the way).  Tactically the
code is basically fine, there's going to be some issues but
really it's the big picture stuff that needs more consideration.

In terms of things that could be split out there's a couple of
big things that jump out.

One is the paths code which feels like something that should
perhaps be pulled up a level to the framework since it feels like
the problems that it is addressing are general problems that all
DSPs face.  Doing something like hard coding this to some very
simple use case that does minimal to no processing would allow
the driver to load and function, then the path code can get a
proper review separately.

The other thing is the instantiating of multiple machine drivers
on a single system.  That's something I've seen occasionally from
other vendors and I do have concerns about how use cases where
someone wants to route audio in ways that result in cross links
between cards so those ended up being integrated.  The question
here isn't really if it works in testing (no matter how thorough
that testing is), the question is if userspace software doing
generic things will be confused by it and if some combination of
future framework changes and user creativity can turn up issues.
There's also the issue of how quirk handling would work in this
setup, and the issue with needing another set of machine drivers.
It's one point where input from users and distros would be
especially good.  This would be harder to cut out for later since
there's not so much code which supports it directly (TBH this is
part of the concern), one thing might be to just only support a
subset of hardware (eg, HDA only or I2S only) such that only one
machine driver can ever be instantiated on a system.

One more tactical thing is that I did comment on earlier was the
use of atomics - in general atomics are error prone and hard to
reason about, unless you're doing something like transferring the
audio data using PIO it's probably better to use higher level
concurrency primitives.  Any performance difference is unlikely
to register and the maintainability is a lot better.

It'd also be good to get this well enough integrated with the
intel-dsp-config code to avoid the need for the dependency on
SND_SOC_INTEL_SKYLAKE_FAMILY=n.  If both are built then it could
start off with always require a command line override to select
the new driver with a _DSP_DRIVER_AVS constant, this can be
revisited later.  That mechanism is really nice for distros and
users since it allows people to do binary distributions without
having to worry about committing to one driver or another,
reducing the risks for things like breakage on upgrade for some
small subset of machines.