ML403 / ALSA driver for AC97 Controller
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 ([email protected]). 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
Quoted 1 line · On 7/21/07, Joachim F=F6rster <[email protected]> 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.
[email protected]
(403) 399-0195
Hi Grant,
Quoted 6 lines · Grant Likely
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
Quoted 8 lines
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 includexparameters.hin your driver. The driver should get it's data from the platform bus registration (of theof_deviceregistration when we move toarch/powerpc).
5. struct 'platform_devicedevices[]' needs to be moved toarch/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.)
Quoted 3 lines
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
Quoted 11 lines · On 8/5/07, Joachim F=F6rster <[email protected]> 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
Kconfigfile e.g. and above all in thevirtex_devices.cfile.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). :-)
Quoted 4 lines
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= .
Quoted 6 lines
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.
[email protected]
(403) 399-0195
Quoted 7 lines · Grant Likely
Quoted 6 lines · On 8/5/07, Joachim F=F6rster <[email protected]> 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
Kconfigfile e.g=
.
Quoted 3 lines
Quoted 3 lines
and above all in the
virtex_devices.cfile.Now, my question is: Which one should I post to the mailing list? (afte=
r
Quoted 8 lines
Quoted 2 lines
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). :-)
Quoted 1 line
Quoted 5 lines
Quoted 3 lines
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.
Quoted 9 lines
Quoted 7 lines
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.
[email protected]
(403) 399-0195
Quoted 6 lines · 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.