[PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Paul Parsons]

Add support for the Synaptics NavPoint connected to a PXA27x SSP port in SPI
slave mode. The driver implements a simple navigation pad. The four raised dots
are mapped to UP/DOWN/LEFT/RIGHT buttons and the centre of the touchpad is
mapped to the ENTER button.

Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
---

V2: Split into 2 parts for hx4700 specific code and navpoint driver. Added
GPIO23_SSP1_SCLK_IN and GPIO102_NAVPOINT_PWR to mfp-pxa27x.h. Replaced power
on/off callback with gpio number. Added msleep() to SSP port ready loop.

diff -uprN clean-3.0.1/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h linux-3.0.1/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
--- clean-3.0.1/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h	2011-08-05 05:59:21.000000000 +0100
+++ linux-3.0.1/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h	2011-08-11 16:49:00.004450942 +0100
@@ -206,6 +206,7 @@
 #define GPIO113_I2S_SYSCLK	MFP_CFG_OUT(GPIO113, AF1, DRIVE_LOW)
 
 /* SSP 1 */
+#define GPIO23_SSP1_SCLK_IN	MFP_CFG_IN(GPIO23, AF2)
 #define GPIO23_SSP1_SCLK	MFP_CFG_OUT(GPIO23, AF2, DRIVE_LOW)
 #define GPIO29_SSP1_SCLK	MFP_CFG_IN(GPIO29, AF3)
 #define GPIO27_SSP1_SYSCLK	MFP_CFG_OUT(GPIO27, AF1, DRIVE_LOW)
@@ -434,6 +435,9 @@
 #define GPIO112_nMSINS		MFP_CFG_IN(GPIO112, AF2)
 #define GPIO32_MSSCLK		MFP_CFG_OUT(GPIO32, AF1, DRIVE_LOW)
 
+/* Touchpad Controller */
+#define GPIO102_NAVPOINT_PWR	MFP_CFG_OUT(GPIO102, AF0, DRIVE_LOW)
+
 /* commonly used pin configurations */
 #define GPIOxx_LCD_16BPP	\
 	GPIO58_LCD_LDD_0,	\
diff -uprN clean-3.0.1/drivers/input/mouse/Kconfig linux-3.0.1/drivers/input/mouse/Kconfig
--- clean-3.0.1/drivers/input/mouse/Kconfig	2011-08-05 05:59:21.000000000 +0100
+++ linux-3.0.1/drivers/input/mouse/Kconfig	2011-08-11 16:49:00.004450942 +0100
@@ -322,4 +322,14 @@ config MOUSE_SYNAPTICS_I2C
 	  To compile this driver as a module, choose M here: the
 	  module will be called synaptics_i2c.
 
+config MOUSE_NAVPOINT_PXA27x
+	bool "Synaptics NavPoint (PXA27x SSP/SPI)"
+	depends on PXA27x && PXA_SSP
+	help
+	  This option enables support for the Synaptics NavPoint connected to
+	  a PXA27x SSP port in SPI slave mode. The driver implements a simple
+	  navigation pad. The four raised dots are mapped to UP/DOWN/LEFT/RIGHT
+	  buttons and the centre of the touchpad is mapped to the ENTER button.
+	  Say Y to enable the Synaptics NavPoint on the HP iPAQ hx4700.
+
 endif
diff -uprN clean-3.0.1/drivers/input/mouse/Makefile linux-3.0.1/drivers/input/mouse/Makefile
--- clean-3.0.1/drivers/input/mouse/Makefile	2011-08-05 05:59:21.000000000 +0100
+++ linux-3.0.1/drivers/input/mouse/Makefile	2011-08-11 16:49:00.004450942 +0100
@@ -19,6 +19,7 @@ obj-$(CONFIG_MOUSE_RISCPC)		+= rpcmouse.
 obj-$(CONFIG_MOUSE_SERIAL)		+= sermouse.o
 obj-$(CONFIG_MOUSE_SYNAPTICS_I2C)	+= synaptics_i2c.o
 obj-$(CONFIG_MOUSE_VSXXXAA)		+= vsxxxaa.o
+obj-$(CONFIG_MOUSE_NAVPOINT_PXA27x)	+= navpoint.o
 
 psmouse-objs := psmouse-base.o synaptics.o
 
