Re: [PATCH] Support for ESI Miditerminal 4140

[Matthias Koenig <list@phasorlab.de>]

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