Kernel: 2.4.17
Module: trident.c
Harware tested: ALi 1647/1535D+ (M5451+ALC100P)
Purpose of the patch: Add /proc/driver/{vendor}/{card}/ac97-{number}
Revision of the patch: 2

IMPORTANT: This mail contains some text repeated in the mail about the
patch in es1371.c module. The repetition is because the mail goes to
different maintainers.

This (2nd iteration) patch fixes what Alan pointed out in previous patch:
1) I blindly forgot to remove the proc entries when removing the module.
2) I didn't realize the code already supports 2 codecs and the spec talks
about 4 codecs.

I used /proc/driver/{vendor}/{card}/ac97-{number} and not:
/proc/driver/{vendor}/{card}/ac97/{number} as suggested in ac97_codec module

comments. Why?

1) Other modules uses it for proc entries and /dev entries.
2) The code is simpler, I'm just incrementing an ASCI 0 at the end of the
string. It gives upto 10 nice names and upto 208 different names, looks
enough for the current spec that talks about 4 codecs (which can contain a
total of 4*4*2=32 channels). I can change it if needed.
3) Current implementations (see emu10k1/main.c) creates:
/proc/driver/{vendor}/{card}/ac97 it means ac97 is a file, I use the
suggested format ac97/%d the ac97 will become a directory. I see it
confusing and also complicates user space programs trying to list codecs.

This revision also logs a warning if the proc entries couldn't be created. I

consider it a warning because the sound is fully functional without it. If
any of the entries fails to be created it isn't a problem because the remove

routine will try to remove all the entries.
I also followed Tommy Reynolds suggestion about using vmalloc/vfree for the
temporal
buffer. Note the size of the buffer is unknown because the PCI ID could
change their size.

Q1: The module creates /proc/ALiM5451 (or something similar entry) to setup
some codec options. I don't think that's the right place and could conflict
if someday more than one M5451 "chip" can exist in the same machine. I think

this entry should be moved to something like:
/proc/driver/{vendor}/{card}/options

Q2: Should I wrap all the stuff with the "define label" used to indicate
that proc support was enabled in the kernel? emu10k1 (the module I took as
reference) doesn't do it.

Q3: As this entry gives important information about the codec and the code
is
in ac97_codec module (it means the overhead imposed to the other modules is
small) I think all the modules using codecs should register it. Now: should
we move the code that creates the entries to ac97_codec to simplify the
other
modules?

SET

--
Salvador Eduardo Tropea (SET). (Electronics Engineer)
Visit my home page: http://welcome.to/SetSoft or
http://www.geocities.com/SiliconValley/Vista/6552/
Alternative e-mail: set@computer.org set@ieee.org
Address: Curapaligue 2124, Caseros, 3 de Febrero
Buenos Aires, (1678), ARGENTINA Phone: +(5411) 4759 0013