Re: [alsa-cvslog] CVS: alsa-kernel/pci/ca0106 ca_midi.c,...

Re: [alsa-cvslog] CVS: alsa-kernel/pci/ca0106 ca_midi.c,...

[Clemens Ladisch]

James Courtier-Dutton wrote:
> Log Message:
> Summary: snd-ca0106: Add midi support.

Some nitpicks:

> void ca_midi_interrupt(ca_midi_t *midi, unsigned int status) {
> 	...
> 	if ((status & midi->ipr_tx) && ca_midi_output_ready(midi)) {
> 		if (midi->substream_output &&
> 		    snd_rawmidi_transmit(midi->substream_output, &byte, 1) == 1) {
> 			ca_midi_write_data(midi, byte);
> 		} else {
> 			midi->interrupt_disable(midi,midi->tx_enable);
> 		}
> 	}

Shouldn't this be a while loop so that more than one byte can be
output in one interrupt?

> +#ifdef CONFIG_SND_DEBUG_DETECT
> +	printk("ca0106: probe for MIDI channel A ...");
> +#endif

Please use snd_printdd() here.


Regards,
Clemens



-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [alsa-cvslog] CVS: alsa-kernel/pci/ca0106 ca_midi.c,...

[Takashi Iwai]

At Fri, 21 Oct 2005 11:00:03 +0200 (METDST),
Clemens Ladisch wrote:
> 
> James Courtier-Dutton wrote:
> > Log Message:
> > Summary: snd-ca0106: Add midi support.
> 
> Some nitpicks:
> 
> > void ca_midi_interrupt(ca_midi_t *midi, unsigned int status) {
> > 	...
> > 	if ((status & midi->ipr_tx) && ca_midi_output_ready(midi)) {
> > 		if (midi->substream_output &&
> > 		    snd_rawmidi_transmit(midi->substream_output, &byte, 1) == 1) {
> > 			ca_midi_write_data(midi, byte);
> > 		} else {
> > 			midi->interrupt_disable(midi,midi->tx_enable);
> > 		}
> > 	}
> 
> Shouldn't this be a while loop so that more than one byte can be
> output in one interrupt?

Another nitpics:

Please no brace in the case of one-line if (as a convention).


> +void ca0106_midi_interrupt_enable(ca_midi_t *midi, int intr){
> +	snd_ca0106_intr_enable((ca0106_t *)(midi->dev_id), intr);
> +}

Please put the opening brace in the next line.
Also, this and the following functions can be static.


But, what is the reason to call them indirectly via pointers at all?
The routine is just for ca0106, so no complex abstraction is
necessary.


Takashi


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [alsa-cvslog] CVS: alsa-kernel/pci/ca0106 ca_midi.c,...

[James Courtier-Dutton]

Takashi Iwai wrote:

>
>But, what is the reason to call them indirectly via pointers at all?
>The routine is just for ca0106, so no complex abstraction is
>necessary.
>
>
>Takashi
>
>
>  
>
The same ca_midi.c file should work with the emu10k1x, sb live, sb 
audigy1/2.
Once this midi code is tested well, we can then share the code amongst 
all the different modules.
So the indirection is for that purpose.

James



-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [alsa-cvslog] CVS: alsa-kernel/pci/ca0106 ca_midi.c,...

[Takashi Iwai]

At Fri, 21 Oct 2005 12:45:53 +0100,
James Courtier-Dutton wrote:
> 
> Takashi Iwai wrote:
> 
> >
> >But, what is the reason to call them indirectly via pointers at all?
> >The routine is just for ca0106, so no complex abstraction is
> >necessary.
> >
> >
> >Takashi
> >
> >
> >  
> >
> The same ca_midi.c file should work with the emu10k1x, sb live, sb 
> audigy1/2.
> Once this midi code is tested well, we can then share the code amongst 
> all the different modules.
> So the indirection is for that purpose.

For such a purpose, we can merge it to mpu401_uart.c, instead.
Many codes are same, and mpu401_uart has already indirect read/write
function pointers.  Only additional irq_enable/disable callbacks would
be required.


Takashi


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [alsa-cvslog] CVS: alsa-kernel/pci/ca0106 ca_midi.c,...

[Tilman Kranz]

Takashi Iwai wrote:

>At Fri, 21 Oct 2005 12:45:53 +0100,
>James Courtier-Dutton wrote:
>  
>
>>Takashi Iwai wrote:
>>    
>>
>>>But, what is the reason to call them indirectly via pointers at all?
>>>The routine is just for ca0106, so no complex abstraction is
>>>necessary.
>>>      
>>>
>>The same ca_midi.c file should work with the emu10k1x, sb live, sb 
>>audigy1/2.
>>Once this midi code is tested well, we can then share the code amongst 
>>all the different modules.
>>So the indirection is for that purpose.
>>    
>>
>For such a purpose, we can merge it to mpu401_uart.c, instead.
>Many codes are same, and mpu401_uart has already indirect read/write
>function pointers.  Only additional irq_enable/disable callbacks would
>be required.
>  
>
Hello.

First of all, I just tried ALSA CVS alsa-driver and alsa-kernel with my
Audigy LS model SB0312. MIDI I/O with the card works. Have many thanks
for letting those changes in.

... plays a little tune of gratitude...

I now would like to comment on what I did.

I am perfectly aware of my ca_midi.c being the at least 2nd reimplementation
of an MPU401 driver and I want to apologize for this introduction
of redundancy.

My implementation was result-driven and I wanted to go back to the working
implementation that is most close to my board. It happened to be emu10k1x.
I could reuse the algorithms 1:1.

Function pointers in ca_midi_t were introduced to enable emu10k1x to use
the code as well, if they want to. As opposed to emu10k1x, every read/write
dereferences the ca_midi_t to the hardware specific read_write which, as
Takashi Iwai already pointed out, happens to be the way mpu401 does it 
as well.

Greetings,
Tilman.



-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl