[PATCH] Support for ESI Miditerminal 4140

[PATCH] Support for ESI Miditerminal 4140

[Matthias Koenig]

[-- Attachment #1: Type: text/plain, Size: 163 bytes --]

Hello,

Here is the patch to add support for the ESI Miditerminal 4140.
Patch is against latest alsa-driver hg.

Signed-off-by: Matthias Koenig <mk@phasorlab.de>


[-- Attachment #2: mts64 driver --]
[-- Type: text/x-patch, Size: 32592 bytes --]

diff -r 4e7af6120ca7 drivers/Kconfig
--- a/drivers/Kconfig	Fri Apr 21 12:40:49 2006 +0200
+++ b/drivers/Kconfig	Mon Apr 24 19:17:13 2006 +0200
@@ -43,3 +43,10 @@ config SND_PCSP
 	  You can compile this as a module, which will be called snd-pcsp.
 
 	  You don't need this driver if you only want your computer to beep.
+
+config SND_MTS64
+	tristate "ESI Miditerminal 4140 driver"
+	depends on SND && PARPORT
+	select SND_RAWMIDI
+	help
+	  Say 'Y' or 'M' to include support for Miditerminal 4140.
diff -r 4e7af6120ca7 drivers/Makefile
--- a/drivers/Makefile	Fri Apr 21 12:40:49 2006 +0200
+++ b/drivers/Makefile	Mon Apr 24 19:17:13 2006 +0200
@@ -10,12 +10,13 @@ snd-serialmidi-objs := serialmidi.o
 snd-serialmidi-objs := serialmidi.o
 snd-aloop-objs := aloop.o
 snd-portman2x4-objs := portman2x4.o
+snd-mts64-objs := mts64.o
 
 obj-$(CONFIG_SND) += pcsp/
 
 obj-$(CONFIG_SND_SERIALMIDI) += snd-serialmidi.o
 obj-$(CONFIG_SND_LOOPBACK) += snd-aloop.o
-obj-$(CONFIG_SND_PORTMAN2X4) += snd-portman2x4.o
+obj-$(CONFIG_SND_MTS64) += snd-mts64.o
 
 include $(SND_TOPDIR)/alsa-kernel/drivers/Makefile
 
diff -r 4e7af6120ca7 drivers/mts64.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/mts64.c	Mon Apr 24 19:17:13 2006 +0200
@@ -0,0 +1,1157 @@
+/*     
+ *   ALSA Driver for Ego Systems Inc. (ESI) Miditerminal 4140
+ *   Copyright (c) 2006 by Matthias König <mk@phasorlab.de>
+ *
+ *   This program is free software; you can redistribute it and/or modify 
+ *   it under the terms of the GNU General Public License as published by 
+ *   the Free Software Foundation; either version 2 of the License, or 
+ *   (at your option) any later version. 
+ *
+ *   This program is distributed in the hope that it will be useful, 
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of 
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#include <sound/driver.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/parport.h>
+#include <linux/spinlock.h>
+#include <linux/delay.h>
+#include <sound/core.h>
+#include <sound/initval.h>
+#include <sound/rawmidi.h>
+#include <sound/control.h>
+
+#define CARD_NAME "Miditerminal 4140"
+#define DRIVER_NAME "MTS64"
+#define PLATFORM_DRIVER "snd_mts64"
+
+static int index[SNDRV_CARDS]  = SNDRV_DEFAULT_IDX;
+static char* id[SNDRV_CARDS]   = SNDRV_DEFAULT_STR;
+static int enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE_PNP;
+
+static struct platform_device* platform_devices[SNDRV_CARDS]; 
+static int device_count;
+
+module_param_array(index, int, NULL, S_IRUGO);
+MODULE_PARM_DESC(index, "Index value for " CARD_NAME " soundcard.");
+module_param_array(id, charp, NULL, S_IRUGO);
+MODULE_PARM_DESC(id, "ID string for " CARD_NAME " soundcard.");
+module_param_array(enable, bool, NULL, S_IRUGO);
+MODULE_PARM_DESC(enable, "Enable " CARD_NAME " soundcard.");
+
+MODULE_AUTHOR("Matthias Koenig <mk@phasorlab.de>");
+MODULE_DESCRIPTION("ESI Miditerminal 4140");
+MODULE_LICENSE("GPL");
+MODULE_SUPPORTED_DEVICE("{{ESI,Miditerminal 4140}}");
+
+/*********************************************************************
+ * Chip specific
+ *********************************************************************/
+#define MTS64_NUM_INPUT_PORTS 5
+#define MTS64_NUM_OUTPUT_PORTS 4
+#define MTS64_SMPTE_SUBSTREAM 4
+
+struct mts64 {
+	spinlock_t lock;
+	struct snd_card* card;
+	snd_rawmidi_t* rmidi;
+	struct pardevice* pardev;
+	int pardev_claimed;
+
+	int open_count;
+	int current_midi_output_port;
+	int current_midi_input_port;
+	unsigned char mode[MTS64_NUM_INPUT_PORTS];
+	snd_rawmidi_substream_t* midi_input_substream[MTS64_NUM_INPUT_PORTS];
+	int smpte_switch;
+	unsigned char time[4]; /* [0]=hh, [1]=mm, [2]=ss, [3]=ff */
+	unsigned char fps;
+};
+
+static int snd_mts64_free(struct mts64* mts)
+{
+	kfree(mts);
+	return 0;
+}
+
+static int __devinit snd_mts64_create(struct snd_card* card, 
+				      struct pardevice* pardev, 
+				      struct mts64** rchip)
+{
+	struct mts64* mts;
+	int i;
+
+	*rchip = NULL;
+
+	mts = kzalloc(sizeof(struct mts64), GFP_KERNEL);
+	if(mts == NULL) 
+		return -ENOMEM;
+
+	/* Init chip specific data */
+	spin_lock_init(&mts->lock);
+	mts->card = card;
+	mts->rmidi = NULL;      /* initialized later in mts64_attach */
+	mts->pardev = pardev;
+	mts->pardev_claimed = 0;
+	mts->open_count = 0;
+	mts->current_midi_output_port = -1;
+	mts->current_midi_input_port = -1;
+	for(i=0; i<MTS64_NUM_INPUT_PORTS; ++i)
+		mts->mode[i] = 0;
+	/* SMPTE */
+	mts->smpte_switch = 0;
+	mts->time[0] = 0;
+	mts->time[1] = 0;
+	mts->time[2] = 0;
+	mts->time[3] = 0;
+	mts->fps = 0;
+
+	*rchip = mts;
+
+	return 0;
+}
+
+/*********************************************************************
+ * HW register related constants
+ *********************************************************************/
+
+/* Status Bits */
+#define MTS64_STAT_BSY             0x80
+#define MTS64_STAT_BIT_SET         0x20  /* readout process, bit is set */
+#define MTS64_STAT_PORT            0x10  /* read byte is a port number */
+
+/* Control Bits */
+#define MTS64_CTL_READOUT          0x08  /* enable readout */
+#define MTS64_CTL_WRITE_CMD        0x06  
+#define MTS64_CTL_WRITE_DATA       0x02  
+#define MTS64_CTL_STROBE           0x01  
+
+/* Command */
+#define MTS64_CMD_RESET            0xfe
+#define MTS64_CMD_PROBE            0x8f  /* Used in probing procedure */
+#define MTS64_CMD_SMPTE_SET_TIME   0xe8
+#define MTS64_CMD_SMPTE_SET_FPS    0xee
+#define MTS64_CMD_SMPTE_STOP       0xef
+#define MTS64_CMD_SMPTE_FPS_24     0xe3
+#define MTS64_CMD_SMPTE_FPS_25     0xe2
+#define MTS64_CMD_SMPTE_FPS_2997   0xe4 
+#define MTS64_CMD_SMPTE_FPS_30D    0xe1
+#define MTS64_CMD_SMPTE_FPS_30     0xe0
+#define MTS64_CMD_COM_OPEN         0xf8  /* setting the communication mode */
+#define MTS64_CMD_COM_CLOSE1       0xff  /* clearing communication mode */
+#define MTS64_CMD_COM_CLOSE2       0xf5
+
+/*********************************************************************
+ * Hardware specific functions
+ *********************************************************************/
+static void mts64_enable_readout(struct parport* p);
+static void mts64_disable_readout(struct parport* p);
+static int mts64_device_ready(struct parport* p);
+static int mts64_device_init(struct parport* p);
+static int mts64_device_open(struct mts64* mts);
+static int mts64_device_close(struct mts64* mts);
+static unsigned char mts64_map_midi_input(unsigned char c);
+static int mts64_probe(struct parport* p);
+static unsigned short mts64_read(struct parport* p);
+static unsigned char mts64_read_char(struct parport* p);
+static void mts64_smpte_start(struct parport* p,
+			      unsigned char hours, unsigned char minutes,
+			      unsigned char seconds, unsigned char frames,
+			      unsigned char idx);
+static void mts64_smpte_stop(struct parport* p);
+static void mts64_write_command(struct parport* p, unsigned char c);
+static void mts64_write_data(struct parport* p, unsigned char c);
+static void mts64_write_midi(struct mts64* mts, unsigned char c, int midiport);
+
+
+/*  Enables the readout procedure
+ *
+ *  Before we can read a midi byte from the device, we have to set
+ *  bit 3 of control port.
+ */
+static void mts64_enable_readout(struct parport* p)
+{
+	unsigned char c;
+
+	c = parport_read_control(p);
+	c |= MTS64_CTL_READOUT;
+	parport_write_control(p, c); 
+}
+
+/*  Disables readout 
+ *
+ *  Readout is disabled by clearing bit 3 of control
+ */
+static void mts64_disable_readout(struct parport* p)
+{
+	unsigned char c;
+
+	c = parport_read_control(p);
+	c &= ~MTS64_CTL_READOUT;
+	parport_write_control(p, c);
+}
+
+/*  waits for device ready
+ *
+ *  Checks if BUSY (Bit 7 of status) is clear
+ *  1 device ready
+ *  0 failure
+ */
+static int mts64_device_ready(struct parport* p)
+{
+	int i;
+	unsigned char c;
+
+	for(i=0; i < 0xffff; ++i) {
+		c = parport_read_status(p);
+		c &= MTS64_STAT_BSY;
+		if(c != 0) return 1;
+	} 
+
+	return 0;
+}
+
+/*  Init device (LED blinking startup magic)
+ *
+ *  Returns:
+ *  1 init ok
+ *  0 failure
+ */
+static int mts64_device_init(struct parport* p)
+{
+	int i;
+
+	mts64_write_command(p, MTS64_CMD_RESET);
+
+	for(i=0; i < 64; ++i) {
+		mdelay(100);
+
+		if(mts64_probe(p)) {
+			/* success */
+			mts64_disable_readout(p);
+			return 1;
+		}
+	}
+
+	mts64_disable_readout(p);
+	return 0;
+}
+
+/* 
+ *  Opens the device (set communication mode) and increases the 
+ *  reference count.
+ */
+static int mts64_device_open(struct mts64* mts)
+{
+	int i;
+	struct parport* p = mts->pardev->port;
+
+	if(mts->open_count == 0) {
+		for(i=0; i<5; ++i)
+			mts64_write_command(p, MTS64_CMD_COM_OPEN);
+	}
+
+	++(mts->open_count);
+	
+	/* FIXME: Busy wait really necessary? */
+	mdelay(50);
+
+	return 0;
+}
+
+/*  
+ *  Close device and decrement reference count.
+ */
+static int mts64_device_close(struct mts64* mts)
+{
+	int i;
+	struct parport* p = mts->pardev->port;
+
+	--(mts->open_count);
+	if(mts->open_count == 0) {
+		for(i=0; i<5; ++i) {
+			mts64_write_command(p, MTS64_CMD_COM_CLOSE1);
+			mts64_write_command(p, MTS64_CMD_COM_CLOSE2);
+		}
+	} else if(mts->open_count < 0) {
+		mts->open_count = 0;
+	}
+
+	/* FIXME: Busy wait really necessary? */
+	mdelay(500);
+
+	return 0;
+}
+
+/*  map hardware port to substream number
+ * 
+ *  When reading a byte from the device, the device tells us
+ *  on what port the byte is. This HW port has to be mapped to
+ *  the midiport (substream number).
+ *  substream 0-3 are Midiports 1-4
+ *  substream 4 is SMPTE Timecode
+ *  The mapping is done by the table:
+ *  HW | 0 | 1 | 2 | 3 | 4 
+ *  SW | 0 | 1 | 4 | 2 | 3
+ */
+static unsigned char mts64_map_midi_input(unsigned char c)
+{
+	unsigned char map[] = { 0, 1, 4, 2, 3 };
+
+	return map[c];
+}
+
+
+/*  Probe parport for device
+ *
+ *  Do we have a Miditerminal 4140 on parport? 
+ *  Returns:
+ *  1 device found
+ *  0 no device
+ */
+static int mts64_probe(struct parport* p)
+{
+	unsigned char c;
+
+	mts64_smpte_stop(p);
+
+	mts64_write_command(p, MTS64_CMD_PROBE);
+
+	/* FIXME */
+	mdelay(50);
+	
+	c = mts64_read(p);
+
+	c &= 0x00ff;
+	if(c != MTS64_CMD_PROBE) return 0;
+	else return 1;
+
+}
+
+/*  Read a byte from device
+ *
+ *  Read byte incl. status from device
+ *
+ *  Returns:
+ *  data in lower 8 bits and status in upper 8 bits
+ */
+static unsigned short mts64_read(struct parport* p)
+{
+	unsigned char data, status;
+
+	mts64_device_ready(p);
+	mts64_enable_readout(p);
+	status = parport_read_status(p);
+	data = mts64_read_char(p);
+	mts64_disable_readout(p);
+
+	return (status << 8) | data;
+}
+
+/*  Read a byte from device
+ *
+ *  Note, that readout mode has to be enabled.
+ *  readout procedure is as follows: 
+ *  - Write number of the Bit to read to DATA
+ *  - Read STATUS
+ *  - Bit 5 of STATUS indicates if Bit is set
+ *
+ *  Returns:
+ *  Byte read from device
+ */
+static unsigned char mts64_read_char(struct parport* p)
+{
+	unsigned char c = 0;
+	unsigned char st;
+	unsigned char i;
+
+	for(i=0; i<8; ++i) {
+		parport_write_data(p, i);
+		c >>= 1;
+		st = parport_read_status(p);
+		if(st & MTS64_STAT_BIT_SET) 
+			c |= 0x80;
+	}
+	
+	return c;
+}
+
+/*  Starts SMPTE Timecode generation
+ *
+ *  The device creates SMPTE Timecode by hardware.
+ *  0 24 fps
+ *  1 25 fps
+ *  2 29.97 fps
+ *  3 30 fps (Drop-frame)
+ *  4 30 fps
+ */
+static void mts64_smpte_start(struct parport* p,
+			      unsigned char hours, unsigned char minutes,
+			      unsigned char seconds, unsigned char frames,
+			      unsigned char idx)
+{
+	unsigned char fps[5] = { MTS64_CMD_SMPTE_FPS_24, 
+				 MTS64_CMD_SMPTE_FPS_25,
+				 MTS64_CMD_SMPTE_FPS_2997, 
+				 MTS64_CMD_SMPTE_FPS_30D,
+				 MTS64_CMD_SMPTE_FPS_30    };
+
+	mts64_write_command(p, MTS64_CMD_SMPTE_SET_TIME);
+	mts64_write_command(p, frames);
+	mts64_write_command(p, seconds);
+	mts64_write_command(p, minutes);
+	mts64_write_command(p, hours);
+
+	mts64_write_command(p, MTS64_CMD_SMPTE_SET_FPS);
+	mts64_write_command(p, fps[idx]);
+}
+
+/*  Stops SMPTE Timecode generation
+ */
+static void mts64_smpte_stop(struct parport* p)
+{
+	mts64_write_command(p, MTS64_CMD_SMPTE_STOP);
+}
+
+/*  Write a command byte to device
+ */
+static void mts64_write_command(struct parport* p, unsigned char c)
+{
+	mts64_device_ready(p);
+
+	parport_write_data(p, c);
+
+	parport_write_control(p, MTS64_CTL_WRITE_CMD);
+	parport_write_control(p, MTS64_CTL_WRITE_CMD | MTS64_CTL_STROBE);
+	parport_write_control(p, MTS64_CTL_WRITE_CMD);
+}
+
+/*  Write a data byte to device 
+ */
+static void mts64_write_data(struct parport* p, unsigned char c)
+{
+	mts64_device_ready(p);
+
+	parport_write_data(p, c);
+
+	parport_write_control(p, MTS64_CTL_WRITE_DATA);
+	parport_write_control(p, MTS64_CTL_WRITE_DATA | MTS64_CTL_STROBE);
+	parport_write_control(p, MTS64_CTL_WRITE_DATA);
+}
+
+/*  Write a MIDI byte to midiport
+ *
+ *  midiport ranges from 0-3 and maps to Ports 1-4
+ *  assumptions: communication mode is on
+ *  
+ */
+static void mts64_write_midi(struct mts64* mts, unsigned char c,
+			     int midiport)
+{
+	struct parport* p = mts->pardev->port;
+
+	/* check current midiport */
+	if(mts->current_midi_output_port != midiport) {
+		mts64_write_command(p, midiport);
+	}
+
+	/* write midi byte */
+	mts64_write_data(p, c);
+}
+
+/*********************************************************************
+ * Control elements
+ *********************************************************************/
+
+/* SMPTE Switch */
+static int snd_mts64_ctl_smpte_switch_info(snd_kcontrol_t *kctl,
+					   snd_ctl_elem_info_t* uinfo)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
+	uinfo->count = 1;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = 1;
+	return 0;
+}
+
+static int snd_mts64_ctl_smpte_switch_get(snd_kcontrol_t* kctl,
+					  snd_ctl_elem_value_t* uctl)
+{
+	struct mts64* mts = snd_kcontrol_chip(kctl);
+	
+	uctl->value.integer.value[0] = mts->smpte_switch;
+	return 0;
+}
+
+static int snd_mts64_ctl_smpte_switch_put(snd_kcontrol_t* kctl,
+					  snd_ctl_elem_value_t* uctl)
+{
+	struct mts64* mts = snd_kcontrol_chip(kctl);
+	int changed = 0;
+	unsigned long flags;
+
+	if(mts->smpte_switch == uctl->value.integer.value[0])
+		return changed;
+
+	changed = 1;
+	mts->smpte_switch = uctl->value.integer.value[0];
+	if(mts->smpte_switch) {
+		spin_lock_irqsave(mts->lock, flags);
+		mts64_smpte_start(mts->pardev->port,
+				  mts->time[0], mts->time[1],
+				  mts->time[2], mts->time[3],
+				  mts->fps);
+		spin_unlock_irqrestore(mts->lock, flags);
+	} else {
+		spin_lock_irqsave(mts->lock, flags);
+		mts64_smpte_stop(mts->pardev->port);
+		spin_unlock_irqrestore(mts->lock, flags);
+	}
+
+	return changed;
+}
+
+static snd_kcontrol_new_t mts64_ctl_smpte_switch __devinitdata = {
+	.iface = SNDRV_CTL_ELEM_IFACE_RAWMIDI,
+	.name  = "SMPTE Playback Switch",
+	.index = 0,
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
+	.private_value = 0,
+	.info = snd_mts64_ctl_smpte_switch_info,
+	.get  = snd_mts64_ctl_smpte_switch_get,
+	.put  = snd_mts64_ctl_smpte_switch_put
+};
+
+/* Time */
+static int snd_mts64_ctl_smpte_time_h_info(snd_kcontrol_t* kctl,
+					   snd_ctl_elem_info_t* uinfo)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 1;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = 23;
+	return 0;
+}
+
+static int snd_mts64_ctl_smpte_time_f_info(snd_kcontrol_t* kctl,
+					   snd_ctl_elem_info_t* uinfo)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 1;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = 99;
+	return 0;
+}
+
+static int snd_mts64_ctl_smpte_time_info(snd_kcontrol_t* kctl,
+					 snd_ctl_elem_info_t* uinfo)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 1;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = 59;
+	return 0;
+}
+
+static int snd_mts64_ctl_smpte_time_get(snd_kcontrol_t* kctl,
+					snd_ctl_elem_value_t* uctl)
+{
+	struct mts64* mts = snd_kcontrol_chip(kctl);
+	int idx = kctl->private_value;
+
+	uctl->value.integer.value[0] = mts->time[idx];
+	return 0;
+}
+
+static int snd_mts64_ctl_smpte_time_put(snd_kcontrol_t* kctl,
+					snd_ctl_elem_value_t* uctl)
+{
+	struct mts64* mts = snd_kcontrol_chip(kctl);
+	int idx = kctl->private_value;
+	int changed = 0;
+
+	if(mts->time[idx] != uctl->value.integer.value[0]) {
+		changed = 1;
+		mts->time[idx] = uctl->value.integer.value[0];
+	}
+
+	return changed;
+}
+
+static snd_kcontrol_new_t mts64_ctl_smpte_time_hours __devinitdata = {
+	.iface = SNDRV_CTL_ELEM_IFACE_RAWMIDI,
+	.name  = "SMPTE Time Hours",
+	.index = 0,
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
+	.private_value = 0,
+	.info = snd_mts64_ctl_smpte_time_h_info,
+	.get  = snd_mts64_ctl_smpte_time_get,
+	.put  = snd_mts64_ctl_smpte_time_put
+};
+
+static snd_kcontrol_new_t mts64_ctl_smpte_time_minutes __devinitdata = {
+	.iface = SNDRV_CTL_ELEM_IFACE_RAWMIDI,
+	.name  = "SMPTE Time Minutes",
+	.index = 0,
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
+	.private_value = 1,
+	.info = snd_mts64_ctl_smpte_time_info,
+	.get  = snd_mts64_ctl_smpte_time_get,
+	.put  = snd_mts64_ctl_smpte_time_put
+};
+
+static snd_kcontrol_new_t mts64_ctl_smpte_time_seconds __devinitdata = {
+	.iface = SNDRV_CTL_ELEM_IFACE_RAWMIDI,
+	.name  = "SMPTE Time Seconds",
+	.index = 0,
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
+	.private_value = 2,
+	.info = snd_mts64_ctl_smpte_time_info,
+	.get  = snd_mts64_ctl_smpte_time_get,
+	.put  = snd_mts64_ctl_smpte_time_put
+};
+
+static snd_kcontrol_new_t mts64_ctl_smpte_time_frames __devinitdata = {
+	.iface = SNDRV_CTL_ELEM_IFACE_RAWMIDI,
+	.name  = "SMPTE Time Frames",
+	.index = 0,
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
+	.private_value = 3,
+	.info = snd_mts64_ctl_smpte_time_f_info,
+	.get  = snd_mts64_ctl_smpte_time_get,
+	.put  = snd_mts64_ctl_smpte_time_put
+};
+
+/* FPS */
+static int snd_mts64_ctl_smpte_fps_info(snd_kcontrol_t* kctl,
+					snd_ctl_elem_info_t* uinfo)
+{
+	char* texts[5] = { "24",
+			   "25",
+			   "29.97",
+			   "30D",
+			   "30"    };
+
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
+	uinfo->count = 1;
+	uinfo->value.enumerated.items = 5;
+	if(uinfo->value.enumerated.item > 4)
+		uinfo->value.enumerated.item = 4;
+	strcpy(uinfo->value.enumerated.name,
+	       texts[uinfo->value.enumerated.item]);
+	
+	return 0;
+}
+
+static int snd_mts64_ctl_smpte_fps_get(snd_kcontrol_t* kctl,
+				       snd_ctl_elem_value_t* uctl)
+{
+	struct mts64* mts = snd_kcontrol_chip(kctl);
+
+	uctl->value.enumerated.item[0] = mts->fps;
+	return 0;
+}
+
+static int snd_mts64_ctl_smpte_fps_put(snd_kcontrol_t* kctl,
+					snd_ctl_elem_value_t* uctl)
+{
+	struct mts64* mts = snd_kcontrol_chip(kctl);
+	int changed = 0;
+
+	if(mts->fps != uctl->value.enumerated.item[0]) {
+		changed = 1;
+		mts->fps = uctl->value.enumerated.item[0];
+	}
+	return changed;
+}
+
+static snd_kcontrol_new_t mts64_ctl_smpte_fps __devinitdata = {
+	.iface = SNDRV_CTL_ELEM_IFACE_RAWMIDI,
+	.name  = "SMPTE Fps",
+	.index = 0,
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
+	.private_value = 0,
+	.info  = snd_mts64_ctl_smpte_fps_info,
+	.get   = snd_mts64_ctl_smpte_fps_get,
+	.put   = snd_mts64_ctl_smpte_fps_put
+};
+
+
+static int __devinit snd_mts64_ctl_create(struct snd_card* card, 
+					  struct mts64* mts) 
+{
+	int err;
+
+	err = snd_ctl_add(card, 
+			  snd_ctl_new1(&mts64_ctl_smpte_switch, mts));
+	if(err < 0) {
+		snd_printk("Cannot create control: smpte_switch\n");
+		return err;
+	}
+
+	err = snd_ctl_add(card, 
+			  snd_ctl_new1(&mts64_ctl_smpte_time_hours, mts));
+	if(err < 0) {
+		snd_printk("Cannot create control: smpte_time_hours\n");
+		return err;
+	}
+	err = snd_ctl_add(card, 
+			  snd_ctl_new1(&mts64_ctl_smpte_time_minutes, mts));
+	if(err < 0) {
+		snd_printk("Cannot create control: smpte_time_minutes\n");
+		return err;
+	}
+	err = snd_ctl_add(card, 
+			  snd_ctl_new1(&mts64_ctl_smpte_time_seconds, mts));
+	if(err < 0) {
+		snd_printk("Cannot create control: smpte_time_seconds\n");
+		return err;
+	}
+	err = snd_ctl_add(card, 
+			  snd_ctl_new1(&mts64_ctl_smpte_time_frames, mts));
+	if(err < 0) {
+		snd_printk("Cannot create control: smpte_time_frames\n");
+		return err;
+	}
+
+	err = snd_ctl_add(card, 
+			  snd_ctl_new1(&mts64_ctl_smpte_fps, mts));
+	if(err < 0) {
+		snd_printk("Cannot create control: smpte_fps\n");
+		return err;
+	}
+
+	return 0;
+}
+
+/*********************************************************************
+ * Rawmidi
+ *********************************************************************/
+#define MTS64_MODE_INPUT_TRIGGERED 0x01
+
+static int snd_mts64_rawmidi_output_open(snd_rawmidi_substream_t* substream)
+{
+	struct mts64* mts = substream->rmidi->private_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(mts->lock, flags);
+	mts64_device_open(mts);
+	spin_unlock_irqrestore(mts->lock, flags);
+
+	return 0;
+}
+
+static int snd_mts64_rawmidi_output_close(snd_rawmidi_substream_t* substream)
+{
+	struct mts64* mts = substream->rmidi->private_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(mts->lock, flags);
+	mts64_device_close(mts);
+	spin_unlock_irqrestore(mts->lock, flags);
+
+	return 0;
+}
+
+static void snd_mts64_rawmidi_output_trigger(snd_rawmidi_substream_t* substream,
+					     int up)
+{
+	struct mts64* mts = substream->rmidi->private_data;
+	unsigned char data;
+	unsigned long flags;
+
+	spin_lock_irqsave(mts->lock, flags);
+	while(snd_rawmidi_transmit_peek(substream, &data, 1) == 1) {
+		mts64_write_midi(mts, data, substream->number+1);
+		snd_rawmidi_transmit_ack(substream, 1);
+	}
+	spin_unlock_irqrestore(mts->lock, flags);
+}
+
+static int snd_mts64_rawmidi_input_open(snd_rawmidi_substream_t* substream)
+{
+	struct mts64* mts = substream->rmidi->private_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(mts->lock, flags);
+	mts64_device_open(mts);
+	spin_unlock_irqrestore(mts->lock, flags);
+
+	return 0;
+}
+
+static int snd_mts64_rawmidi_input_close(snd_rawmidi_substream_t* substream)
+{
+	struct mts64* mts = substream->rmidi->private_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(mts->lock, flags);
+	mts64_device_close(mts);
+	spin_unlock_irqrestore(mts->lock, flags);
+
+	return 0;
+}
+
+static void snd_mts64_rawmidi_input_trigger(snd_rawmidi_substream_t* substream,
+					    int up)
+{
+	struct mts64* mts = substream->rmidi->private_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(mts->lock, flags);	
+	if(up) {
+		mts->mode[substream->number] |= MTS64_MODE_INPUT_TRIGGERED;
+	} else {
+		mts->mode[substream->number] &= ~MTS64_MODE_INPUT_TRIGGERED;
+	}
+	spin_unlock_irqrestore(mts->lock, flags);
+}
+
+static snd_rawmidi_ops_t snd_mts64_rawmidi_output_ops = {
+	.open    = snd_mts64_rawmidi_output_open,
+	.close   = snd_mts64_rawmidi_output_close,
+	.trigger = snd_mts64_rawmidi_output_trigger
+};
+
+static snd_rawmidi_ops_t snd_mts64_rawmidi_input_ops = {
+	.open    = snd_mts64_rawmidi_input_open,
+	.close   = snd_mts64_rawmidi_input_close,
+	.trigger = snd_mts64_rawmidi_input_trigger
+};
+
+/* Create and initialize the rawmidi component */
+static int __devinit snd_mts64_rawmidi_create(struct snd_card* card, 
+					      struct mts64* chip, 
+					      snd_rawmidi_t** rrmidi)
+{
+	snd_rawmidi_t* rmidi;
+	snd_rawmidi_substream_t* substream;
+	struct list_head* list;
+	int err;
+	
+	*rrmidi = NULL;
+
+	err = snd_rawmidi_new(card, CARD_NAME, 0, 
+			      MTS64_NUM_OUTPUT_PORTS, 
+			      MTS64_NUM_INPUT_PORTS, 
+			      &rmidi);
+	if(err < 0) return err;
+
+	rmidi->private_data = chip;
+	strcpy(rmidi->name, CARD_NAME);
+	rmidi->info_flags = SNDRV_RAWMIDI_INFO_OUTPUT |
+		            SNDRV_RAWMIDI_INFO_INPUT |
+                            SNDRV_RAWMIDI_INFO_DUPLEX;
+
+	/* register rawmidi ops */
+	snd_rawmidi_set_ops(rmidi, SNDRV_RAWMIDI_STREAM_OUTPUT, 
+			    &snd_mts64_rawmidi_output_ops);
+	snd_rawmidi_set_ops(rmidi, SNDRV_RAWMIDI_STREAM_INPUT, 
+			    &snd_mts64_rawmidi_input_ops);
+
+	/* name substreams */
+	/* output */
+	list_for_each(list, 
+		      &rmidi->streams[SNDRV_RAWMIDI_STREAM_OUTPUT].substreams) {
+		substream = list_entry(list, snd_rawmidi_substream_t, list);
+		sprintf(substream->name,
+			"Miditerminal %d", substream->number+1);
+	}
+	/* input */
+	list_for_each(list, 
+		      &rmidi->streams[SNDRV_RAWMIDI_STREAM_INPUT].substreams) {
+		substream = list_entry(list, snd_rawmidi_substream_t, list);
+		chip->midi_input_substream[substream->number] = substream;
+		switch(substream->number) {
+		case MTS64_SMPTE_SUBSTREAM:
+			strcpy(substream->name, "Miditerminal SMPTE");
+			break;
+		default:
+			sprintf(substream->name,
+				"Miditerminal %d", substream->number+1);
+		}
+	}
+
+	/* controls */
+	snd_mts64_ctl_create(card, chip);
+
+	*rrmidi = rmidi;
+	return 0;
+}
+
+/*********************************************************************
+ * parport stuff
+ *********************************************************************/
+static void mts64_interrupt(int irq, void* private, struct pt_regs* r)
+{
+	struct mts64* mts = ((struct snd_card*)private)->private_data;
+	unsigned long flags;
+	unsigned short ret;
+	unsigned char status, data;
+	snd_rawmidi_substream_t* substream;
+
+	spin_lock_irqsave(mts->lock, flags);
+	ret = mts64_read(mts->pardev->port);
+	data = ret & 0x00ff;
+	status = ret >> 8;
+
+	if(status & MTS64_STAT_PORT) {
+		mts->current_midi_input_port = mts64_map_midi_input(data);
+	} else {
+		substream = mts->midi_input_substream[mts->current_midi_input_port];
+		if(mts->mode[substream->number] & MTS64_MODE_INPUT_TRIGGERED)
+			snd_rawmidi_receive(substream, &data, 1);
+	}
+	spin_unlock_irqrestore(mts->lock, flags);
+}
+
+static int __devinit mts64_probe_port(struct parport* p)
+{
+	struct pardevice* pardev;
+	int res;
+	unsigned long flags;
+
+	pardev = parport_register_device(p, DRIVER_NAME,
+					 NULL, NULL, NULL,
+					 0, NULL);
+	if(!pardev)
+		goto __err;
+	
+	if(parport_claim(pardev))
+		goto __err1;
+
+	local_irq_save(flags);
+	res = mts64_probe(p);
+	local_irq_restore(flags);
+
+	parport_release(pardev);
+	parport_unregister_device(pardev);
+
+	return res;
+	
+__err1:
+	parport_unregister_device(pardev);
+__err:
+	return 0;
+}
+
+static void __devinit mts64_attach(struct parport* p)
+{
+	struct platform_device* device;
+
+	device = platform_device_alloc(PLATFORM_DRIVER, device_count);
+	if(!device) 
+		return;
+
+	/* Temporary assignment to forward the parport */
+	platform_set_drvdata(device, p);
+
+	if(platform_device_register(device) < 0) {
+		platform_device_put(device);
+		return;
+	}
+
+	/* Since we dont get the return value of probe
+	 * We need to check if device probing succeeded or not */
+	if(!platform_get_drvdata(device)) {
+		platform_device_unregister(device);
+		return;
+	}
+
+	/* register device in global table */
+	platform_devices[device_count] = device;
+	device_count++;
+}
+
+static void mts64_detach(struct parport* p)
+{
+	/* nothing to do here */
+}
+
+static struct parport_driver mts64_parport_driver = {
+	.name   = "mts64",
+	.attach = mts64_attach,
+	.detach = mts64_detach
+};
+
+/*********************************************************************
+ * platform stuff
+ *********************************************************************/
+static int __devinit snd_mts64_probe(struct platform_device* pdev)
+{
+	struct pardevice* pardev;
+	struct parport* p;
+	int dev = pdev->id;
+	struct snd_card* card = NULL;
+	struct mts64* mts = NULL;
+	snd_rawmidi_t* rmidi = NULL;
+	int err;
+	unsigned long flags;
+
+	p = platform_get_drvdata(pdev);
+	platform_set_drvdata(pdev, NULL);
+
+	if(dev >= SNDRV_CARDS) {
+		return -ENODEV;
+	}
+	if(!enable[dev]) {
+		return -ENOENT;
+	}
+
+	if(!mts64_probe_port(p)) {
+		return -ENODEV;
+	}
+
+	card = snd_card_new(index[dev], id[dev], THIS_MODULE, 0);
+	if(card == NULL) {
+		snd_printd("Cannot create card\n");
+		err = -ENOMEM;
+		goto __err;
+	}
+	strcpy(card->driver, DRIVER_NAME);
+	strcpy(card->shortname, "ESI " CARD_NAME);
+	sprintf(card->longname,  "%s at 0x%lx, irq %i", 
+		card->shortname, p->base, p->irq);
+	
+	pardev = parport_register_device(p,                 /* port */
+					 DRIVER_NAME,       /* name */
+					 NULL,              /* preempt */
+					 NULL,              /* wakeup */
+					 mts64_interrupt,   /* ISR */
+					 PARPORT_DEV_EXCL,  /* flags */
+					 (void*)card);      /* private */
+	if(pardev == NULL) {
+		snd_printd("Cannot register pardevice\n");
+		err = -EIO;
+		goto __err_free_card;
+	}
+
+	if((err = snd_mts64_create(card, pardev, &mts)) < 0) {
+		snd_printd("Cannot create main component\n");
+		goto __err_free_device;
+	}
+	card->private_data = mts;
+	
+	if((err = snd_mts64_rawmidi_create(card, mts, &rmidi)) < 0) {
+		snd_printd("Creating Rawmidi component failed\n");
+		goto __err_free_device;
+	}
+
+	mts->rmidi = rmidi;
+
+	/* claim parport */
+	if(parport_claim(pardev)) {
+		snd_printk("Cannot claim parport 0x%lx\n", pardev->port->base);
+		err = -EIO;
+		goto __err_free_device;
+	}
+	mts->pardev_claimed = 1;
+
+	/* init device */
+	local_irq_save(flags);
+	mts64_device_init(p);
+	local_irq_restore(flags);
+
+	platform_set_drvdata(pdev, card);
+
+	/* At this point card will be usable */
+	if((err = snd_card_register(card)) < 0) {
+		snd_printk("Cannot register card\n");
+		goto __err_release;
+	}
+
+	snd_printk("ESI Miditerminal 4140 on 0x%lx\n", p->base);
+	return 0;
+
+__err_release:
+	parport_release(pardev);
+	mts->pardev_claimed = 0;
+__err_free_device:
+	parport_unregister_device(pardev);
+__err_free_card:
+	snd_card_free(card);
+__err:
+	return err;
+
+}
+
+static int snd_mts64_remove(struct platform_device* pdev)
+{
+	struct snd_card* card = platform_get_drvdata(pdev);
+	struct mts64* mts = card->private_data;
+	struct pardevice* pardev = mts->pardev;
+
+	if(card) {
+		snd_card_free(card);
+		snd_mts64_free(mts);
+	}
+	if(pardev) {
+		if(mts->pardev_claimed)
+			parport_release(pardev);
+		parport_unregister_device(pardev);
+	}
+
+	return 0;
+}
+
+
+static struct platform_driver snd_mts64_driver = {
+	.probe  = snd_mts64_probe,
+	.remove = snd_mts64_remove,
+	.driver = {
+		.name = PLATFORM_DRIVER
+	}
+};
+
+/*********************************************************************
+ * module init stuff
+ *********************************************************************/
+static void mts64_unregister_all(void)
+{
+	int i;
+
+	for(i=0; i<SNDRV_CARDS; ++i) {
+		if(platform_devices[i]) {
+			platform_device_unregister(platform_devices[i]);
+			platform_devices[i] = NULL;
+		}
+	}		
+	platform_driver_unregister(&snd_mts64_driver);
+	parport_unregister_driver(&mts64_parport_driver);
+}
+
+static int __init mts64_module_init(void)
+{
+	int err;
+
+	if((err = platform_driver_register(&snd_mts64_driver)) < 0)
+		return err;
+
+	if(parport_register_driver(&mts64_parport_driver) != 0) {
+		platform_driver_unregister(&snd_mts64_driver);
+		return -EIO;
+	}
+
+	if(device_count == 0) {
+		mts64_unregister_all();
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void __exit mts64_module_exit(void)
+{
+	mts64_unregister_all();
+}
+
+module_init(mts64_module_init);
+module_exit(mts64_module_exit);

Re: [PATCH] Support for ESI Miditerminal 4140

[Takashi Iwai]

At Mon, 24 Apr 2006 19:23:22 +0200,
Matthias Koenig wrote:
> 
> diff -r 4e7af6120ca7 drivers/Kconfig
> --- a/drivers/Kconfig	Fri Apr 21 12:40:49 2006 +0200
> +++ b/drivers/Kconfig	Mon Apr 24 19:17:13 2006 +0200
> @@ -43,3 +43,10 @@ config SND_PCSP
>  	  You can compile this as a module, which will be called snd-pcsp.
>  
>  	  You don't need this driver if you only want your computer to beep.
> +
> +config SND_MTS64
> +	tristate "ESI Miditerminal 4140 driver"
> +	depends on SND && PARPORT
> +	select SND_RAWMIDI
> +	help
> +	  Say 'Y' or 'M' to include support for Miditerminal 4140.

A bit more explanative, please.


> diff -r 4e7af6120ca7 drivers/mts64.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/drivers/mts64.c	Mon Apr 24 19:17:13 2006 +0200
...
> +static int index[SNDRV_CARDS]  = SNDRV_DEFAULT_IDX;
> +static char* id[SNDRV_CARDS]   = SNDRV_DEFAULT_STR;

Please use a normal C-style, char *id[].

> +struct mts64 {
> +	spinlock_t lock;
> +	struct snd_card* card;
> +	snd_rawmidi_t* rmidi;
> +	struct pardevice* pardev;
> +	int pardev_claimed;
> +
> +	int open_count;
> +	int current_midi_output_port;
> +	int current_midi_input_port;
> +	unsigned char mode[MTS64_NUM_INPUT_PORTS];
> +	snd_rawmidi_substream_t* midi_input_substream[MTS64_NUM_INPUT_PORTS];

We removed xxx_t typedefs.  Use struct instead.

> +static int __devinit snd_mts64_create(struct snd_card* card, 
> +				      struct pardevice* pardev, 
> +				      struct mts64** rchip)
> +{
> +	struct mts64* mts;
> +	int i;
> +
> +	*rchip = NULL;
> +
> +	mts = kzalloc(sizeof(struct mts64), GFP_KERNEL);
> +	if(mts == NULL) 
> +		return -ENOMEM;
> +
> +	/* Init chip specific data */
> +	spin_lock_init(&mts->lock);
> +	mts->card = card;
> +	mts->rmidi = NULL;      /* initialized later in mts64_attach */
> +	mts->pardev = pardev;
> +	mts->pardev_claimed = 0;
> +	mts->open_count = 0;
> +	mts->current_midi_output_port = -1;
> +	mts->current_midi_input_port = -1;
> +	for(i=0; i<MTS64_NUM_INPUT_PORTS; ++i)
> +		mts->mode[i] = 0;
> +	/* SMPTE */
> +	mts->smpte_switch = 0;
> +	mts->time[0] = 0;
> +	mts->time[1] = 0;
> +	mts->time[2] = 0;
> +	mts->time[3] = 0;
> +	mts->fps = 0;

You can omit zero-initializations above.

> +/*  waits for device ready
> + *
> + *  Checks if BUSY (Bit 7 of status) is clear
> + *  1 device ready
> + *  0 failure
> + */
> +static int mts64_device_ready(struct parport* p)
> +{
> +	int i;
> +	unsigned char c;
> +
> +	for(i=0; i < 0xffff; ++i) {

Space after 'for' and spaces between '=' are preferred.
Keep the coding style found in linux/Documentation/CodingStyle.

> +		c = parport_read_status(p);
> +		c &= MTS64_STAT_BSY;
> +		if(c != 0) return 1;

Break a line (and add a space after 'if').

> +/*  Init device (LED blinking startup magic)
> + *
> + *  Returns:
> + *  1 init ok
> + *  0 failure
> + */
> +static int mts64_device_init(struct parport* p)
> +{
> +	int i;
> +
> +	mts64_write_command(p, MTS64_CMD_RESET);
> +
> +	for(i=0; i < 64; ++i) {
> +		mdelay(100);

Such a long delay in local_irq_disable()?


> +/* 
> + *  Opens the device (set communication mode) and increases the 
> + *  reference count.
> + */
> +static int mts64_device_open(struct mts64* mts)
> +{
> +	int i;
> +	struct parport* p = mts->pardev->port;
> +
> +	if(mts->open_count == 0) {
> +		for(i=0; i<5; ++i)
> +			mts64_write_command(p, MTS64_CMD_COM_OPEN);
> +	}
> +
> +	++(mts->open_count);
> +	
> +	/* FIXME: Busy wait really necessary? */
> +	mdelay(50);

Ditto.  A too long delay in irq_disable context.

> +
> +	return 0;
> +}
> +
> +/*  
> + *  Close device and decrement reference count.
> + */
> +static int mts64_device_close(struct mts64* mts)
> +{
> +	int i;
> +	struct parport* p = mts->pardev->port;
> +
> +	--(mts->open_count);
> +	if(mts->open_count == 0) {
> +		for(i=0; i<5; ++i) {
> +			mts64_write_command(p, MTS64_CMD_COM_CLOSE1);
> +			mts64_write_command(p, MTS64_CMD_COM_CLOSE2);
> +		}
> +	} else if(mts->open_count < 0) {
> +		mts->open_count = 0;
> +	}

Should omit braces for a single line after 'if'.

> +
> +	/* FIXME: Busy wait really necessary? */
> +	mdelay(500);

A too long delay in irq_disable.

> +
> +	return 0;
> +}
> +
> +/*  map hardware port to substream number
> + * 
> + *  When reading a byte from the device, the device tells us
> + *  on what port the byte is. This HW port has to be mapped to
> + *  the midiport (substream number).
> + *  substream 0-3 are Midiports 1-4
> + *  substream 4 is SMPTE Timecode
> + *  The mapping is done by the table:
> + *  HW | 0 | 1 | 2 | 3 | 4 
> + *  SW | 0 | 1 | 4 | 2 | 3
> + */
> +static unsigned char mts64_map_midi_input(unsigned char c)
> +{
> +	unsigned char map[] = { 0, 1, 4, 2, 3 };

Should be static char array.

> +
> +	return map[c];
> +}
> +
> +
> +/*  Probe parport for device
> + *
> + *  Do we have a Miditerminal 4140 on parport? 
> + *  Returns:
> + *  1 device found
> + *  0 no device
> + */
> +static int mts64_probe(struct parport* p)
> +{
> +	unsigned char c;
> +
> +	mts64_smpte_stop(p);
> +
> +	mts64_write_command(p, MTS64_CMD_PROBE);
> +
> +	/* FIXME */
> +	mdelay(50);

Possible to replace with msleep()?

> +	
> +	c = mts64_read(p);
> +
> +	c &= 0x00ff;
> +	if(c != MTS64_CMD_PROBE) return 0;
> +	else return 1;

Break lines.

> +static void mts64_smpte_start(struct parport* p,
> +			      unsigned char hours, unsigned char minutes,
> +			      unsigned char seconds, unsigned char frames,
> +			      unsigned char idx)
> +{
> +	unsigned char fps[5] = { MTS64_CMD_SMPTE_FPS_24, 
> +				 MTS64_CMD_SMPTE_FPS_25,
> +				 MTS64_CMD_SMPTE_FPS_2997, 
> +				 MTS64_CMD_SMPTE_FPS_30D,
> +				 MTS64_CMD_SMPTE_FPS_30    };

Use a static array.

> +/*  Write a MIDI byte to midiport
> + *
> + *  midiport ranges from 0-3 and maps to Ports 1-4
> + *  assumptions: communication mode is on
> + *  
> + */
> +static void mts64_write_midi(struct mts64* mts, unsigned char c,
> +			     int midiport)
> +{
> +	struct parport* p = mts->pardev->port;
> +
> +	/* check current midiport */
> +	if(mts->current_midi_output_port != midiport) {
> +		mts64_write_command(p, midiport);
> +	}

Remove braces around a single if line.

> +
> +	/* write midi byte */
> +	mts64_write_data(p, c);
> +}
> +
> +/*********************************************************************
> + * Control elements
> + *********************************************************************/
> +
> +/* SMPTE Switch */
> +static int snd_mts64_ctl_smpte_switch_info(snd_kcontrol_t *kctl,
> +					   snd_ctl_elem_info_t* uinfo)

Use struct snd_kcontrol and struct snd_ctl_elem_info.
(For checking the obsolete typedefs, comment out the inclusion of
"typedefs.h" in adriver.h.)

> +
> +static snd_kcontrol_new_t mts64_ctl_smpte_switch __devinitdata = {

Use struct snd_kcontrol_new.

> +	.iface = SNDRV_CTL_ELEM_IFACE_RAWMIDI,
> +	.name  = "SMPTE Playback Switch",
> +	.index = 0,
> +	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
> +	.private_value = 0,
> +	.info = snd_mts64_ctl_smpte_switch_info,
> +	.get  = snd_mts64_ctl_smpte_switch_get,
> +	.put  = snd_mts64_ctl_smpte_switch_put
> +};
> +/* FPS */
> +static int snd_mts64_ctl_smpte_fps_info(snd_kcontrol_t* kctl,
> +					snd_ctl_elem_info_t* uinfo)
> +{
> +	char* texts[5] = { "24",
> +			   "25",
> +			   "29.97",
> +			   "30D",
> +			   "30"    };

Use a static array.

> +static void mts64_interrupt(int irq, void* private, struct pt_regs* r)
> +{
> +	struct mts64* mts = ((struct snd_card*)private)->private_data;
> +	unsigned long flags;
> +	unsigned short ret;
> +	unsigned char status, data;
> +	snd_rawmidi_substream_t* substream;
> +
> +	spin_lock_irqsave(mts->lock, flags);

You need to irqsave/restore in the interrupt handler.


thanks,

Takashi


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] Support for ESI Miditerminal 4140

[Matthias Koenig]

Hi Takashi,

Thank you for review!

Takashi Iwai <tiwai@suse.de> writes:
> At Mon, 24 Apr 2006 19:23:22 +0200,
> Matthias Koenig wrote:
[...]
>> +config SND_MTS64
>> +	tristate "ESI Miditerminal 4140 driver"
>> +	depends on SND && PARPORT
>> +	select SND_RAWMIDI
>> +	help
>> +	  Say 'Y' or 'M' to include support for Miditerminal 4140.
>
> A bit more explanative, please.

Ok

>> +static char* id[SNDRV_CARDS]   = SNDRV_DEFAULT_STR;
>
> Please use a normal C-style, char *id[].

Ok

>> +	snd_rawmidi_substream_t* midi_input_substream[MTS64_NUM_INPUT_PORTS];
>
> We removed xxx_t typedefs.  Use struct instead.

Ok. Sorry, this has been already on my plan, I don't know why forgot 
about it.

[...]
>> +	mts->smpte_switch = 0;
>> +	mts->time[0] = 0;
>> +	mts->time[1] = 0;
>> +	mts->time[2] = 0;
>> +	mts->time[3] = 0;
>> +	mts->fps = 0;
>
> You can omit zero-initializations above.

Ok

[...]
> Space after 'for' and spaces between '=' are preferred.
> Keep the coding style found in linux/Documentation/CodingStyle.

Ok

>> +static int mts64_device_init(struct parport* p)
>> +{
>> +	int i;
>> +
>> +	mts64_write_command(p, MTS64_CMD_RESET);
>> +
>> +	for(i=0; i < 64; ++i) {
>> +		mdelay(100);
>
> Such a long delay in local_irq_disable()?
>
>> +	/* FIXME: Busy wait really necessary? */
>> +	mdelay(50);
>
> Ditto.  A too long delay in irq_disable context.

Yes, these long delays have caused me some bellyache.
I got the information about the device from reverse engineering the Windows
driver and the delay times are exactly taken from there (actually there is a
function which repeatedly calls KeStallExecutionProcessor with an argument 
of 50us until the given time is elapsed).
I will try to find a better solution. Maybe some additional state variable
which will be triggered by a timer. Also most of the delays are happening
in process context, so it might be possible to sleep. At least it will be
possible for the open/close callbacks.
I'll rework those sections.

[...]
>> +	} else if(mts->open_count < 0) {
>> +		mts->open_count = 0;
>> +	}
>
> Should omit braces for a single line after 'if'.

Ok

>> +
>> +	/* FIXME: Busy wait really necessary? */
>> +	mdelay(500);
>
> A too long delay in irq_disable.

s.a.

[...]
>> +static unsigned char mts64_map_midi_input(unsigned char c)
>> +{
>> +	unsigned char map[] = { 0, 1, 4, 2, 3 };
>
> Should be static char array.

Ok

[...]
>> +static int mts64_probe(struct parport* p)
>> +{
>> +	unsigned char c;
>> +
>> +	mts64_smpte_stop(p);
>> +
>> +	mts64_write_command(p, MTS64_CMD_PROBE);
>> +
>> +	/* FIXME */
>> +	mdelay(50);
>
> Possible to replace with msleep()?

This might be an option. 

[...]
>> +	if(c != MTS64_CMD_PROBE) return 0;
>> +	else return 1;
>
> Break lines.

Ok

>> +static void mts64_smpte_start(struct parport* p,
>> +			      unsigned char hours, unsigned char minutes,
>> +			      unsigned char seconds, unsigned char frames,
>> +			      unsigned char idx)
>> +{
>> +	unsigned char fps[5] = { MTS64_CMD_SMPTE_FPS_24, 
>> +				 MTS64_CMD_SMPTE_FPS_25,
>> +				 MTS64_CMD_SMPTE_FPS_2997, 
>> +				 MTS64_CMD_SMPTE_FPS_30D,
>> +				 MTS64_CMD_SMPTE_FPS_30    };
>
> Use a static array.

Ok

[...]
>> +	/* check current midiport */
>> +	if(mts->current_midi_output_port != midiport) {
>> +		mts64_write_command(p, midiport);
>> +	}
>
> Remove braces around a single if line.

OK


[...]
>> +/* SMPTE Switch */
>> +static int snd_mts64_ctl_smpte_switch_info(snd_kcontrol_t *kctl,
>> +					   snd_ctl_elem_info_t* uinfo)
>
> Use struct snd_kcontrol and struct snd_ctl_elem_info.
> (For checking the obsolete typedefs, comment out the inclusion of
> "typedefs.h" in adriver.h.)

Ok, removed all typedefs
>
>> +
>> +static snd_kcontrol_new_t mts64_ctl_smpte_switch __devinitdata = {
>
> Use struct snd_kcontrol_new.

Ok
>
>> +	.iface = SNDRV_CTL_ELEM_IFACE_RAWMIDI,
>> +	.name  = "SMPTE Playback Switch",
>> +	.index = 0,
>> +	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
>> +	.private_value = 0,
>> +	.info = snd_mts64_ctl_smpte_switch_info,
>> +	.get  = snd_mts64_ctl_smpte_switch_get,
>> +	.put  = snd_mts64_ctl_smpte_switch_put
>> +};
>> +/* FPS */
>> +static int snd_mts64_ctl_smpte_fps_info(snd_kcontrol_t* kctl,
>> +					snd_ctl_elem_info_t* uinfo)
>> +{
>> +	char* texts[5] = { "24",
>> +			   "25",
>> +			   "29.97",
>> +			   "30D",
>> +			   "30"    };
>
> Use a static array.

OK
>
>> +static void mts64_interrupt(int irq, void* private, struct pt_regs* r)
>> +{
>> +	struct mts64* mts = ((struct snd_card*)private)->private_data;
>> +	unsigned long flags;
>> +	unsigned short ret;
>> +	unsigned char status, data;
>> +	snd_rawmidi_substream_t* substream;
>> +
>> +	spin_lock_irqsave(mts->lock, flags);
>
> You need to irqsave/restore in the interrupt handler.

I'm not sure if I understand your point here.
This is what I already do. Do you mean "you _don't_ need to 
irqsave/restore ..."?

thanks,
Matthias


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] Support for ESI Miditerminal 4140

[Takashi Iwai]

At Tue, 25 Apr 2006 11:49:11 +0200,
Matthias Koenig wrote:
> 
> >> +static int mts64_device_init(struct parport* p)
> >> +{
> >> +	int i;
> >> +
> >> +	mts64_write_command(p, MTS64_CMD_RESET);
> >> +
> >> +	for(i=0; i < 64; ++i) {
> >> +		mdelay(100);
> >
> > Such a long delay in local_irq_disable()?
> >
> >> +	/* FIXME: Busy wait really necessary? */
> >> +	mdelay(50);
> >
> > Ditto.  A too long delay in irq_disable context.
> 
> Yes, these long delays have caused me some bellyache.
> I got the information about the device from reverse engineering the Windows
> driver and the delay times are exactly taken from there (actually there is a
> function which repeatedly calls KeStallExecutionProcessor with an argument 
> of 50us until the given time is elapsed).

Hm, but you call mdelay(50), not udelay(50).
I won't care about the latter case but mdelay(50) is definitely to be
avoided.

> I will try to find a better solution. Maybe some additional state variable
> which will be triggered by a timer. Also most of the delays are happening
> in process context, so it might be possible to sleep. At least it will be
> possible for the open/close callbacks.
> I'll rework those sections.

If it's really mdelay(50), change spin_lock to mutex_lock and replace
mdelay to msleep() -- at least, for open/close calls, where you can
sleep safely.


> >> +static void mts64_interrupt(int irq, void* private, struct pt_regs* r)
> >> +{
> >> +	struct mts64* mts = ((struct snd_card*)private)->private_data;
> >> +	unsigned long flags;
> >> +	unsigned short ret;
> >> +	unsigned char status, data;
> >> +	snd_rawmidi_substream_t* substream;
> >> +
> >> +	spin_lock_irqsave(mts->lock, flags);
> >
> > You need to irqsave/restore in the interrupt handler.
> 
> I'm not sure if I understand your point here.
> This is what I already do. Do you mean "you _don't_ need to 
> irqsave/restore ..."?

Yep, exactly.  Sorry for a typo :)


thanks,

Takashi


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] Support for ESI Miditerminal 4140

[Matthias Koenig]

Takashi Iwai <tiwai@suse.de> writes:
> At Tue, 25 Apr 2006 11:49:11 +0200,
> Matthias Koenig wrote:
>> Yes, these long delays have caused me some bellyache.
>> I got the information about the device from reverse engineering the Windows
>> driver and the delay times are exactly taken from there (actually there is a
>> function which repeatedly calls KeStallExecutionProcessor with an argument 
>> of 50us until the given time is elapsed).
>
> Hm, but you call mdelay(50), not udelay(50).
> I won't care about the latter case but mdelay(50) is definitely to be
> avoided.

Yes, it's 50ms of _total_ time to wait.
The Code does something like this (roughly translated to C):

void wait_some_time(long long interval) /* interval is in ms */ 
{
        long long time = PcGetTimeInterval(0); /* Current time in 100ns ticks */
        interval *= 10000;  /* translate to 100ns ticks */
        do {
                KeStallExecutionProcessor(50);   /* This is us */
        } while (interval < PcGetTimeInterval(time));
}

> If it's really mdelay(50), change spin_lock to mutex_lock and replace
> mdelay to msleep() -- at least, for open/close calls, where you can
> sleep safely.

I totally overlooked the following sentence in "Writing an ALSA Driver":
"The open and close callbacks of a rawmidi device are serialized with a
mutex, and can sleep."

So, if I understand this correctly, there is already a mutex held by the
ALSA core and it is guaranteed that there are no multiple accesses of
open/close callbacks?
Hmm, then why should I hold an additional mutex?

>> > You need to irqsave/restore in the interrupt handler.
>> 
>> I'm not sure if I understand your point here.
>> This is what I already do. Do you mean "you _don't_ need to 
>> irqsave/restore ..."?
>
> Yep, exactly.  Sorry for a typo :)

Ok.

Thanks,
Matthias


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] Support for ESI Miditerminal 4140

[Takashi Iwai]

At Tue, 25 Apr 2006 14:17:03 +0200,
Matthias Koenig wrote:
> 
> Takashi Iwai <tiwai@suse.de> writes:
> > At Tue, 25 Apr 2006 11:49:11 +0200,
> > Matthias Koenig wrote:
> >> Yes, these long delays have caused me some bellyache.
> >> I got the information about the device from reverse engineering the Windows
> >> driver and the delay times are exactly taken from there (actually there is a
> >> function which repeatedly calls KeStallExecutionProcessor with an argument 
> >> of 50us until the given time is elapsed).
> >
> > Hm, but you call mdelay(50), not udelay(50).
> > I won't care about the latter case but mdelay(50) is definitely to be
> > avoided.
> 
> Yes, it's 50ms of _total_ time to wait.
> The Code does something like this (roughly translated to C):
> 
> void wait_some_time(long long interval) /* interval is in ms */ 
> {
>         long long time = PcGetTimeInterval(0); /* Current time in 100ns ticks */
>         interval *= 10000;  /* translate to 100ns ticks */
>         do {
>                 KeStallExecutionProcessor(50);   /* This is us */
>         } while (interval < PcGetTimeInterval(time));
> }
> 
> > If it's really mdelay(50), change spin_lock to mutex_lock and replace
> > mdelay to msleep() -- at least, for open/close calls, where you can
> > sleep safely.
> 
> I totally overlooked the following sentence in "Writing an ALSA Driver":
> "The open and close callbacks of a rawmidi device are serialized with a
> mutex, and can sleep."
> 
> So, if I understand this correctly, there is already a mutex held by the
> ALSA core and it is guaranteed that there are no multiple accesses of
> open/close callbacks?

The mutex belongs to the rawmidi device, so open/close is mutual as
long as the same rawmidi device is used no matter which substream is
used.  It doesn't care open/close of another rawmidi device, though.

> Hmm, then why should I hold an additional mutex?

Well, I just thought of something as a replacement of mts->lock, but
regarding open/close, yes, you can get rid of an extra mutex.

You might still need a spinlock somewhere, but definitely not together
with mdelay().


Takashi


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] Support for ESI Miditerminal 4140

[Matthias Koenig]

Takashi Iwai <tiwai@suse.de> writes:
> The mutex belongs to the rawmidi device, so open/close is mutual as
> long as the same rawmidi device is used no matter which substream is
> used.  It doesn't care open/close of another rawmidi device, though.

Ah, thanks! This is the informationen I needed.

>> Hmm, then why should I hold an additional mutex?
>
> Well, I just thought of something as a replacement of mts->lock, but
> regarding open/close, yes, you can get rid of an extra mutex.
>
> You might still need a spinlock somewhere, but definitely not together
> with mdelay().

Yes, I decoupled now delay stuff from the spinlock sections and placed
the spinlocks just near the HW access.

New patch for next turn will follow.

Matthias


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642