Re: [alsa-devel] [PATCH 2.6.34-rc5] HID: Prodikeys PC-MIDI HID Driver

[Don Prince <dhprince.devel@yahoo.co.uk>]

On 06/05/10 07:30, Clemens Ladisch wrote:
> dhprince.devel@yahoo wrote:
>   
>> A specialised HID driver for the CreativeLabs Prodikeys PC-MIDI USB 
>> Keyboard.
>> ...
>>     
> I cannot comment on the input stuff, but the sound stuff looks good
> overall.
>   

Thanks my first linux coding.

>   
>> +What:        /sys/bus/hid/drivers/prodikeys/.../channel
>> +Date:        April 2010
>> +KernelVersion:    2.6.34
>> +Contact:    Don Prince <dhprince.devel@yahoo.co.uk>
>> +Descripts/checkpatch.plscription:
>>     
> Huh?
>
>   

Fixed. I am being plagued by spurious button presses due to I suspect
conflicts between
my KVM switch and PS/2 to USB keyboard/Mouse converter.

>> +What:        /sys/bus/hid/drivers/prodikeys/.../sustain
>> +Description:
>> +        Allows control (via software) the sustain duration of a
>> +        note held by the pc-midi driver.
>>     
> Why is this feature needed?  Does the device report key releases
> incorrectly?
>
>   

The keyboard pretends to support sustain. The driver actually does it.

