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

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.

+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?

+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?

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.

+static void pcmidi_send_note(struct pcmidi_snd *pm, ...

• res = snd_rawmidi_receive(pm->in_substream, buffer, 3);

This return value is never used.

•                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.

+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.

+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.

+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.

• 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.

• 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.

Regards, Clemens