diff -uprN clean-3.0.1/drivers/input/mouse/navpoint.c linux-3.0.1/drivers/input/mouse/navpoint.c
--- clean-3.0.1/drivers/input/mouse/navpoint.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-3.0.1/drivers/input/mouse/navpoint.c	2011-08-11 16:50:35.093084330 +0100
@@ -0,0 +1,317 @@
+/*
+ *  Copyright (C) 2011 Paul Parsons <lost.distance@yahoo.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/input.h>
+#include <linux/input/navpoint.h>
+#include <linux/interrupt.h>
+#include <linux/pxa2xx_ssp.h>
+#include <linux/slab.h>
+
+/*
+ *	Synaptics NavPoint (PXA27x SSP/SPI) driver.
+ */
+
+struct driver_data {
+	struct ssp_device *ssp;
+	int		gpio;
+	struct input_dev *input;
+	int		index;
+	uint8_t		data[8];
+	int		pressed;	/* 1 = pressed, 0 = released */
+	unsigned	code;		/* Key code of the last key pressed */
+};
+
+static const u32 sscr0 = 0
+	| SSCR0_TUM		/* TIM = 1; No TUR interrupts */
+	| SSCR0_RIM		/* RIM = 1; No ROR interrupts */
+	| SSCR0_SSE		/* SSE = 1; SSP enabled */
+	| SSCR0_Motorola	/* FRF = 0; Motorola SPI */
+	| SSCR0_DataSize(16)	/* DSS = 15; Data size = 16-bit */
+	;
+static const u32 sscr1 = 0
+	| SSCR1_SCFR		/* SCFR = 1; SSPSCLK only during transfers */
+	| SSCR1_SCLKDIR		/* SCLKDIR = 1; Slave mode */
+	| SSCR1_SFRMDIR		/* SFRMDIR = 1; Slave mode */
+	| SSCR1_RWOT		/* RWOT = 1; Receive without transmit mode */
+	| SSCR1_RxTresh(1)	/* RFT = 0; Receive FIFO threshold = 1 */
+	| SSCR1_SPH		/* SPH = 1; SSPSCLK inactive 0.5 + 1 cycles */
+	| SSCR1_RIE		/* RIE = 1; Receive FIFO interrupt enabled */
+	;
+static const u32 sssr = 0
+	| SSSR_BCE		/* BCE = 1; Clear BCE */
+	| SSSR_TUR		/* TUR = 1; Clear TUR */
+	| SSSR_EOC		/* EOC = 1; Clear EOC */
+	| SSSR_TINT		/* TINT = 1; Clear TINT */
+	| SSSR_PINT		/* PINT = 1; Clear PINT */
+	| SSSR_ROR		/* ROR = 1; Clear ROR */
+	;
+
+/*
+ *	MEP Query $22: Touchpad Coordinate Range Query is not supported by
+ *	the NavPoint module, so sampled values provide the N/S/E/W limits.
+ */
+
+#define WEST	2416		/* 1/3 width */
+#define EAST	3904		/* 2/3 width */
+#define SOUTH	2480		/* 1/3 height */
+#define NORTH	3424		/* 2/3 height */
+
+static void navpoint_packet(void *dev)
+{
+	struct driver_data *drv_data;
+	int pressed;
+	unsigned x, y;
+	unsigned code;
+
+	drv_data = dev;
+
+	switch (drv_data->data[0]) {
+	case 0xff:	/* Garbage (packet?) between reset and Hello packet */
+	case 0x00:	/* Module 0, NULL packet */
+		break;
+	case 0x0e:	/* Module 0, Absolute packet */
+		pressed = (drv_data->data[1] & 0x01);
+		if ((drv_data->pressed ^ pressed) == 0)	/* No change */
+			break;
+		drv_data->pressed = pressed;
+		x = ((drv_data->data[2] & 0x1f) << 8) | drv_data->data[3];
+		y = ((drv_data->data[4] & 0x1f) << 8) | drv_data->data[5];
+		if (x < WEST)
+			code = KEY_LEFT;
+		else if (x > EAST)
+			code = KEY_RIGHT;
+		else if (y < SOUTH)
+			code = KEY_DOWN;
+		else if (y > NORTH)
+			code = KEY_UP;
+		else
+			code = KEY_ENTER;
+		if (pressed)
+			drv_data->code = code;
+		input_report_key(drv_data->input, drv_data->code, pressed);
+		input_sync(drv_data->input);
+		break;
+	case 0x19:	/* Module 0, Hello packet */
+		if ((drv_data->data[1] & 0xf0) == 0x10)
+			break;
+		/* FALLTHROUGH */
+	default:
+		dev_warn(dev, "spurious packet: 0x%02x,0x%02x,...\n",
+			drv_data->data[0],
+			drv_data->data[1]);
+		break;
+	}
+
+	drv_data->index = 0;
+}
+
+static irqreturn_t navpoint_int(int irq, void *dev)
+{
+	struct driver_data *drv_data;
+	struct ssp_device *ssp;
+	u32 status;
+	irqreturn_t ret;
+
+	drv_data = dev;
+	ssp = drv_data->ssp;
+
+	status = pxa_ssp_read_reg(ssp, SSSR);
+	ret = (status & (sssr | SSSR_RFS)) ? IRQ_HANDLED : IRQ_NONE;
+
+	if (status & sssr) {
+		dev_warn(dev, "spurious interrupt: 0x%02x\n", status);
+		pxa_ssp_write_reg(ssp, SSSR, (status & sssr));
+	}
+
+	while (status & SSSR_RNE) {
+		u32 data;
+
+		data = pxa_ssp_read_reg(ssp, SSDR);
+		drv_data->data[drv_data->index + 0] = (data >> 8);
+		drv_data->data[drv_data->index + 1] = data;
+		drv_data->index += 2;
+		if ((drv_data->data[0] & 0x07) < drv_data->index)
+			navpoint_packet(dev);
+		status = pxa_ssp_read_reg(ssp, SSSR);
+	}
+
+	return ret;
+}
+
+#ifdef CONFIG_SUSPEND
+int navpoint_suspend(struct device *dev)
+{
+	struct driver_data *drv_data;
+	struct ssp_device *ssp;
+
+	drv_data = dev_get_drvdata(dev);
+	ssp = drv_data->ssp;
+
+	if (drv_data->gpio)
+		gpio_set_value(drv_data->gpio, 0);
+
+	pxa_ssp_write_reg(ssp, SSCR0, 0);
+
+	clk_disable(ssp->clk);
+
+	return 0;
+}
+
+int navpoint_resume(struct device *dev)
+{
+	struct driver_data *drv_data;
+	struct ssp_device *ssp;
+
+	drv_data = dev_get_drvdata(dev);
+	ssp = drv_data->ssp;
+
+	clk_enable(ssp->clk);
+
+	pxa_ssp_write_reg(ssp, SSCR1, sscr1);
+	pxa_ssp_write_reg(ssp, SSSR, sssr);
+	pxa_ssp_write_reg(ssp, SSTO, 0);
+	pxa_ssp_write_reg(ssp, SSCR0, sscr0);	/* SSCR0_SSE written last */
+
+	if (drv_data->gpio)
+		gpio_set_value(drv_data->gpio, 1);
+
+	/* Wait until SSP port is ready for slave clock operations */
+	while (pxa_ssp_read_reg(ssp, SSSR) & SSSR_CSS)
+		msleep(1);
+
+	return 0;
+}
+
+static const struct dev_pm_ops navpoint_pm_ops = {
+	.suspend	= navpoint_suspend,
+	.resume		= navpoint_resume,
+};
+#endif
+
+static int __devinit navpoint_probe(struct platform_device *pdev)
+{
+	struct navpoint_platform_data *pdata = pdev->dev.platform_data;
+	int ret;
+	struct driver_data *drv_data;
+	struct ssp_device *ssp;
+	struct input_dev *input;
+
+	drv_data = kzalloc(sizeof(struct driver_data), GFP_KERNEL);
+	if (!drv_data) {
+		ret = -ENOMEM;
+		goto ret0;
+	}
+
+	ssp = pxa_ssp_request(pdata->port, pdev->name);
+	if (!ssp) {
+		ret = -ENODEV;
+		goto ret1;
+	}
+
+	ret = request_irq(ssp->irq, navpoint_int, 0, pdev->name, drv_data);
+	if (ret)
+		goto ret2;
+
+	input = input_allocate_device();
+	if (!input) {
+		ret = -ENOMEM;
+		goto ret3;
+	}
+	input->name = pdev->name;
+	__set_bit(EV_KEY, input->evbit);
+	__set_bit(KEY_ENTER, input->keybit);
+	__set_bit(KEY_UP, input->keybit);
+	__set_bit(KEY_LEFT, input->keybit);
+	__set_bit(KEY_RIGHT, input->keybit);
+	__set_bit(KEY_DOWN, input->keybit);
+	input->dev.parent = &pdev->dev;
+	ret = input_register_device(input);
+	if (ret)
+		goto ret4;
+
+	drv_data->ssp = ssp;
+	drv_data->gpio = pdata->gpio;
+	drv_data->input = input;
+
+	platform_set_drvdata(pdev, drv_data);
+
+	(void) navpoint_resume(&pdev->dev);
+
+	dev_info(&pdev->dev, "ssp%d, irq %d\n", pdata->port, ssp->irq);
+
+	return 0;
+
+ret4:
+	input_free_device(input);
+ret3:
+	free_irq(ssp->irq, drv_data);
+ret2:
+	pxa_ssp_free(ssp);
+ret1:
+	kfree(drv_data);
+ret0:
+	return ret;
+}
+
+static int __devexit navpoint_remove(struct platform_device *pdev)
+{
+	struct driver_data *drv_data;
+	struct ssp_device *ssp;
+	struct input_dev *input;
+
+	drv_data = platform_get_drvdata(pdev);
+	ssp = drv_data->ssp;
+	input = drv_data->input;
+
+	(void) navpoint_suspend(&pdev->dev);
+
+	input_unregister_device(input);
+
+	free_irq(ssp->irq, drv_data);
+
+	pxa_ssp_free(ssp);
+
+	kfree(drv_data);
+
+	return 0;
+}
+
+static struct platform_driver navpoint_driver = {
+	.probe		= navpoint_probe,
+	.remove		= __devexit_p(navpoint_remove),
+	.driver = {
+		.name	= "navpoint",
+		.owner	= THIS_MODULE,
+#ifdef CONFIG_SUSPEND
+		.pm	= &navpoint_pm_ops,
+#endif
+	},
+};
+
+static int __init navpoint_init(void)
+{
+	return platform_driver_register(&navpoint_driver);
+}
+
+static void __exit navpoint_exit(void)
+{
+	platform_driver_unregister(&navpoint_driver);
+}
+
+module_init(navpoint_init);
+module_exit(navpoint_exit);
+
+MODULE_AUTHOR("Paul Parsons <lost.distance@yahoo.com>");
+MODULE_DESCRIPTION("Synaptics NavPoint (PXA27x SSP/SPI) driver");
+MODULE_LICENSE("GPL");
diff -uprN clean-3.0.1/include/linux/input/navpoint.h linux-3.0.1/include/linux/input/navpoint.h
--- clean-3.0.1/include/linux/input/navpoint.h	1970-01-01 01:00:00.000000000 +0100
+++ linux-3.0.1/include/linux/input/navpoint.h	2011-08-11 16:50:50.761188696 +0100
@@ -0,0 +1,12 @@
+/*
+ *  Copyright (C) 2011 Paul Parsons <lost.distance@yahoo.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ */
+
+struct navpoint_platform_data {
+	int		port;		/* PXA SSP port for pxa_ssp_request() */
+	int		gpio;		/* GPIO for power on/off */
+};

[PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Eric Miao]

On Sun, Aug 14, 2011 at 6:31 PM, Paul Parsons <lost.distance@yahoo.com> wrote:
> Hi Igor,
>
> Thanks for reviewing the driver. Specific responses below.
>
> ? ? This patch should be 1/n as the other patches depend on it,
> ? ? should not include the mfp stuff and should be sent also to
> ? ? linux-input list
> ? ? and the input subsystem maintainer (both added to CC).
>
> OK, will do.
>
> +/* Touchpad Controller */
> +#define GPIO102_NAVPOINT_PWR??? MFP_CFG_OUT(GPIO102, AF0, DRIVE_LOW)
> +
>
> ? ? The name is not generic, so IMO can't be placed in the generic file.
> ? ? Can't you use the already existing GPIO102_GPIO macro
> ? ? and then change the direction in the board or driver file?
>
> MFP_CFG_OUT(GPIO102, ...) had already moved from the patch v1 platform file to patch v2 mfp-pxa27x.h because it was suggested that MFP macros should not be used directly.
> Changing the direction in the platform file would surely require using the MFP macros again, so how to keep everyone happy? Maybe I should just define a more generic name such as GPIO102_GPIO_OUT?

I guess we need a new definition for this config, looks like SCLK here
is different from a normal configuration because the pxa27x is used
as a SPI slave here?

And checked a bit with the pxa27x developer manual, there is no SCLK
for this GPIO102????

