Message-Id: <20070419230719.a118a132.akpm@linux-foundation.org>
Date:	Thu, 19 Apr 2007 23:07:19 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Jiri Slaby <jirislaby@...il.com>
Cc:	<linux-kernel@...r.kernel.org>,
	johann deneux <johann.deneux@...il.com>,
	Dmitry Torokhov <dtor@...ightbb.com>, <stenyak@...il.com>,
	<linux-input@...ey.karlin.mff.cuni.cz>
Subject: Re: [RFC 2/2] Input: phantom, add a new driver

On Tue, 17 Apr 2007 22:02:10 +0200 (CEST) Jiri Slaby <jirislaby@...il.com> wrote:

> phantom, add a new driver
> 
> Sensable Phantom is a up to 7DOF force feedback (up to 6DOF FF) device. It's
> atypical, so it's based on the new added FF_RAW effect.
> 
> diff --git a/drivers/input/misc/phantom.c b/drivers/input/misc/phantom.c
> new file mode 100644
> index 0000000..58f55cd
> --- /dev/null
> +++ b/drivers/input/misc/phantom.c
> @@ -0,0 +1,387 @@
> +/*
> + *  Copyright (C) 2005-2007 Jiri Slaby <jirislaby@...il.com>
> + *
> + *  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.
> + *
> + *  You need an userspace library to cooperate with this driver. It (and other
> + *  info) may be obtained here:
> + *  http://www.fi.muni.cz/~xslaby/phantom.html
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/pci.h>
> +#include <linux/fs.h>
> +#include <linux/poll.h>
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +
> +#include <asm/io.h>
> +
> +#define PHANTOM_VERSION		"n0.9.4"

That's an impressive version number ;)

> +#define PHM_MAX_TORQUES		3
> +
> +#define PHN_CONTROL		0x6
> +#define PHN_CTL_AMP		0x1
> +#define PHN_CTL_BUT		0x2
> +#define PHN_CTL_IRQ		0x10
> +
> +#define PHN_IRQCTL		0x4c
> +
> +#define PHN_ZERO_FORCE		2048

<wonders what all those do>

> +#define PCI_ENCODER(dev, axis) ((0 - (int)ioread32((dev)->iaddr + (axis))) & \
> +									0xffff)

Is there any reason why this cannot be a lower-cased inline C function? 
Nicer to read, typesafe, etc.

> +#define PHB_RUNNING		1
> +#define PHB_RESET		2
> +
> +static struct PH_CLASSTYPE *phantom_class;

I guess that PH_CLASSTYPE is some protect-me-from-gregkh compatibility
thing.  But there isn't such a macro in the tree.  I switched this to plain
old `class'.

> +struct phantom_device {
> +	void __iomem *caddr;
> +	u32 __iomem *iaddr;
> +	u32 __iomem *oaddr;
> +	u32 amp_bit;
> +	s16 torques[PHM_MAX_TORQUES];
> +	unsigned long status;
> +
> +	struct input_dev *idev;
> +};
> +
> +static int phantom_status(struct phantom_device *dev, unsigned long newstat)
> +{
> +	pr_debug("phantom_status %lx %lx\n", dev->status, newstat);
> +
> +	if (!(dev->status & PHB_RUNNING) && (newstat & PHB_RUNNING)) {
> +		iowrite32(PHN_CTL_IRQ | PHN_CTL_AMP, dev->iaddr + PHN_CONTROL);
> +		dev->amp_bit = PHN_CTL_IRQ;
> +		iowrite32(0x43, dev->caddr + PHN_IRQCTL);
> +	} else if ((dev->status & PHB_RUNNING) && !(newstat & PHB_RUNNING))
> +		iowrite32(0, dev->caddr + PHN_IRQCTL);
> +
> +	dev->status = newstat;
> +
> +	return 0;
> +}
> +
> +static void phantom_close(struct input_dev *idev)
> +{
> +	struct phantom_device *dev = idev->private;
> +
> +	phantom_status(dev, dev->status & ~PHB_RUNNING);
> +}
> +
> +static void phantom_reset(struct phantom_device *dev)
> +{
> +	pr_debug("resetting\n");
> +
> +	iowrite32(~0x1f, dev->iaddr);
> +	wmb();
> +	iowrite32(0x1f, dev->iaddr);
> +	dev->status |= PHB_RESET;
> +}

Does the wmb here actually do anything useful?  If so, a comment is needed
because it isn't possible to work out why that statement is there by
reading the code.

> ...
>
> +
> +static irqreturn_t phantom_isr(int irq, void *data)
> +{
> +	struct phantom_device *dev = data;
> +	struct input_dev *idev = dev->idev;
> +	unsigned int a, hw_status;
> +
> +	hw_status = ioread32(dev->iaddr + PHN_CONTROL);
> +	if (!(hw_status & PHN_CTL_IRQ))
> +		return IRQ_NONE;
> +
> +	iowrite32(0, dev->iaddr);
> +	wmb();
> +	iowrite32(0xc0, dev->iaddr);

there too.

> +	if (unlikely(idev == NULL))
> +		return IRQ_HANDLED;

Can this happen?  If so, a comment explaining why would be nice.

> +	input_report_abs(idev, ABS_X, PCI_ENCODER(dev, 0));
> +	input_report_abs(idev, ABS_Y, PCI_ENCODER(dev, 1));
> +	input_report_abs(idev, ABS_Z, PCI_ENCODER(dev, 2));
> +	input_report_abs(idev, ABS_RX, PCI_ENCODER(dev, 3));
> +	input_report_abs(idev, ABS_RY, PCI_ENCODER(dev, 4));
> +	input_report_abs(idev, ABS_RZ, PCI_ENCODER(dev, 5));
> +	input_report_key(idev, BTN_0, !!(hw_status & PHN_CTL_BUT));
> +	input_sync(idev);
> +	input_event(idev, EV_SYN, SYN_CONFIG, 0);
> +
> +	for (a = 0; a < PHM_MAX_TORQUES; a++)
> +		iowrite32(dev->torques[a] + PHN_ZERO_FORCE, dev->oaddr + a);
> +	iowrite32(dev->amp_bit, dev->iaddr + PHN_CONTROL);
> +	dev->amp_bit ^= PHN_CTL_AMP;
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/******************************************************************************

A plain old

/*
 * Init and deinit driver
 */