> These three sysfs controls could also be implemented as mixer controls.
> This would allow them to be automatically saved and restored when the
> computer is shut down.
>
>   
>> +    { HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS, 
>> USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
>>     
> Your mailer wrapped long lines and killed the tabs.
> Please see <Documentation/email-clients.txt>.
>
> And your code looks as if it does not use eight-column tabs for
> indention.
>
>   

The code is written using 8 column tabs but you are right thunderbird
messed it up. I believe I fixed  thunderbird now.

>> +static void pcmidi_send_note(struct pcmidi_snd *pm,
>> ...
>> +    res = snd_rawmidi_receive(pm->in_substream, buffer, 3);
>>     
> This return value is never used.
>
>   

Fixed.

>> +                    if (!atomic_read(&pms->in_use)) {
>> +                        pms->status = status;
>> +                        pms->note = note;
>> +                        pms->velocity = velocity;
>> +                        atomic_set(&pms->in_use, 1);
>>     
> If you're using the atomic variable to protect against some concurrently
> executing code, then you have a race condition in the time interval
> between the read and the set.
>
>   

Fixed. It was me being overkill, they are actually redundant.

>> +static void pcmidi_out_tasklet(unsigned long data)
>> ...
>> +    while (1) {
>> +        /* just read them and drop them */
>> +        u8 b;
>> +        if (snd_rawmidi_transmit(pm->out_substream, &b, 1) != 1) {
>> +            pm->out_active = 0;
>> +            break;
>> +        }
>>     
> This isn't quite how output ports are supposed to be implemented.  :-)
>
> I'd guess you want to remove the OUTPUT and DUPLEX flags and to drop all
> output-related functions.
>
>   
I know, it was naff, but without it the I get this from "amidi"

$ amidi -l

Dir Device    Name

cannot get rawmidi information 2:0: No such file or directory


Is it supposed to print this?

Although the midi still works.

$ amidi -p hw:2,0 --dump

90 2F 5C

80 2F 7F

90 30 52

80 30 7F

90 32 00

80 32 7F


The test code now looks like this

/*dhp	snd_component_add(card, "MIDI");*/
	err = snd_rawmidi_new(card, card->shortname, 0,
			      0, 1, &rwmidi);
	if (err < 0) {
		pk_error("failed to create pc-midi rawmidi device: error %d\n",
			err);
		goto fail;
	}
	pm->rwmidi = rwmidi;
	strncpy(rwmidi->name, card->shortname, sizeof(rwmidi->name));
	rwmidi->info_flags = SNDRV_RAWMIDI_INFO_INPUT /*dhp|
				SNDRV_RAWMIDI_INFO_OUTPUT |
				SNDRV_RAWMIDI_INFO_DUPLEX*/;
	rwmidi->private_data = pm;

/*dhp	snd_rawmidi_set_ops(rwmidi, SNDRV_RAWMIDI_STREAM_OUTPUT,
		&pcmidi_out_ops);
*/	snd_rawmidi_set_ops(rwmidi, SNDRV_RAWMIDI_STREAM_INPUT,
		&pcmidi_in_ops);


Other command output is printed below.

$ cat /proc/asound/cards

 0 [NVidia         ]: HDA-Intel - HDA NVidia

                      HDA NVidia at 0xdfdf8000 irq 21

 1 [HDMI           ]: HDA-Intel - HDA ATI HDMI

                      HDA ATI HDMI at 0xdffec000 irq 31

 2 [PCMIDI         ]: PC-MIDI - PC-MIDI

                      Prodikeys PC-MIDI Keyboard

$ aconnect -i

client 0: 'System' [type=kernel]

    0 'Timer           '

    1 'Announce        '

client 14: 'Midi Through' [type=kernel]

    0 'Midi Through Port-0'

client 24: 'PC-MIDI' [type=kernel]

    0 'PC-MIDI

$ aconnect -o

client 14: 'Midi Through' [type=kernel]

    0 'Midi Through Port-0'

client 128: 'FLUID Synth (Qsynth1)' [type=user]

    0 'Synth input port (Qsynth1:0)'

$ aconnect -iol

client 0: 'System' [type=kernel]

    0 'Timer           '

    1 'Announce        '

        Connecting To: 15:0

client 14: 'Midi Through' [type=kernel]

    0 'Midi Through Port-0'

client 24: 'PC-MIDI' [type=kernel]

    0 'PC-MIDI         '

        Connecting To: 128:0

client 128: 'FLUID Synth (Qsynth1)' [type=user]

    0 'Synth input port (Qsynth1:0)'

        Connected From: 24:0

$ amidi -L

RawMIDI list:

default {

        type hw

        card {

                @func getenv

                vars {

                        0 ALSA_RAWMIDI_CARD

                        1 ALSA_CARD

                }

                default {

                        @func refer

                        name 'defaults.rawmidi.card'

                }

        }

        device {

                @func igetenv

                vars {

                        0 ALSA_RAWMIDI_DEVICE

                }

                default {

                        @func refer

                        name 'defaults.rawmidi.device'

                }

        }

}

hw {

        @args.0 CARD

        @args.1 DEV

        @args.2 SUBDEV

        @args.CARD {

                type string

                default {

                        @func getenv

                        vars {

                                0 ALSA_RAWMIDI_CARD

                                1 ALSA_CARD

                        }

                        default {

                                @func refer

                                name 'defaults.rawmidi.card'

                        }

                }

        }

        @args.DEV {

                type integer

                default {

                        @func igetenv

                        vars {

                                0 ALSA_RAWMIDI_DEVICE

                        }

                        default {

                                @func refer

                                name 'defaults.rawmidi.device'

                        }

                }

        }

        @args.SUBDEV {

                type integer

                default -1

        }

        type hw

        card $CARD

        device $DEV

        subdevice $SUBDEV

        hint {

                description 'Direct rawmidi driver device'

                device $DEV

        }

}

virtual {

        @args.0 MERGE

        @args.MERGE {

                type string

                default 1

        }

        type virtual

        merge $MERGE

}



>> +static void pcmidi_in_trigger(struct snd_rawmidi_substream *substream, 
>> int up)
>> ...
>> +    if (up)
>> +        set_bit(substream->number, &pm->in_triggered);
>> +    else
>> +        clear_bit(substream->number, &pm->in_triggered);
>>     
> You have only one substream, a boolean variable would suffice.
>   

Fixed.

>   
>> +int pcmidi_snd_initialise(struct pcmidi_snd *pm)
>> ...
>> +    int out_ports = 1;
>> +    int in_ports = 1;
>>     
> These variables are not variable and therefore not needed.
>
>   

Fixed.

>> +    snd_component_add(card, "MIDI");
>>     
> This function is intended for sound cards that are composed of several
> components, and the component name is usually a chip name.  I think
> you don't need to call this function at all.
>
>   

Fixed.

>> +    err = snd_card_register(card);
>> ...
>> +    /* create sysfs variables */
>> +    err = device_create_file(&pm->pk->hdev->dev,
>> +                 sysfs_device_attr_channel);
>> ...
>> +    spin_lock_init(&pm->rawmidi_in_lock);
>> +
>> +    init_sustain_timers(pm);
>>     
> snd_card_register() makes all sound interfaces available to userspace.
> Initializations for any data that might be accessed from userspace must
> be done before that call.
>
>   

Fixed.

> Regards,
> Clemens
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
>   


		
___________________________________________________________ 
The all-new Yahoo! Mail goes wherever you go - free your email address from your Internet provider. http://uk.docs.yahoo.com/nowyoucan.html