>
> +config MOUSE_NAVPOINT_PXA27x
> +??? bool "Synaptics NavPoint (PXA27x SSP/SPI)"
> +??? depends on PXA27x && PXA_SSP
> +??? help
> +??? ? This option enables support for the Synaptics NavPoint connected to
> +??? ? a PXA27x SSP port in SPI slave mode. The driver implements a simple
> +??? ? navigation pad. The four raised dots are mapped to UP/DOWN/LEFT/RIGHT
> +??? ? buttons and the centre of the touchpad is mapped to the ENTER button.
> +??? ? Say Y to enable the Synaptics NavPoint on the HP iPAQ hx4700.
>
> ? ? There is no direct dependency for HP iPAQ hx4700,
> ? ? so in theory each board that want to use it can use it.
> ? ? I'd remove the "on the HP iPAQ ..." from the above sentence.
>
> I copied that wording from my earlier ASIC3 LED driver. I suppose my feeling was that it seemed unlikely that any other platform would ever use this driver (otherwise I wouldn't be the first to write it). I'm happy to remove it.
>
> +obj-$(CONFIG_MOUSE_NAVPOINT_PXA27x)??? += navpoint.o
>
> ? ? This does not look like mouse device... Why place it in the mouse
> ? ? directory?
> ? ? I think the most reasonable place would be drivers/input/keyboard
>
> Because the underlying hardware is a touchpad controller and most of the touchpad drivers live in the mouse directory. I could have added a mouse interface to this driver but chose not to (at least for now) because the hx4700 platform already has a working touchscreen controller; a second mouse device was not needed. If future platforms require a mouse interface then one could be added relatively easily; surely this would be preferable to replicating the whole driver.
>
> +struct driver_data {
> +??? struct ssp_device *ssp;
> +??? int??? ??? gpio;
> +??? struct input_dev *input;
> +??? int??? ??? index;
> +??? uint8_t??? ??? data[8];
> +??? int??? ??? pressed;??? /* 1 = pressed, 0 = released */
> +??? unsigned??? code;??? ??? /* Key code of the last key pressed */
>
> ? ? Why do you need it?
>
> The code field? To match a button release with a button press. See later.
>
> +#define WEST??? 2416??? ??? /* 1/3 width */
> +#define EAST??? 3904??? ??? /* 2/3 width */
> +#define SOUTH??? 2480??? ??? /* 1/3 height */
> +#define NORTH??? 3424??? ??? /* 2/3 height */
>
> ? ? May be supply those via the platform_data?
>
> OK, will do.
>
> +??? drv_data = dev;
>
> ? ? Can be done in the declaration line.
>
> OK.
>
> +??? ??? if (pressed)
> +??? ??? ??? drv_data->code = code;
>
> ? ? Why do you need to store the code? You don't use it in any place...
>
> I use it in the next line. The store is conditional whereas the read is unconditional. That's why I need to store it.
>
> +??? drv_data = dev;
>
> ? ? This can be done in the declaration line.
>
> OK.
>
> +??? status = pxa_ssp_read_reg(ssp, SSSR);
> +??? ret = (status & (sssr | SSSR_RFS)) ? IRQ_HANDLED : IRQ_NONE;
> +
> +??? if (status & sssr) {
> +??? ??? dev_warn(dev, "spurious interrupt: 0x%02x\n", status);
> +??? ??? pxa_ssp_write_reg(ssp, SSSR, (status & sssr));
> +??? }
> +
> +??? while (status & SSSR_RNE) {
> +??? ??? u32 data;
> +
> +??? ??? data = pxa_ssp_read_reg(ssp, SSDR);
> +??? ??? drv_data->data[drv_data->index + 0] = (data >> 8);
> +??? ??? drv_data->data[drv_data->index + 1] = data;
> +??? ??? drv_data->index += 2;
> +??? ??? if ((drv_data->data[0] & 0x07) < drv_data->index)
> +??? ??? ??? navpoint_packet(dev);
> +??? ??? status = pxa_ssp_read_reg(ssp, SSSR);
> +??? }
>
> ? ? Should all this really be done in the interrupt context?
>
> Yes. The FIFO controller triggers a CPU interrupt when the number of RX FIFO entries >= SSCR1.RFT (RX FIFO threshold), which I have set to 1. If I don't drain the RX FIFO then the interrupt source will not be cleared and the interrupt handler will be re-entered immediately (I have verified this). In short, draining the RX FIFO clears the interrupt.
>
> +??? drv_data = dev_get_drvdata(dev);
> +??? ssp = drv_data->ssp;
>
> ? ? The above two can be done in the declaration line...
>
> OK.
>
> +??? drv_data = dev_get_drvdata(dev);
> +??? ssp = drv_data->ssp;
>
> ? ? The above two can be done in the declaration line...
>
> OK.
>
> +??? drv_data->gpio = pdata->gpio;
>
> ? ? I'd suggest checking if the supplied gpio is valid
> ? ? and also may be configure it for output?
>
> The suspend and resume functions check that the gpio is valid (which is taken to be non-zero) before using it. The platform file has already configured the gpio for output; it's the GPIO102 discussed earlier.
>
> +??? (void) navpoint_resume(&pdev->dev);
>
> ? ? Will this compile if !CONFIG_SUSPEND?
>
> oops. OK.
>
> +??? drv_data = platform_get_drvdata(pdev);
> +??? ssp = drv_data->ssp;
> +??? input = drv_data->input;
>
> ? ? You need neither ssp nor input variables - you only use them once.
> ? ? This will remove 4 lines...
>
> Actually the ssp variable is used twice. Regardless, by declaring those two variables I ensured that most of navpoint_remove() was identical to the error return of navpoint_probe(). I suppose I value consistency more than saving lines. I'm happy to move those two assignments to the declaration line.
>
> +??? (void) navpoint_suspend(&pdev->dev);
>
> ? ? and will this compile if !CONFIG_SUSPEND?
>
> oops again. OK.
>
> Regards,
> Paul
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gpio-102.png
Type: image/png
Size: 11983 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110815/231c65cf/attachment.png>

[PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Paul Parsons]

--- On Mon, 15/8/11, Eric Miao <eric.y.miao@gmail.com> wrote:
> > MFP_CFG_OUT(GPIO102, ...) had already moved from the
> patch v1 platform file to patch v2 mfp-pxa27x.h because it
> was suggested that MFP macros should not be used directly.
> > Changing the direction in the platform file would
> surely require using the MFP macros again, so how to keep
> everyone happy? Maybe I should just define a more generic
> name such as GPIO102_GPIO_OUT?
> 
> I guess we need a new definition for this config, looks
> like SCLK here
> is different from a normal configuration because the pxa27x
> is used
> as a SPI slave here?
> 
> And checked a bit with the pxa27x developer manual, there
> is no SCLK
> for this GPIO102????

GPIO102 is apparently a navpoint power enable; the old linux-2.6.21-hh20 kernel from handhelds.org defines it thus:

#define GPIO_NR_HX4700_SYNAPTICS_POWER_ON        102

I verified that it does need to be asserted before the navpoint will work. I don't possess any hx4700 developer documentation; perhaps Philipp can shed some light on GPIO102.

[PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Eric Miao]

On Mon, Aug 15, 2011 at 5:49 PM, Paul Parsons <lost.distance@yahoo.com> wrote:
> --- On Mon, 15/8/11, Eric Miao <eric.y.miao@gmail.com> wrote:
>> > MFP_CFG_OUT(GPIO102, ...) had already moved from the
>> patch v1 platform file to patch v2 mfp-pxa27x.h because it
>> was suggested that MFP macros should not be used directly.
>> > Changing the direction in the platform file would
>> surely require using the MFP macros again, so how to keep
>> everyone happy? Maybe I should just define a more generic
>> name such as GPIO102_GPIO_OUT?
>>
>> I guess we need a new definition for this config, looks
>> like SCLK here
>> is different from a normal configuration because the pxa27x
>> is used
>> as a SPI slave here?
>>
>> And checked a bit with the pxa27x developer manual, there
>> is no SCLK
>> for this GPIO102????
>
> GPIO102 is apparently a navpoint power enable; the old linux-2.6.21-hh20 kernel from handhelds.org defines it thus:
>
> #define GPIO_NR_HX4700_SYNAPTICS_POWER_ON ? ? ? ?102
>
> I verified that it does need to be asserted before the navpoint will work. I don't possess any hx4700 developer documentation; perhaps Philipp can shed some light on GPIO102.
>

Ah, that case it's simply a GPIO then. You could simply use GPIO102_GPIO
to configure the pin. And direction is controlled by gpio_direction_*().

[PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Igor Grinberg]

My mailer started converting stuff to html from some point, so I send this again, sorry..


On 08/14/11 13:31, Paul Parsons wrote:

> Hi Igor,
>
> Thanks for reviewing the driver. Specific responses below.
>
>     This patch should be 1/n as the other patches depend on it,
>     should not include the mfp stuff and should be sent also to
>     linux-input list
>     and the input subsystem maintainer (both added to CC).
>
> OK, will do.
>
> +/* Touchpad Controller */
> +#define GPIO102_NAVPOINT_PWR    MFP_CFG_OUT(GPIO102, AF0, DRIVE_LOW)
> +
>
>     The name is not generic, so IMO can't be placed in the generic file.
>     Can't you use the already existing GPIO102_GPIO macro
>     and then change the direction in the board or driver file?
>
> MFP_CFG_OUT(GPIO102, ...) had already moved from the patch v1 platform file to patch v2 mfp-pxa27x.h because it was suggested that MFP macros should not be used directly.
> Changing the direction in the platform file would surely require using the MFP macros again, so how to keep everyone happy? Maybe I should just define a more generic name such as GPIO102_GPIO_OUT?

No, you will not need to use the MFP macros, just gpio_direction_*() calls.
Eric has already answered this and I agree with him.
Use the MFP to configure the alternate function and other MFP related stuff
and use the gpio_direction_*() calls later to set the direction of the GPIO.

>     
> +config MOUSE_NAVPOINT_PXA27x
> +    bool "Synaptics NavPoint (PXA27x SSP/SPI)"
> +    depends on PXA27x && PXA_SSP
> +    help
> +      This option enables support for the Synaptics NavPoint connected to
> +      a PXA27x SSP port in SPI slave mode. The driver implements a simple
> +      navigation pad. The four raised dots are mapped to UP/DOWN/LEFT/RIGHT
> +      buttons and the centre of the touchpad is mapped to the ENTER button.
> +      Say Y to enable the Synaptics NavPoint on the HP iPAQ hx4700.
>
>     There is no direct dependency for HP iPAQ hx4700,
>     so in theory each board that want to use it can use it.
>     I'd remove the "on the HP iPAQ ..." from the above sentence.
>
> I copied that wording from my earlier ASIC3 LED driver. I suppose my feeling was that it seemed unlikely that any other platform would ever use this driver (otherwise I wouldn't be the first to write it). I'm happy to remove it.
>     
> +obj-$(CONFIG_MOUSE_NAVPOINT_PXA27x)    += navpoint.o
>  
>     This does not look like mouse device... Why place it in the mouse
>     directory?
>     I think the most reasonable place would be drivers/input/keyboard
>
> Because the underlying hardware is a touchpad controller and most of the touchpad drivers live in the mouse directory. I could have added a mouse interface to this driver but chose not to (at least for now) because the hx4700 platform already has a working touchscreen controller; a second mouse device was not needed. If future platforms require a mouse interface then one could be added relatively easily; surely this would be preferable to replicating the whole driver.

So the device is a full touchpad? and can be used for mouse pointer?
Is the same device used as mouse pointer by some other driver?

> +struct driver_data {
> +    struct ssp_device *ssp;
> +    int        gpio;
> +    struct input_dev *input;
> +    int        index;
> +    uint8_t        data[8];
> +    int        pressed;    /* 1 = pressed, 0 = released */
> +    unsigned    code;        /* Key code of the last key pressed */
>
>     Why do you need it?
>
> The code field? To match a button release with a button press. See later.
>     
> +#define WEST    2416        /* 1/3 width */
> +#define EAST    3904        /* 2/3 width */
> +#define SOUTH    2480        /* 1/3 height */
> +#define NORTH    3424        /* 2/3 height */
>
>     May be supply those via the platform_data?
>
> OK, will do.
>
> +    drv_data = dev;
>
>     Can be done in the declaration line.
>
> OK.
>
> +        if (pressed)
> +            drv_data->code = code;
>
>     Why do you need to store the code? You don't use it in any place...
>
> I use it in the next line. The store is conditional whereas the read is unconditional. That's why I need to store it.

Ok. I've missed that fact, sorry..

> +    drv_data = dev;
>
>     This can be done in the declaration line.
>
> OK.
>
> +    status = pxa_ssp_read_reg(ssp, SSSR);
> +    ret = (status & (sssr | SSSR_RFS)) ? IRQ_HANDLED : IRQ_NONE;
> +
> +    if (status & sssr) {
> +        dev_warn(dev, "spurious interrupt: 0x%02x\n", status);
> +        pxa_ssp_write_reg(ssp, SSSR, (status & sssr));
> +    }
> +
> +    while (status & SSSR_RNE) {
> +        u32 data;
> +
> +        data = pxa_ssp_read_reg(ssp, SSDR);
> +        drv_data->data[drv_data->index + 0] = (data >> 8);
> +        drv_data->data[drv_data->index + 1] = data;
> +        drv_data->index += 2;
> +        if ((drv_data->data[0] & 0x07) < drv_data->index)
> +            navpoint_packet(dev);
> +        status = pxa_ssp_read_reg(ssp, SSSR);
> +    }
>
>     Should all this really be done in the interrupt context?
>
> Yes. The FIFO controller triggers a CPU interrupt when the number of RX FIFO entries >= SSCR1.RFT (RX FIFO threshold), which I have set to 1. If I don't drain the RX FIFO then the interrupt source will not be cleared and the interrupt handler will be re-entered immediately (I have verified this). In short, draining the RX FIFO clears the interrupt.
>
> +    drv_data = dev_get_drvdata(dev);
> +    ssp = drv_data->ssp;
>
>     The above two can be done in the declaration line...
>
> OK.
>
> +    drv_data = dev_get_drvdata(dev);
> +    ssp = drv_data->ssp;
>
>     The above two can be done in the declaration line...
>
> OK.
>
> +    drv_data->gpio = pdata->gpio;
>
>     I'd suggest checking if the supplied gpio is valid
>     and also may be configure it for output?
>
> The suspend and resume functions check that the gpio is valid (which is taken to be non-zero) before using it. The platform file has already configured the gpio for output; it's the GPIO102 discussed earlier.

What I'm suggesting here is check that the gpio is valid here and only once
and not in the suspend/resume functions. That gpio is not going to change,
so there is no point is checking it each time in suspend/resume.
Also the check for non-zero is not correct. 0 is a valid gpio number on most platforms.
You need to use the gpio_is_valid() call for this.

> +    (void) navpoint_resume(&pdev->dev);
>
>     Will this compile if !CONFIG_SUSPEND?
>
> oops. OK.
>
> +    drv_data = platform_get_drvdata(pdev);
> +    ssp = drv_data->ssp;
> +    input = drv_data->input;
>
>     You need neither ssp nor input variables - you only use them once.
>     This will remove 4 lines...
>
> Actually the ssp variable is used twice. Regardless, by declaring those two variables I ensured that most of navpoint_remove() was identical to the error return of navpoint_probe(). I suppose I value consistency more than saving lines. I'm happy to move those two assignments to the declaration line.
>
> +    (void) navpoint_suspend(&pdev->dev);
>
>     and will this compile if !CONFIG_SUSPEND?
>
> oops again. OK.
>
> Regards,
> Paul
>

-- 
Regards,
Igor.

[PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Marek Vasut]

On Thursday, August 11, 2011 06:26:30 PM Paul Parsons wrote:
> Add support for the Synaptics NavPoint connected to a PXA27x SSP port in
> SPI slave mode. The driver implements a simple navigation pad. The four
> raised dots are mapped to UP/DOWN/LEFT/RIGHT buttons and the centre of the
> touchpad is mapped to the ENTER button.
> 
> Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
> ---
> 
> V2: Split into 2 parts for hx4700 specific code and navpoint driver. Added
> GPIO23_SSP1_SCLK_IN and GPIO102_NAVPOINT_PWR to mfp-pxa27x.h. Replaced
> power on/off callback with gpio number. Added msleep() to SSP port ready
> loop.
> 
> diff -uprN clean-3.0.1/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
> linux-3.0.1/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h ---
> clean-3.0.1/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h	2011-08-05
> 05:59:21.000000000 +0100 +++
> linux-3.0.1/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h	2011-08-11
> 16:49:00.004450942 +0100 @@ -206,6 +206,7 @@
>  #define GPIO113_I2S_SYSCLK	MFP_CFG_OUT(GPIO113, AF1, DRIVE_LOW)
> 
>  /* SSP 1 */
> +#define GPIO23_SSP1_SCLK_IN	MFP_CFG_IN(GPIO23, AF2)
>  #define GPIO23_SSP1_SCLK	MFP_CFG_OUT(GPIO23, AF2, DRIVE_LOW)
>  #define GPIO29_SSP1_SCLK	MFP_CFG_IN(GPIO29, AF3)
>  #define GPIO27_SSP1_SYSCLK	MFP_CFG_OUT(GPIO27, AF1, DRIVE_LOW)
> @@ -434,6 +435,9 @@
>  #define GPIO112_nMSINS		MFP_CFG_IN(GPIO112, AF2)
>  #define GPIO32_MSSCLK		MFP_CFG_OUT(GPIO32, AF1, DRIVE_LOW)
> 
> +/* Touchpad Controller */
> +#define GPIO102_NAVPOINT_PWR	MFP_CFG_OUT(GPIO102, AF0, DRIVE_LOW)
> +
>  /* commonly used pin configurations */
>  #define GPIOxx_LCD_16BPP	\
>  	GPIO58_LCD_LDD_0,	\
> diff -uprN clean-3.0.1/drivers/input/mouse/Kconfig
> linux-3.0.1/drivers/input/mouse/Kconfig ---
> clean-3.0.1/drivers/input/mouse/Kconfig	2011-08-05 05:59:21.000000000
> +0100 +++ linux-3.0.1/drivers/input/mouse/Kconfig	2011-08-11
> 16:49:00.004450942 +0100 @@ -322,4 +322,14 @@ config MOUSE_SYNAPTICS_I2C
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called synaptics_i2c.
> 
> +config MOUSE_NAVPOINT_PXA27x
> +	bool "Synaptics NavPoint (PXA27x SSP/SPI)"
> +	depends on PXA27x && PXA_SSP
> +	help
> +	  This option enables support for the Synaptics NavPoint connected to
> +	  a PXA27x SSP port in SPI slave mode. The driver implements a simple
> +	  navigation pad. The four raised dots are mapped to UP/DOWN/LEFT/RIGHT
> +	  buttons and the centre of the touchpad is mapped to the ENTER button.
> +	  Say Y to enable the Synaptics NavPoint on the HP iPAQ hx4700.
> +
>  endif
> diff -uprN clean-3.0.1/drivers/input/mouse/Makefile
> linux-3.0.1/drivers/input/mouse/Makefile ---
> clean-3.0.1/drivers/input/mouse/Makefile	2011-08-05 05:59:21.000000000
> +0100 +++ linux-3.0.1/drivers/input/mouse/Makefile	2011-08-11
> 16:49:00.004450942 +0100 @@ -19,6 +19,7 @@ obj-$(CONFIG_MOUSE_RISCPC)		+=
> rpcmouse.
>  obj-$(CONFIG_MOUSE_SERIAL)		+= sermouse.o
>  obj-$(CONFIG_MOUSE_SYNAPTICS_I2C)	+= synaptics_i2c.o
>  obj-$(CONFIG_MOUSE_VSXXXAA)		+= vsxxxaa.o
> +obj-$(CONFIG_MOUSE_NAVPOINT_PXA27x)	+= navpoint.o

Keep the list sorted
> 
>  psmouse-objs := psmouse-base.o synaptics.o
> 
> diff -uprN clean-3.0.1/drivers/input/mouse/navpoint.c
> linux-3.0.1/drivers/input/mouse/navpoint.c ---
> clean-3.0.1/drivers/input/mouse/navpoint.c	1970-01-01 01:00:00.000000000
> +0100 +++ linux-3.0.1/drivers/input/mouse/navpoint.c	2011-08-11
> 16:50:35.093084330 +0100 @@ -0,0 +1,317 @@
> +/*
> + *  Copyright (C) 2011 Paul Parsons <lost.distance@yahoo.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/input.h>
> +#include <linux/input/navpoint.h>
> +#include <linux/interrupt.h>
> +#include <linux/pxa2xx_ssp.h>
> +#include <linux/slab.h>
> +
> +/*
> + *	Synaptics NavPoint (PXA27x SSP/SPI) driver.
> + */
> +
> +struct driver_data {
> +	struct ssp_device *ssp;
> +	int		gpio;
> +	struct input_dev *input;
> +	int		index;
> +	uint8_t		data[8];
> +	int		pressed;	/* 1 = pressed, 0 = released */
> +	unsigned	code;		/* Key code of the last key pressed */
> +};

Use tabs for alignment please.
> +
> +static const u32 sscr0 = 0
> +	| SSCR0_TUM		/* TIM = 1; No TUR interrupts */
> +	| SSCR0_RIM		/* RIM = 1; No ROR interrupts */
> +	| SSCR0_SSE		/* SSE = 1; SSP enabled */
> +	| SSCR0_Motorola	/* FRF = 0; Motorola SPI */
> +	| SSCR0_DataSize(16)	/* DSS = 15; Data size = 16-bit */
> +	;
> +static const u32 sscr1 = 0
> +	| SSCR1_SCFR		/* SCFR = 1; SSPSCLK only during transfers */
> +	| SSCR1_SCLKDIR		/* SCLKDIR = 1; Slave mode */
> +	| SSCR1_SFRMDIR		/* SFRMDIR = 1; Slave mode */
> +	| SSCR1_RWOT		/* RWOT = 1; Receive without transmit mode */
> +	| SSCR1_RxTresh(1)	/* RFT = 0; Receive FIFO threshold = 1 */
> +	| SSCR1_SPH		/* SPH = 1; SSPSCLK inactive 0.5 + 1 cycles */
> +	| SSCR1_RIE		/* RIE = 1; Receive FIFO interrupt enabled */
> +	;
> +static const u32 sssr = 0
> +	| SSSR_BCE		/* BCE = 1; Clear BCE */
> +	| SSSR_TUR		/* TUR = 1; Clear TUR */
> +	| SSSR_EOC		/* EOC = 1; Clear EOC */
> +	| SSSR_TINT		/* TINT = 1; Clear TINT */
> +	| SSSR_PINT		/* PINT = 1; Clear PINT */
> +	| SSSR_ROR		/* ROR = 1; Clear ROR */
> +	;
> +
> +/*
> + *	MEP Query $22: Touchpad Coordinate Range Query is not supported by
> + *	the NavPoint module, so sampled values provide the N/S/E/W limits.
> + */
> +
> +#define WEST	2416		/* 1/3 width */
> +#define EAST	3904		/* 2/3 width */
> +#define SOUTH	2480		/* 1/3 height */
> +#define NORTH	3424		/* 2/3 height */

Can't we calibrate this instead of having this hardcoded ? Or use sysfs 
attributes / module params ?

> +
> +static void navpoint_packet(void *dev)
> +{
> +	struct driver_data *drv_data;
> +	int pressed;
> +	unsigned x, y;
> +	unsigned code;
> +
> +	drv_data = dev;
> +
> +	switch (drv_data->data[0]) {
> +	case 0xff:	/* Garbage (packet?) between reset and Hello packet */
> +	case 0x00:	/* Module 0, NULL packet */
> +		break;
> +	case 0x0e:	/* Module 0, Absolute packet */
> +		pressed = (drv_data->data[1] & 0x01);
> +		if ((drv_data->pressed ^ pressed) == 0)	/* No change */
> +			break;
> +		drv_data->pressed = pressed;
> +		x = ((drv_data->data[2] & 0x1f) << 8) | drv_data->data[3];
> +		y = ((drv_data->data[4] & 0x1f) << 8) | drv_data->data[5];
> +		if (x < WEST)
> +			code = KEY_LEFT;
> +		else if (x > EAST)
> +			code = KEY_RIGHT;
> +		else if (y < SOUTH)
> +			code = KEY_DOWN;
> +		else if (y > NORTH)
> +			code = KEY_UP;
> +		else
> +			code = KEY_ENTER;
> +		if (pressed)
> +			drv_data->code = code;
> +		input_report_key(drv_data->input, drv_data->code, pressed);
> +		input_sync(drv_data->input);
> +		break;
> +	case 0x19:	/* Module 0, Hello packet */
> +		if ((drv_data->data[1] & 0xf0) == 0x10)
> +			break;
> +		/* FALLTHROUGH */
> +	default:
> +		dev_warn(dev, "spurious packet: 0x%02x,0x%02x,...\n",
> +			drv_data->data[0],
> +			drv_data->data[1]);
> +		break;
> +	}
> +
> +	drv_data->index = 0;
> +}
> +
> +static irqreturn_t navpoint_int(int irq, void *dev)
> +{
> +	struct driver_data *drv_data;
> +	struct ssp_device *ssp;
> +	u32 status;
> +	irqreturn_t ret;
> +
> +	drv_data = dev;
> +	ssp = drv_data->ssp;
> +
> +	status = pxa_ssp_read_reg(ssp, SSSR);
> +	ret = (status & (sssr | SSSR_RFS)) ? IRQ_HANDLED : IRQ_NONE;

Please avoid ternary ops.

> +
> +	if (status & sssr) {
> +		dev_warn(dev, "spurious interrupt: 0x%02x\n", status);
> +		pxa_ssp_write_reg(ssp, SSSR, (status & sssr));
> +	}
> +
> +	while (status & SSSR_RNE) {
> +		u32 data;
> +
> +		data = pxa_ssp_read_reg(ssp, SSDR);
> +		drv_data->data[drv_data->index + 0] = (data >> 8);
> +		drv_data->data[drv_data->index + 1] = data;
> +		drv_data->index += 2;
> +		if ((drv_data->data[0] & 0x07) < drv_data->index)

Maybe document magic numbers?

> +			navpoint_packet(dev);
> +		status = pxa_ssp_read_reg(ssp, SSSR);
> +	}
> +
> +	return ret;
> +}
> +
> +#ifdef CONFIG_SUSPEND
> +int navpoint_suspend(struct device *dev)
> +{
> +	struct driver_data *drv_data;
> +	struct ssp_device *ssp;
> +
> +	drv_data = dev_get_drvdata(dev);
> +	ssp = drv_data->ssp;
> +
> +	if (drv_data->gpio)
> +		gpio_set_value(drv_data->gpio, 0);
> +
> +	pxa_ssp_write_reg(ssp, SSCR0, 0);
> +
> +	clk_disable(ssp->clk);
> +
> +	return 0;
> +}
> +
> +int navpoint_resume(struct device *dev)
> +{
> +	struct driver_data *drv_data;
> +	struct ssp_device *ssp;
> +
> +	drv_data = dev_get_drvdata(dev);
> +	ssp = drv_data->ssp;
> +
> +	clk_enable(ssp->clk);
> +
> +	pxa_ssp_write_reg(ssp, SSCR1, sscr1);
> +	pxa_ssp_write_reg(ssp, SSSR, sssr);
> +	pxa_ssp_write_reg(ssp, SSTO, 0);
> +	pxa_ssp_write_reg(ssp, SSCR0, sscr0);	/* SSCR0_SSE written last */
> +
> +	if (drv_data->gpio)
> +		gpio_set_value(drv_data->gpio, 1);
> +
> +	/* Wait until SSP port is ready for slave clock operations */
> +	while (pxa_ssp_read_reg(ssp, SSSR) & SSSR_CSS)
> +		msleep(1);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops navpoint_pm_ops = {
> +	.suspend	= navpoint_suspend,
> +	.resume		= navpoint_resume,
> +};
> +#endif
> +
> +static int __devinit navpoint_probe(struct platform_device *pdev)
> +{
> +	struct navpoint_platform_data *pdata = pdev->dev.platform_data;
> +	int ret;
> +	struct driver_data *drv_data;
> +	struct ssp_device *ssp;
> +	struct input_dev *input;
> +
> +	drv_data = kzalloc(sizeof(struct driver_data), GFP_KERNEL);
> +	if (!drv_data) {
> +		ret = -ENOMEM;
> +		goto ret0;
> +	}
> +
> +	ssp = pxa_ssp_request(pdata->port, pdev->name);
> +	if (!ssp) {
> +		ret = -ENODEV;
> +		goto ret1;
> +	}
> +
> +	ret = request_irq(ssp->irq, navpoint_int, 0, pdev->name, drv_data);
> +	if (ret)
> +		goto ret2;
> +
> +	input = input_allocate_device();
> +	if (!input) {
> +		ret = -ENOMEM;
> +		goto ret3;
> +	}
> +	input->name = pdev->name;
> +	__set_bit(EV_KEY, input->evbit);
> +	__set_bit(KEY_ENTER, input->keybit);
> +	__set_bit(KEY_UP, input->keybit);
> +	__set_bit(KEY_LEFT, input->keybit);
> +	__set_bit(KEY_RIGHT, input->keybit);
> +	__set_bit(KEY_DOWN, input->keybit);
> +	input->dev.parent = &pdev->dev;
> +	ret = input_register_device(input);
> +	if (ret)
> +		goto ret4;
> +
> +	drv_data->ssp = ssp;
> +	drv_data->gpio = pdata->gpio;
> +	drv_data->input = input;
> +
> +	platform_set_drvdata(pdev, drv_data);
> +
> +	(void) navpoint_resume(&pdev->dev);
> +
> +	dev_info(&pdev->dev, "ssp%d, irq %d\n", pdata->port, ssp->irq);
> +
> +	return 0;
> +
> +ret4:
> +	input_free_device(input);
> +ret3:
> +	free_irq(ssp->irq, drv_data);
> +ret2:
> +	pxa_ssp_free(ssp);
> +ret1:
> +	kfree(drv_data);
> +ret0:
> +	return ret;
> +}
> +
> +static int __devexit navpoint_remove(struct platform_device *pdev)
> +{
> +	struct driver_data *drv_data;
> +	struct ssp_device *ssp;
> +	struct input_dev *input;
> +
> +	drv_data = platform_get_drvdata(pdev);
> +	ssp = drv_data->ssp;
> +	input = drv_data->input;
> +
> +	(void) navpoint_suspend(&pdev->dev);
> +
> +	input_unregister_device(input);
> +
> +	free_irq(ssp->irq, drv_data);
> +
> +	pxa_ssp_free(ssp);
> +
> +	kfree(drv_data);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver navpoint_driver = {
> +	.probe		= navpoint_probe,
> +	.remove		= __devexit_p(navpoint_remove),
> +	.driver = {
> +		.name	= "navpoint",
> +		.owner	= THIS_MODULE,
> +#ifdef CONFIG_SUSPEND
> +		.pm	= &navpoint_pm_ops,
> +#endif
> +	},
> +};
> +
> +static int __init navpoint_init(void)
> +{
> +	return platform_driver_register(&navpoint_driver);
> +}
> +
> +static void __exit navpoint_exit(void)
> +{
> +	platform_driver_unregister(&navpoint_driver);
> +}
> +
> +module_init(navpoint_init);
> +module_exit(navpoint_exit);
> +
> +MODULE_AUTHOR("Paul Parsons <lost.distance@yahoo.com>");
> +MODULE_DESCRIPTION("Synaptics NavPoint (PXA27x SSP/SPI) driver");
> +MODULE_LICENSE("GPL");

Isn't the original authors credit missing here and/or in the header?

> diff -uprN clean-3.0.1/include/linux/input/navpoint.h
> linux-3.0.1/include/linux/input/navpoint.h ---
> clean-3.0.1/include/linux/input/navpoint.h	1970-01-01 01:00:00.000000000
> +0100 +++ linux-3.0.1/include/linux/input/navpoint.h	2011-08-11
> 16:50:50.761188696 +0100 @@ -0,0 +1,12 @@
> +/*
> + *  Copyright (C) 2011 Paul Parsons <lost.distance@yahoo.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + */
> +
> +struct navpoint_platform_data {
> +	int		port;		/* PXA SSP port for pxa_ssp_request() */
> +	int		gpio;		/* GPIO for power on/off */
> +};

Cheers!

> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Paul Parsons]

> > MFP_CFG_OUT(GPIO102, ...) had already moved from the
> patch v1 platform file to patch v2 mfp-pxa27x.h because it
> was suggested that MFP macros should not be used directly.
> > Changing the direction in the platform file would
> surely require using the MFP macros again, so how to keep
> everyone happy? Maybe I should just define a more generic
> name such as GPIO102_GPIO_OUT?
> 
> No, you will not need to use the MFP macros, just
> gpio_direction_*() calls.
> Eric has already answered this and I agree with him.
> Use the MFP to configure the alternate function and other
> MFP related stuff
> and use the gpio_direction_*() calls later to set the
> direction of the GPIO.

OK, will do.

> > Because the underlying hardware is a touchpad
> controller and most of the touchpad drivers live in the
> mouse directory. I could have added a mouse interface to
> this driver but chose not to (at least for now) because the
> hx4700 platform already has a working touchscreen
> controller; a second mouse device was not needed. If future
> platforms require a mouse interface then one could be added
> relatively easily; surely this would be preferable to
> replicating the whole driver.
> 
> So the device is a full touchpad? and can be used for mouse
> pointer?

Yes, that's correct.

> Is the same device used as mouse pointer by some other
> driver?

Not as far as I can see. The word "NavPoint" appears only once in the current kernel source: within the hx4700 platform file. It's possible that other PDA models of similar 2004 vintage used the device, but I don't know whether any of them are supported in the kernel.

> > The suspend and resume functions check that the gpio
> is valid (which is taken to be non-zero) before using it.
> The platform file has already configured the gpio for
> output; it's the GPIO102 discussed earlier.
> 
> What I'm suggesting here is check that the gpio is valid
> here and only once
> and not in the suspend/resume functions. That gpio is not
> going to change,
> so there is no point is checking it each time in
> suspend/resume.
> Also the check for non-zero is not correct. 0 is a valid
> gpio number on most platforms.
> You need to use the gpio_is_valid() call for this.

I understand. My feeling was that other platforms wanting to use this driver would not necessarily use a gpio for power on/off; indeed the gpio field was originally a callback to provide full flexibility. Hence suspend / resume needed to use the gpio conditionally anyway. Nevertheless I'm happy to make the change and leave it for future platform developers to revisit suspend / resume as necessary.

Regards,
Paul

[PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Paul Parsons]

Hi Marek,

> obj-$(CONFIG_MOUSE_RISCPC)???
> ??? +=
> > rpcmouse.
> >? obj-$(CONFIG_MOUSE_SERIAL)???
> ??? += sermouse.o
> >?
> obj-$(CONFIG_MOUSE_SYNAPTICS_I2C)??? +=
> synaptics_i2c.o
> >? obj-$(CONFIG_MOUSE_VSXXXAA)???
> ??? += vsxxxaa.o
> > +obj-$(CONFIG_MOUSE_NAVPOINT_PXA27x)???
> += navpoint.o
> 
> Keep the list sorted

OK, will do.

> > +struct driver_data {
> > +??? struct ssp_device *ssp;
> > +??? int???
> ??? gpio;
> > +??? struct input_dev *input;
> > +??? int???
> ??? index;
> > +??? uint8_t???
> ??? data[8];
> > +??? int???
> ??? pressed;??? /* 1 =
> pressed, 0 = released */
> > +??? unsigned???
> code;??? ??? /* Key code of
> the last key pressed */
> > +};
> 
> Use tabs for alignment please.

OK, but in this case using tabs consistently would have meant breaking up lines to stay within the 80-column limit; neither solution was elegant.

> > +/*
> > + *??? MEP Query $22: Touchpad
> Coordinate Range Query is not supported by
> > + *??? the NavPoint module, so sampled
> values provide the N/S/E/W limits.
> > + */
> > +
> > +#define WEST??? 2416???
> ??? /* 1/3 width */
> > +#define EAST??? 3904???
> ??? /* 2/3 width */
> > +#define SOUTH???
> 2480??? ??? /* 1/3 height */
> > +#define NORTH???
> 3424??? ??? /* 2/3 height */
> 
> Can't we calibrate this instead of having this hardcoded ?

We could if the NavPoint supported MEP Query $22 (Touchpad Coordinate Range Query). Unfortunately it doesn't (I tried), presumably because the NavPoint uses an older version of MEP (Synaptics Modular Embedded Protocol).

> Or use sysfs 
> attributes / module params ?

Or platform data? That's much simpler. Without knowing how this driver will be used by other platforms (if at all) I'm wary about over-engineering it.

> > +??? ret = (status & (sssr |
> SSSR_RFS)) ? IRQ_HANDLED : IRQ_NONE;
> 
> Please avoid ternary ops.

OK, will do.

> > +??? ??? if
> ((drv_data->data[0] & 0x07) < drv_data->index)
> 
> Maybe document magic numbers?

OK, will do.

> > +MODULE_AUTHOR("Paul Parsons <lost.distance@yahoo.com>");
> > +MODULE_DESCRIPTION("Synaptics NavPoint (PXA27x
> SSP/SPI) driver");
> > +MODULE_LICENSE("GPL");
> 
> Isn't the original authors credit missing here and/or in
> the header?

Original authors? I don't understand; would you please elaborate.

Regards,
Paul

[PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Marek Vasut]

On Tuesday, August 16, 2011 04:21:18 PM Paul Parsons wrote:
> Hi Marek,
> 
> > obj-$(CONFIG_MOUSE_RISCPC)   
> >     +=
> > 
> > > rpcmouse.
> > >
> > >  obj-$(CONFIG_MOUSE_SERIAL)   
> > 
> >     += sermouse.o
> > 
> > > 
> > 
> > obj-$(CONFIG_MOUSE_SYNAPTICS_I2C)    +=
> > synaptics_i2c.o
> > 
> > >  obj-$(CONFIG_MOUSE_VSXXXAA)   
> > 
> >     += vsxxxaa.o
> > 
> > > +obj-$(CONFIG_MOUSE_NAVPOINT_PXA27x)   
> > 
> > += navpoint.o
> > 
> > Keep the list sorted
> 
> OK, will do.
> 
> > > +struct driver_data {
> > > +    struct ssp_device *ssp;
> > > +    int   
> > 
> >     gpio;
> > 
> > > +    struct input_dev *input;
> > > +    int   
> > 
> >     index;
> > 
> > > +    uint8_t   
> > 
> >     data[8];
> > 
> > > +    int   
> > 
> >     pressed;    /* 1 =
> > pressed, 0 = released */
> > 
> > > +    unsigned   
> > 
> > code;        /* Key code of
> > the last key pressed */
> > 
> > > +};
> > 
> > Use tabs for alignment please.
> 
> OK, but in this case using tabs consistently would have meant breaking up
> lines to stay within the 80-column limit; neither solution was elegant.

Just put tabs before variable names ... keep "struct blah" with space ;-)
> 
> > > +/*
> > > + *    MEP Query $22: Touchpad
> > 
> > Coordinate Range Query is not supported by
> > 
> > > + *    the NavPoint module, so sampled
> > 
> > values provide the N/S/E/W limits.
> > 
> > > + */
> > > +
> > > +#define WEST    2416   
> > 
> >     /* 1/3 width */
> > 
> > > +#define EAST    3904   
> > 
> >     /* 2/3 width */
> > 
> > > +#define SOUTH   
> > 
> > 2480        /* 1/3 height */
> > 
> > > +#define NORTH   
> > 
> > 3424        /* 2/3 height */
> > 
> > Can't we calibrate this instead of having this hardcoded ?
> 
> We could if the NavPoint supported MEP Query $22 (Touchpad Coordinate Range
> Query). Unfortunately it doesn't (I tried), presumably because the
> NavPoint uses an older version of MEP (Synaptics Modular Embedded
> Protocol).

Then you can adjust it via module parameters and have these as default values. 
or sysfs ... maybe sysfs would be better
> 
> > Or use sysfs
> > attributes / module params ?
> 
> Or platform data? That's much simpler. Without knowing how this driver will
> be used by other platforms (if at all) I'm wary about over-engineering it.

True ... then likely module params or sysfs to keep it simple. Pdata seems 
stupid.
> 
> > > +    ret = (status & (sssr |
> > 
> > SSSR_RFS)) ? IRQ_HANDLED : IRQ_NONE;
> > 
> > Please avoid ternary ops.
> 
> OK, will do.
> 
> > > +        if
> > 
> > ((drv_data->data[0] & 0x07) < drv_data->index)
> > 
> > Maybe document magic numbers?
> 
> OK, will do.
> 
> > > +MODULE_AUTHOR("Paul Parsons <lost.distance@yahoo.com>");
> > > +MODULE_DESCRIPTION("Synaptics NavPoint (PXA27x
> > 
> > SSP/SPI) driver");
> > 
> > > +MODULE_LICENSE("GPL");
> > 
> > Isn't the original authors credit missing here and/or in
> > the header?
> 
> Original authors? I don't understand; would you please elaborate.

I dunno, well you took it from linux-hh tree, right? So if there was any 
original author, give him a credit ... if that's you, just ignore this. I didn't 
expect there to be still someone from -hh times around and active :-)
> 
> Regards,
> Paul

[PATCH v2 2/2] pxa/hx4700: Add Synaptics NavPoint (PXA27x SSP/SPI) driver

[Paul Parsons]

> > We could if the NavPoint supported MEP Query $22
> (Touchpad Coordinate Range
> > Query). Unfortunately it doesn't (I tried), presumably
> because the
> > NavPoint uses an older version of MEP (Synaptics
> Modular Embedded
> > Protocol).
> 
> Then you can adjust it via module parameters and have these
> as default values. 
> or sysfs ... maybe sysfs would be better
> > 
> > > Or use sysfs
> > > attributes / module params ?
> > 
> > Or platform data? That's much simpler. Without knowing
> how this driver will
> > be used by other platforms (if at all) I'm wary about
> over-engineering it.
> 
> True ... then likely module params or sysfs to keep it
> simple. Pdata seems 
> stupid.

OK, module parameters are somewhat simpler so I'll go with those.

> > Original authors? I don't understand; would you please
> elaborate.
> 
> I dunno, well you took it from linux-hh tree, right? So if
> there was any 
> original author, give him a credit ... if that's you, just
> ignore this. I didn't 
> expect there to be still someone from -hh times around and
> active :-)

Actually it doesn't come from the hh kernel. I did reference the hh driver for SSP setup such as frame format and data size, since I have no NavPoint documentation, but this driver is new and very much simpler.

Regards,
Paul