is sufficient and typical.

But this is not actually the most useful of comments anyway ;)

> + * Init and deinit driver
> + */
> +static void __devinit phantom_init_idev(const struct pci_dev *pdev,
> +		struct phantom_device *dev)
> +{
> +	struct input_dev *idev = dev->idev;
> +
> +	idev->private = dev;
> +	idev->name = "Sensable Phantom";
> +	idev->id.bustype = BUS_PCI;

<looks>

<wonders why BUS_PCI got defined in input.h>

<and in drivers/isdn/hardware/eicon/cardtype.h>

<and in drivers/char/sxboards.h>

<hurriedly stops looking>

> +	idev->id.vendor = pdev->vendor;
> +	idev->id.product = pdev->device;
> +	idev->id.version = 1;
> +	idev->close = phantom_close;
> +
> +	set_bit(BTN_0, idev->keybit);
> +	input_set_abs_params(idev, ABS_X, -0x8000, 0x7fff, 4, 0);
> +	input_set_abs_params(idev, ABS_Y, -0x8000, 0x7fff, 4, 0);
> +	input_set_abs_params(idev, ABS_Z, -0x8000, 0x7fff, 4, 0);
> +	input_set_abs_params(idev, ABS_RX, -0x8000, 0x7fff, 4, 0);
> +	input_set_abs_params(idev, ABS_RY, -0x8000, 0x7fff, 4, 0);
> +	input_set_abs_params(idev, ABS_RZ, -0x8000, 0x7fff, 4, 0);
> +	set_bit(FF_RAW, idev->ffbit);
> +
> +	set_bit(EV_KEY, idev->evbit);
> +	set_bit(EV_ABS, idev->evbit);
> +	set_bit(EV_FF, idev->evbit);
> +	set_bit(FF_AUTOCENTER, idev->ffbit); /* reset phantom */
> +}
> +
> +static int __devinit phantom_probe(struct pci_dev *pdev,
> +	const struct pci_device_id *pci_id)
> +{
> +	struct phantom_device *pht;
> +	int retval;
> +
> +	retval = pci_enable_device(pdev);
> +	if (retval)
> +		goto err;
> +
> +	retval = pci_request_regions(pdev, "phantom");
> +	if (retval)
> +		goto err;

Missed a pci_disable_device() here?

> +	retval = -ENOMEM;
> +	pht = kzalloc(sizeof(*pht), GFP_KERNEL);
> +	if (pht == NULL) {
> +		dev_err(&pdev->dev, "unable to allocate device\n");
> +		goto err_reg;
> +	}
> +
> +	pht->caddr = pci_iomap(pdev, 0, 0);
> +	if (pht->caddr == NULL) {
> +		dev_err(&pdev->dev, "can't remap conf space\n");
> +		goto err_fr;
> +	}
> +	pht->iaddr = pci_iomap(pdev, 2, 0);
> +	if (pht->iaddr == NULL) {
> +		dev_err(&pdev->dev, "can't remap input space\n");
> +		goto err_unmc;
> +	}
> +	pht->oaddr = pci_iomap(pdev, 3, 0);
> +	if (pht->oaddr == NULL) {
> +		dev_err(&pdev->dev, "can't remap output space\n");
> +		goto err_unmi;
> +	}
> +
> +	iowrite32(0, pht->caddr + PHN_IRQCTL);
> +	retval = request_irq(pdev->irq, phantom_isr,
> +			IRQF_SHARED | IRQF_DISABLED, "phantom", pht);
> +	if (retval) {
> +		dev_err(&pdev->dev, "can't establish ISR\n");
> +		goto err_unmo;
> +	}
> +
> +	pht->idev = input_allocate_device();
> +	if (pht->idev == NULL) {
> +		dev_err(&pdev->dev, "can't create input device\n");
> +		retval = -ENOMEM;
> +		goto err_irq;
> +	}
> +
> +	phantom_init_idev(pdev, pht);
> +
> +	retval = input_ff_create_memless(pht->idev, NULL, phantom_play);
> +	if (retval) {
> +		dev_err(&pdev->dev, "can't create FF device\n");
> +		goto err_idev;
> +	}
> +
> +	pht->idev->ff->set_autocenter = phantom_autocenter;
> +
> +	retval = input_register_device(pht->idev);
> +	if (retval) {
> +		dev_err(&pdev->dev, "can't register input device\n");
> +		goto err_idev;
> +	}
> +
> +	pci_set_drvdata(pdev, pht);
> +
> +	return 0;
> +err_idev:
> +	input_free_device(pht->idev);
> +err_irq:
> +	free_irq(pdev->irq, pht);
> +err_unmo:
> +	pci_iounmap(pdev, pht->oaddr);
> +err_unmi:
> +	pci_iounmap(pdev, pht->iaddr);
> +err_unmc:
> +	pci_iounmap(pdev, pht->caddr);
> +err_fr:
> +	kfree(pht);
> +err_reg:
> +	pci_release_regions(pdev);

err_disable:
	pci_disable_device(pdev);

> +err:
> +	return retval;
> +}
> +
> +static void __devexit phantom_remove(struct pci_dev *pdev)
> +{
> +	struct phantom_device *pht = pci_get_drvdata(pdev);
> +
> +	iowrite32(0, pht->caddr + PHN_IRQCTL);
> +
> +	input_unregister_device(pht->idev);
> +
> +	free_irq(pdev->irq, pht);
> +
> +	pci_iounmap(pdev, pht->oaddr);
> +	pci_iounmap(pdev, pht->iaddr);
> +	pci_iounmap(pdev, pht->caddr);
> +
> +	kfree(pht);
> +
> +	pci_release_regions(pdev);
> +}

