ML403 / ALSA driver for AC97 Controller

ML403 / ALSA driver for AC97 Controller

[Joachim Förster]

Hi all,

some days ago I "finished" a first working version of my ALSA driver for
Xilinx' AC97 Controller Reference ip core, usually used on Xilinx' ML403
board. In fact it's a kind of byproduct of another project I am
currently working on.

Anybody interested in the driver may download it here:
http://www.esic-solutions.com/support.html

The version number 0.0.1-pre1 indicates, that it is a kind of (early)
prerelease, which might be far from a "complete" state. So, if you have
any problems with it, feel free to mail me (JOFT@gmx.de). Or, if there
are any suggestions, opinions, special experience ... etc., let me know
it.

The tar file contains a README, with all relevant info - I hope I didn't
forget something important. Just one thing: Capture support basically
works, but there are overruns after ~30s with rates > ~20kHz at least on
the board and setup I'm working on => consider this to be a know bug.
Playback support (which was the primary goal) seems to be pretty stable.

I thought it would be a good idea to publish the driver, because over
the last few months, there were - not much - but several mails on this
list on sound support for the ML403.

 Joachim

Re: ML403 / ALSA driver for AC97 Controller

[Grant Likely]

On 7/21/07, Joachim F=F6rster <mls.JOFT@gmx.de> wrote:
> Hi all,

Thanks Joachim, this is interesting work.  I'd like to see this get
into mainline.

A few initial comments:
1. You should post this code as a patch against the kernel tree.
Links to tarballs are not the best way to get code reviewed.  Post
your patches to both the ALSA and linuxppc-dev mailing lists.
2. (get this out of the way quickly) Coding standard doesn't match the
kernel (indent w/ tabs, keep lines < 80 chars, etc).  You should run
your code through 'scripts/Lindent' in the kernel tree.  That will
sort out many of the whitespace issues.
3. In the same vein, c++ style comments need to be scrubbed.
4. Do not directly include xparameters.h in your driver.  The driver
should get it's data from the platform bus registration (of the
of_device registration when we move to arch/powerpc).
5. struct 'platform_device devices[]' needs to be moved to
arch/ppc/sysdev/virtex_devices.c
6. Have you looked into the new ALSA infrastructure which separates
Codecs from controllers (in sound/soc)?  It might be a good idea to go
that route for this driver.

That being said, it looks like good work.  Please cc me when you send
the next version.

Thanks,
g.

---
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: ML403 / ALSA driver for AC97 Controller

[Joachim Förster]

Hi Grant,

On Sat, 2007-07-21 at 14:17 -0600, Grant Likely wrote:
> A few initial comments:
> 1. You should post this code as a patch against the kernel tree.
> Links to tarballs are not the best way to get code reviewed.  Post
> your patches to both the ALSA and linuxppc-dev mailing lists.
> 2. (get this out of the way quickly) Coding standard doesn't match the
> kernel (indent w/ tabs, keep lines < 80 chars, etc).  You should run
> your code through 'scripts/Lindent' in the kernel tree.  That will
> sort out many of the whitespace issues.
> 3. In the same vein, c++ style comments need to be scrubbed.
> 4. Do not directly include xparameters.h in your driver.  The driver
> should get it's data from the platform bus registration (of the
> of_device registration when we move to arch/powerpc).
> 5. struct 'platform_device devices[]' needs to be moved to
> arch/ppc/sysdev/virtex_devices.c

About a week ago I finished work on the last four issues and in last
days I created several patches against different kernel trees. ATM I
have three patches, against:

tag v2.6.22 (Linus)
branch master (Linus)
branch virtex-dev (your v2.6.22 based branch)

I had to make different patches, because one version didn't apply
cleanly to the other branches, due to differences in a Kconfig file e.g.
and above all in the virtex_devices.c file.

Now, my question is: Which one should I post to the mailing list? (after
testing these patches - I haven't got a chance yet to test them on real
hardware - compilation is ok).

One more thing: I made two parts, one patch adds the driver and the
other one makes the registration with the platform bus. Is this ok? (I
saw this scheme in your virtex-dev branch.)

> 6. Have you looked into the new ALSA infrastructure which separates
> Codecs from controllers (in sound/soc)?  It might be a good idea to go
> that route for this driver.

Meanwhile, I had a _very_ short look into ASoC ... and I don't really
know ... My driver already uses the AC97 Layer of ALSA, so in some way
the codec is already separated from the controller. Xilinx' AC97
Controller Reference does have some very bad impact on the codec which
forced me to implement codec register shadowing ... hmmmm, I have to
look at it (ASoC) again - as soon as there is more (free)time ...

 Joachim

