Re: [PATCH] ALSA: cs5535audio: only build OLPC support if MGEODE_LXis defined

[Andres Salomon]

On Fri, 14 Nov 2008 13:45:50 -0500
Jeremy Katz <katzj@xxxxxxxxxx> wrote:

> On Fri, 2008-11-14 at 10:34 -0700, Jordan Crouse wrote:
> > Pavel Machek wrote:
> > >>>> i've zapped this patch meanwhile:
> > >>>>
> > >>>>  1355c96: x86/olpc: make CONFIG_OLPC dependent on
> > >>>> CONFIG_MGEODE_LX
> > >>>>
> > >>>> because it cripples the ability to run distribution kernels on
> > >>>> the OLPC.
> > >>> OK, I reverted also all relevant changes for cs5535audio driver
> > >>> now. The patches are saved in topic/cs5535audio branch, though.
> > >>>
> > >>> Let's fix OLPC-geode coupling first.
> > >> Hm, I'd really rather prefer this to be upstream.  The patch I
> > >> sent adds no regressions, allows the driver to happily coexist
> > >> with existing stuff, and *does* add support if you configure
> > >> OLPC with MGEODE_LX (generic kernels don't get the additional
> > >> benefits, but those configured specifically for OLPC do).
> > > 
> > > Yes, but the patch is also not a good way of going forward, so it
> > > should not be in mainline.
> > 
> > For the moment, this is a reasonable intermediate solution.  I
> > think the way forward will involve a great deal more work.
> 
> It's the work that should have been done from the beginning.  And once
> code is upstream, the motivation to make things "correct" drops

Wow.  You have it so backwards, it's not even funny.

When stuff is upstream, there is pressure from lots of people to make
things "correct".  When stuff isn't upstream, there's very little pressure;
that's how the ALSA stuff has managed to sit outside of the upstream
kernel for ages.  It's a tiny bit of code, easy to port to new kernels,
and therefore can happily be forgotten about in an out-of-tree kernel.

You say "it's work that should have been done from the beginning", and
yet this is the first time that I've heard anyone request it.  If
the geode code had remained out of mainline, I wouldn't have heard the
request at all.

I *really* don't want this to turn into OFW/promfs mess; attempts at
mainline inclusion fail, relevant parties who we contact ignore us
regarding the "correct" way forward, and therefore things sit outside
of mainline for ages with no progress whatsoever.


[...]
> > Also, while we are at it, we should be more specific and rename the 
> > hooks from geode_ to cs5536_, since there will soon be a system in
> > the wild with a MIPS processor and a CS5536.
> > 
> > But needless to say, this will be a goodly amount of work and churn
> > that will need some heavy testing in the OLPC kernel.  In the
> > meantime, Andres' fix will make the upstream kernel happier.
> 
> It is work, but I'm not sure it's huge amounts.  And by doing it
> *upstream first*, you benefit from the feedback and testing of the
> wider community.  Development of things in a silo is what tends to
> lead to these sorts of threads :/
> 

It is huge amounts, because it's API work.  If we were just twiddling
hardware bits, that would be easy.  However, we're trying to come up
with an interface that works not only with existing upstream drivers, but
also the remaining out-of-mainline drivers that we maintain.  I also
expressed interest in perhaps using the generic GPIO API for geode,
but that's something that requires a good deal of thinking about.  I
don't want to rush through it just to get the ALSA driver upstream.

The DCON driver has not gone upstream exactly because the GPIO API
does not work well for it right now (and out of tree, we were banging
on GPIO pins directly).  Obviously, any changes to the API should also
take the DCON into account; I'd rather fix this all the *right* way
than to just write a quick patch that munges the API to make you happy,
and then have to have even more code churn later to completely redo
the API.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/