At Wed, 20 Sep 2006 13:35:20 +0200,
Remy Bruno wrote:
>
> Hi,
>
> I modified the hdspm driver in order to support the RME AES32 card.
Great, thanks for the patch!
> As I'm not sure if it is possible to get the rights for modifying the hg
> repository,
In HG or GIT development, you don't need the right for modification of
upstream. The commitment on your local tree is basically independent
from the upstream. It's a complete different development style.
Of course, you can keep the old way (i.e. keeping changes
uncommitted), so that the sync with upstream works more easily like
before, too.
> I send the diff file attached (obtained with "diff -u -r", as
> neither "hg bundle" nor "hg outgoing -p" worked). This diff is against the last
> hg tree.
HG bundle or outgoing works only after you commit the changes on your
local tree. So, hg diff is the correct way to generate a patch.
> The modifications have been done on the basis of the RME Windows driver source
> code that RME sent to me (but that I am not allowed to send back).
Well, be careful about this statement.
You are not always allowed to "copy" the stuff from Windows source
code unless explicitly permitted by the provider. I think RME is one
of such linux-friendly guys, so I don't think it's too problematic,
though...
> The AES32
> and MADI cards are differentiated by the firmware revision. I tested the
> modified driver on an AES32 and (at least partly) validated it. Of course, the
> MADI part should not be concerned by my modifications, except for one point:
> the offset of HDSPM_statusRegister2. In the original hdspm file, it is set to
> 96, but when I look at the source code by RME, it is set to 48 (but increment
> is 32 bits, so it corresponds to 192), for AES32 as well as for MADI. So I set
> it to 192 for both cards. The new driver should thus be tested on the MADI to
> check if I didn't break this.
Yep.
> I also changed the hdsp driver: by default, precise_ptr was set to 1, which
> does not work on the cards I have (our company uses many hdsp cards, so I was
> able to check that this feature does not work at least on some cards). So I
> reset it to 0, which works on all the cards we have. Of course, it is still
> possible to change it back to 1 by using the appropriate control, but I think
> it is better to have default values that work for all cards (and furthermore, I
> don't see the purpose of this feature, as it works well without it...).
>
> Please let me know if my changes are integrated in the main tree.
The code changes look fine to me. But for merging to upstream, please
follow the standard coding style:
* 8 columns tab indent (hard tabs are preferred)
* Avoid C++ comments but use C-style
* Fold lines to fit within 80 chars as much as possible
* Don't put braces around a single if() line, i.e.
if (xxx)
bbb;
More details can be found in Documentation/CodingStyle in linux
kernel tree.
Also, some other nitpicking below:
> +// (status >> HDSPM_AES32_wcFreq_bit) & 0xF gives WC frequency
> +#define HDSPM_bit2freq(n) (\
> + (n)==1?32000:\
> + (n)==2?44100:\
> + (n)==3?48000:\
> + (n)==4?64000:\
> + (n)==5?88200:\
> + (n)==6?96000:\
> + (n)==7?128000:\
> + (n)==8?176400:\
> + (n)==9?192000:0)
Better to use an array and make it a static inline function.
> +static int hdspm_emphasis(struct hdspm * hdspm)
> +{
> + return (hdspm->control_register & HDSPM_Emphasis) ? 1 : 0;
> +}
The emphasis, dolby and professional bits are for IEC958 status bits,
right? If so, most drivers provide a generic control element
"IEC958 Playback Default" (in the source code, it's packed like
.name = SNDRV_CTL_NAME_IEC958("",PLAYBACK,DEFAULT)
). It's a control for encoding/decoding the iec958 status bits.
But, it's OK to use the controls as you implemented right now as the
first version. We can change this later.
> static void __devinit snd_hdspm_proc_init(struct hdspm * hdspm)
> {
> struct snd_info_entry *entry;
>
> if (!snd_card_proc_new(hdspm->card, "hdspm", &entry))
> snd_info_set_text_ops(entry, hdspm,
> - snd_hdspm_proc_read);
> + hdspm->is_aes32?
> + snd_hdspm_proc_read_aes32 :
> + snd_hdspm_proc_read_madi);
> + /* // debug file to read all hdspm registers
> + if (!snd_card_proc_new(hdspm->card, "debug", &entry))
> + snd_info_set_text_ops(entry, hdspm,
> + snd_hdspm_proc_read_debug); */
> }
If it's only for debugging, put #ifdef CONFIG_SND_DEBUG there.
> @@ -3468,9 +4361,14 @@
> pci_read_config_word(hdspm->pci,
> PCI_CLASS_REVISION, &hdspm->firmware_rev);
>
> + hdspm->is_aes32 = (hdspm->firmware_rev >= HDSPM_AESREVISION);
> +
> strcpy(card->driver, "HDSPM");
I guess it's better to give a different driver name here for AES32.
This is an identifier used in alsa-lib, and the card-specific
configuration is parsed by this string
(i.e. /usr/share/alsa/cards/*.conf is chosen by this).
> @@ -3496,6 +4394,11 @@
> (unsigned long)hdspm->iobase, hdspm->port,
> hdspm->port + io_extent - 1);
>
> + // Windows driver modifies the two following PCI registers
> + pci_write_config_byte(pci, PCI_COMMAND,
> + PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY);
> + pci_write_config_byte(pci, PCI_LATENCY_TIMER, 0xFF);
> +
PCI_COMMAND_MASTER is what pci_set_master() does.
The setting of LATENCY_TIMER and COMMAND_MEMORY are skeptical.
Avoid this unless you certainly need this setup.
thanks,
Takashi
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/alsa-devel