Re: ML403 / ALSA driver for AC97 Controller

[Grant Likely]

On 8/5/07, Joachim F=F6rster <mls.JOFT@gmx.de> wrote:
> tag v2.6.22 (Linus)
> branch master (Linus)
> branch virtex-dev (your v2.6.22 based branch)
>
> I had to make different patches, because one version didn't apply
> cleanly to the other branches, due to differences in a Kconfig file e.g.
> and above all in the virtex_devices.c file.
>
> Now, my question is: Which one should I post to the mailing list? (after
> testing these patches - I haven't got a chance yet to test them on real
> hardware - compilation is ok).

Post the one against master, ultimately that's where the code needs to
go.  I'll take care of any conflicts with my tree, and v2.6.22 is
irrelevant (at least as far as getting things into mainline is
concerned).  :-)
>
> One more thing: I made two parts, one patch adds the driver and the
> other one makes the registration with the platform bus. Is this ok? (I
> saw this scheme in your virtex-dev branch.)

Absolutely!  Splitting it up into logical changes is the right way to do it=
.

> Meanwhile, I had a _very_ short look into ASoC ... and I don't really
> know ... My driver already uses the AC97 Layer of ALSA, so in some way
> the codec is already separated from the controller. Xilinx' AC97
> Controller Reference does have some very bad impact on the codec which
> forced me to implement codec register shadowing ... hmmmm, I have to
> look at it (ASoC) again - as soon as there is more (free)time ...

Heh, okay.  I haven't dug into ASoC either, so I can't make too many commen=
ts.

Cheers,
g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: ML403 / ALSA driver for AC97 Controller

[Grant Likely]

On 8/5/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> On 8/5/07, Joachim F=F6rster <mls.JOFT@gmx.de> wrote:
> > tag v2.6.22 (Linus)
> > branch master (Linus)
> > branch virtex-dev (your v2.6.22 based branch)
> >
> > I had to make different patches, because one version didn't apply
> > cleanly to the other branches, due to differences in a Kconfig file e.g=
.
> > and above all in the virtex_devices.c file.
> >
> > Now, my question is: Which one should I post to the mailing list? (afte=
r
> > testing these patches - I haven't got a chance yet to test them on real
> > hardware - compilation is ok).
>
> Post the one against master, ultimately that's where the code needs to
> go.  I'll take care of any conflicts with my tree, and v2.6.22 is
> irrelevant (at least as far as getting things into mainline is
> concerned).  :-)
> >
> > One more thing: I made two parts, one patch adds the driver and the
> > other one makes the registration with the platform bus. Is this ok? (I
> > saw this scheme in your virtex-dev branch.)
>
> Absolutely!  Splitting it up into logical changes is the right way to do =
it.
>
> > Meanwhile, I had a _very_ short look into ASoC ... and I don't really
> > know ... My driver already uses the AC97 Layer of ALSA, so in some way
> > the codec is already separated from the controller. Xilinx' AC97
> > Controller Reference does have some very bad impact on the codec which
> > forced me to implement codec register shadowing ... hmmmm, I have to
> > look at it (ASoC) again - as soon as there is more (free)time ...
>
> Heh, okay.  I haven't dug into ASoC either, so I can't make too many comm=
ents.

One more thing; make sure you CC the ALSA list when you post.

Cheers,
g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: ML403 / ALSA driver for AC97 Controller

[Timur Tabi]

On Aug 5, 2007, at 4:00 AM, Joachim F=F6rster wrote:

> Meanwhile, I had a _very_ short look into ASoC ... and I don't really
> know ... My driver already uses the AC97 Layer of ALSA, so in some way
> the codec is already separated from the controller. Xilinx' AC97
> Controller Reference does have some very bad impact on the codec which
> forced me to implement codec register shadowing ... hmmmm, I have to
> look at it (ASoC) again - as soon as there is more (free)time ...

I've written an ASoC driver (not yet published, though).  ASoC is =20
good when the codec is mix-and-match with the transport mechanism (eg =20=

I2S or AC97), and the transport hardware is physically separate from =20
the DMA hardware.  Now, I don't know much about AC97 or the AC97 =20
driver in ALSA, so I can't tell you whether AC97 drivers in general =20
should be ASoC.

I'm on vacation this week.  Can you send me an email with a pointer =20
to your source code, because I think I lost the original post on this =20=

thread?  I'll take a look at it next week and let you know whether =20
you should consider ASoC.

Also, ALSA patches should be posted against the head of the ALSA =20
Mercurial tree, not Linus' 2.6.22 branch, which is already outdated =20
by ALSA standards.  And we should be having this conversation on alsa-=20=

devel.