Re: [PATCH -v2 1/1] Misc: add sensable phantom driver

[Jiri Slaby <jirislaby@gmail.com>]

Andrew Morton napsal(a):
> On Wed,  2 May 2007 17:56:01 +0200 (CEST) Jiri Slaby <jirislaby@gmail.com> wrote:
> 
>> add sensable phantom driver
> 
> <googles>
> 
> Is that one of these? 
> http://www.est-kl.com/aufbau_general/index_hard_haptic.html?http://www.est-kl.com/hardware/haptic/sensable/phantom_omni.html

Omni is a small ieee1394 device. This is much bigger phantom premium pci
version:
http://www.sensable.com/haptic-phantom-premium-6dof.htm

> I don't think we've ever had a driver for a device like that before.  I
> wonder how you designed the userspace interface to it.

I used the userspace libs that worked on 2.4 linux and rewrote them a little
bit. However this is the old way, obsoleted I think, but it's still used in
our human-computer interaction lab.

The other one is through supported OpenHaptics suite, which was added in
this version -- that is the original Sensable (the device manufacturer)
product, but they provide 2.4 linux drivers only with it.

>> +static int phantom_ioctl(struct inode *inode, struct file *file, u_int cmd,
>> +	u_long arg)
> 
> Could we please use standard-looking types in this driver?  u32 and u64 and
> things?

Huh, wonders how this got there, thanks!

> However in this case, as it's an ioctl handler it should be "int" and
> "unsigned long".
> 
> Does this driver require compat handling?  (Seems not?)


>> +static int phantom_release(struct inode *inode, struct file *file)
>> +{
>> +	struct phantom_device *dev = file->private_data;
>> +
>> +	if (mutex_lock_interruptible(&dev->open_lock))
>> +		return -ERESTARTSYS;
>> +
>> +	dev->opened = 0;
>> +	phantom_status(dev, dev->status & ~PHB_RUNNING);
>> +
>> +	mutex_unlock(&dev->open_lock);
>> +
>> +	return 0;
>> +}
> 
> The mutex_lock_interruptible() is wrong, I think.  This function is called
> when we do the final close().  If the signal _is_ taken then it's game
> over: the device can no longer be opened.

Yes, starving at sys_close, I think so.

<fast searching in drivers/*>

Wouldn't this be a problem here too:
drivers/media/dvb/cinergyT2/cinergyT2.c
drivers/usb/misc/auerswald.c
at least that the device won't be stopped or something like that?

>> +/*
>> + * Init and deinit driver
>> + */
>> +
>> +static unsigned int __devinit phantom_get_free(void)
>> +{
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < PHANTOM_MAX_MINORS; i++)
>> +		if (phantom_devices[i] == 0)
>> +			break;
>> +
>> +	return i;
>> +}
> 
> This function assumes that the phantom_devices[] array will never have any
> holes in it.  But if phantom_remove() is called out-of-order, it _will_
> have holes.  Perhaps - I didn't look too hard.

I'm afraid I didn't get this. It goes through all phantom_devices and
returns first free index. If a hole is present it will return "i" which
corresponds to the hole, because phantom_devices[i] is 0. Note that it is
unsigned char array.


thanks for the clues,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
 B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E