#ifdef CONFIG_PM

> +static int phantom_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> +	struct phantom_device *dev = pci_get_drvdata(pdev);
> +
> +	iowrite32(0, dev->caddr + PHN_IRQCTL);
> +
> +	return 0;
> +}
> +
> +static int phantom_resume(struct pci_dev *pdev)
> +{
> +	struct phantom_device *dev = pci_get_drvdata(pdev);
> +
> +	iowrite32(0, dev->caddr + PHN_IRQCTL);
> +
> +	return 0;
> +}

#else
#define phantom_suspend NULL
#define phantom_resume NULL
#endif

(then test it with CONFIG_PM=n!)

> +static struct pci_device_id phantom_pci_tbl[] __devinitdata = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9050),
> +		.class = PCI_CLASS_BRIDGE_OTHER << 8, .class_mask = 0xffff00 },
> +	{ 0, }
> +};
> +MODULE_DEVICE_TABLE(pci, phantom_pci_tbl);
> +
> +static struct pci_driver phantom_pci_driver = {
> +	.name = "phantom",
> +	.id_table = phantom_pci_tbl,
> +	.probe = phantom_probe,
> +	.remove = __devexit_p(phantom_remove),
> +	.suspend = phantom_suspend,
> +	.resume = phantom_resume
> +};

This goes into the read/write section.  Make it const?

> +static ssize_t phantom_show_version(struct class *cls, char *buf)
> +{
> +	return sprintf(buf, PHANTOM_VERSION "\n");
> +}
> +
> +static CLASS_ATTR(version, 0444, phantom_show_version, NULL);
> +
> +static int __init phantom_init(void)
> +{
> +	int retval;
> +
> +	phantom_class = class_create(THIS_MODULE, "phantom");
> +	if (IS_ERR(phantom_class)) {
> +		retval = PTR_ERR(phantom_class);
> +		printk(KERN_ERR "phantom: can't register phantom class\n");
> +		goto err;
> +	}
> +	retval = class_create_file(phantom_class, &class_attr_version);
> +	if (retval) {
> +		printk(KERN_ERR "phantom: can't create sysfs version file\n");
> +		goto err_class;
> +	}
> +
> +	retval = pci_register_driver(&phantom_pci_driver);
> +	if (retval) {
> +		printk(KERN_ERR "phantom: can't register pci driver\n");
> +		goto err_attr;
> +	}
> +
> +	printk(KERN_INFO "Phantom Linux Driver, version " PHANTOM_VERSION ", "
> +			"init OK\n");
> +
> +	return 0;
> +err_attr:
> +	class_remove_file(phantom_class, &class_attr_version);
> +err_class:
> +	class_destroy(phantom_class);
> +err:
> +	return retval;
> +}
> +
> +static void __exit phantom_exit(void)
> +{
> +	pci_unregister_driver(&phantom_pci_driver);
> +
> +	class_remove_file(phantom_class, &class_attr_version);
> +	class_destroy(phantom_class);
> +
> +	pr_debug("phantom: module successfully removed\n");
> +}
> +
> +module_init(phantom_init);
> +module_exit(phantom_exit);
> +
> +MODULE_AUTHOR("Jiri Slaby <jirislaby@...il.com>");
> +MODULE_DESCRIPTION("Sensable Phantom driver");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(PHANTOM_VERSION);

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists