Re: [PATCH] sst: Intel SST audio driver

[Liam Girdwood <lrg@slimlogic.co.uk>]

On Mon, 2010-10-18 at 10:10 +0200, Takashi Iwai wrote:
> At Mon, 18 Oct 2010 08:20:17 +0100,
> Mark Brown wrote:
> > 
> > On Mon, Oct 18, 2010 at 08:14:16AM +0200, Takashi Iwai wrote:
> > > Mark Brown wrote:
> > 
> > > > I do have some qualms about staging but given Greg's active policing of
> > > > it and work to push things into the actual trees the warnings about its
> > > > quality and general mainlineness are much more meaningful than those for
> > > > experimental.  There's also the tainting of kernels, which is really
> > > > useful.
> > 
> > > But who of the end user would care about it?  It's distributor and
> > > developer who care.
> > 
> > End users I'm not so worried about in the immediate instance, it's
> > mostly developers and in the embedded space there's way more of them
> > than end users.  A lot of the time end users don't get sufficent access
> > to the hardware to worry about kernels (though in this case that's less
> > likely to be the case).
> > 
> > > And, is the code quality is in question, which is supposed to be
> > > fixable there?  Your argument against this driver doesn't sound like
> > > fixable.
> > 
> > What makes you think these problems are unfixable?  It should be fairly
> > straightforward to pull out the CODEC and board parts of the driver and
> > factor them out - as I said last night the ASoC CPU side support is a
> > very thin wrapper around vanilla ALSA.  The code and board sections of
> > the code look fairly basic and straightforward to redo within the ASoC
> > APIs.  I don't think this is an unreasonably difficult thing to do and
> > it's certainly nowhere near starting from scratch.
> > 
> > > So then why you do you think keeping that stuff in staging tree is OK
> > > while sound tree is not?  It's a question to be or not to be.
> > > End-users never hesitate using the staging if it works.  They don't
> > > notice the difference; they don't notice kernel taint either.
> > 
> > Like I say, I have some qualms about staging but they're pretty minor.
> > The idea of accepting embedded drivers which don't use ASoC seems like a
> > massive step back compared to where what we've got right now, and is
> > going down the path of accepting drivers that don't even sit within
> > ALSA.
> > 
> > Currently with Linux we're able to say that if a CPU is supported and a
> > CODEC is supported then we can use the two together.  That's great, and
> > it's good for all concerned.  If we're going to start accepting vendor
> > specific audio stacks we'll no longer be in a position to say that.  We
> > will be back to having to talk about specific combinations of devices
> > working together.
> > 
> > > The staging driver is also in upstream; distribution enables it almost
> > > always, no matter how the quality of each driver is.  For developers,
> > > the distinction is clear, of course.  But for the rest, it's not.
> > 
> > The biggest issues here surround developers.
> > 
> > > So, if you really don't want this style of implementation, you'll have
> > > to take an action for getting rid of the driver from staging tree.
> > > Otherwise others will submit the similar drivers, and they'll land
> > > there, too.  Then we can keep the stuff in sound or other git tree
> > > in another branch until the things gets sorted out.
> > 
> > One of the nice things about staging (indeed the key thing from my point
> > of view) is that Greg does from time to time remove drivers from staging
> > which look like they're not making any progress towards leaving staging.
> > In contrast drivers in the main tree generally sit there indefinitely
> > with minimal impetus to improve them.
> > 
> > > > I'm sorry I don't really understand what you're saying here.  The
> > > > encoder offload stuff is all totally orthogonal to the issue of using
> > > > the ASoC APIs.  On the CPU side the ASoC API is mostly a thin wrapper
> > > > around the standard ALSA API so anything that works in ALSA should slot
> > > > fairly readily into ASoC.  What I'd expect to see happen is that the
> > > > CODEC and board stuff would get pulled out and everything else would
> > > > stay pretty much as-is, including the DSP.
> > 
> > > > CPUs like OMAP (which are obviously already in the ASoC framework) also
> > > > support this sort of stuff, the issues with mainline for them have been
> > > > around the API to the DSP, though it looks like the TI stuff in staging
> > > > is getting sorted out now.
> > 
> > > Well, what I don't understand in your argument is why we must stop it
> > > being deployed.
> > 
> > Because it's not using the relevant framework at all, it's gone and
> > reinvented the wheel without a pressing reason to do so and this will
> > be very likely to create problems if the part is at all successful.  It
> > will also set a really bad precedent for other vendors which is even
> > more worrying than the specific driver.
> > 
> > Throughout the kernel we regularly have to push back on embedded vendors
> > (not just CPU vendors) who produce drivers outside the standard
> > frameworks and get them converted to the standard frameworks so that
> > they can be deployed without having to go into driver specific APIs all
> > the time.  Due to the fact that everything gets merged into one mainline
> > tree Linux has much higher standards all round with this sort of thing
> > than most other OSs but it takes work to enforce that.
> > 
> > This does include a bunch of the recently posted Intel drivers,
> > including this one, but there's nothing particularly unusual here.
> > 
> > >                  Because it doesn't match with the current design of
> > > the framework?  If so, a straight question must come next: can't the
> > > framework itself be extended to suit with such hardware?
> > 
> > As I wrote several times I can see no problem supporting this hardware
> > with current ASoC.  I'm not sure how I can be any clearer on this point.
> > 
> > > > I'm saying I see nothing about this hardware which should prevent it
> > > > working with ASoC right now.  As I have said in terms of the overall
> > > > structure of the device it's not really doing anything that hasn't been
> > > > done elsewhere.  There's stuff we'd have to bodge in the short term, but
> > > > only fairly small bits that can be handled fairly straightforwardly.
> > 
> > > OK, then we should fix this first.
> > 
> > This is all driver specific stuff, there's nothing that needs doing
> > here immediately outside of the driver that I can spot right now.
> 
> ... so the whole messages can be reduced to a sentense:
>   "Rewrite the driver based on ASoC!"
> 
> Then it's a question to Intel guys.  If they would like to support for
> it, we can happily wait for it before merging.  If they don't want but
> keep as is, then questions are:
>  - whether to keep it in upstream or not
>  - where to keep it, in staging or in sound tree
> 

I've just had a look at the code and my fear is here that this will be
dumped in it's current state and forgotten about when Intel moves onto
their next big thing(tm). I hope I'm wrong though....

I also have to echo Mark and Pavel's concerns here that re-inventing the
wheel in some areas is both unnecessary and divisive. Subsystem
frameworks are one of the kernels key strengths and this lessens that to
a degree.

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk