Date:   Mon, 24 Apr 2017 10:41:25 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     AZO <typesylph@...il.com>
Cc:     linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers: input: joystick: Add PSX(Play Staion 1/2) pad
 with SPI driver Add PSX(Play Staion 1/2) pad with SPI driver. Pads can be
 connected directry SPI bus.

Hi,

Thank you for your submission.

On Mon, Apr 24, 2017 at 09:48:05AM +0900, AZO wrote:
> ---

We would need a better patch description and your signed-off-by,
please.

>  drivers/input/joystick/Kconfig      |   9 +
>  drivers/input/joystick/Makefile     |   1 +
>  drivers/input/joystick/psxpad-spi.c | 679 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 689 insertions(+)
>  create mode 100644 drivers/input/joystick/psxpad-spi.c
> 
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index 4215b5382092..bd36cbc43a34 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -330,4 +330,13 @@ config JOYSTICK_MAPLE
>  	  To compile this as a module choose M here: the module will be called
>  	  maplecontrol.
>  
> +config JOYSTICK_PSXPAD_SPI
> +	tristate "PSX(Play Station 1/2) pad with SPI Bus Driver"

Please add space before opening parenthesis here and elsewhere.

> +	depends on INPUT_POLLDEV && SPI
> +	help
> +	  Say Y here if you connect PSX(PS1/2) pad with SPI Interface.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called psxpad-spi.
> +
>  endif
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 92dc0de9dfed..2b71848cdc13 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -32,4 +32,5 @@ obj-$(CONFIG_JOYSTICK_WARRIOR)		+= warrior.o
>  obj-$(CONFIG_JOYSTICK_XPAD)		+= xpad.o
>  obj-$(CONFIG_JOYSTICK_ZHENHUA)		+= zhenhua.o
>  obj-$(CONFIG_JOYSTICK_WALKERA0701)	+= walkera0701.o
> +obj-$(CONFIG_JOYSTICK_PSXPAD_SPI)	+= psxpad-spi.o

Please insert into alphabetically-suitable place.

>  
> diff --git a/drivers/input/joystick/psxpad-spi.c b/drivers/input/joystick/psxpad-spi.c
> new file mode 100644
> index 000000000000..c5b497b2352e
> --- /dev/null
> +++ b/drivers/input/joystick/psxpad-spi.c
> @@ -0,0 +1,679 @@
> +/*
> + * PSX(Play Station 1/2) pad (SPI Interface)
> + *
> + * Copyright (C) 2017 AZO <typesylph@...il.com>
> + * Licensed under the GPL-2 or later.
> + *
> + * PSX pad plug (not socket)
> + *  123 456 789
> + * (...|...|...)
> + *
> + * 1: DAT -> MISO (pullup with 1k owm to 3.3V)
> + * 2: CMD -> MOSI
> + * 3: 9V (for motor, if not use N.C.)
> + * 4: GND
> + * 5: 3.3V
> + * 6: Attention -> CS(SS)
> + * 7: SCK -> SCK
> + * 8: N.C.
> + * 9: ACK -> N.C.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/input-polldev.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +
> +//#define PSXPAD_ENABLE_ANALOG2
> +#define PSXPAD_ENABLE_FF

FF should either be enabled unconditionally, or expressed via Kconfig
dependency.

> +
> +enum {
> +	PSXPAD_SPI_SPEED_125KHZ = 0,
> +	PSXPAD_SPI_SPEED_250KHZ,
> +	PSXPAD_SPI_SPEED_500KHZ,
> +	PSXPAD_SPI_SPEED_UNKNOWN
> +};
> +
> +#define PSXPAD_DEFAULT_SPI_DELAY	100
> +#define PSXPAD_DEFAULT_SPI_SPEED	PSXPAD_SPI_SPEED_125KHZ
> +#define PSXPAD_DEFAULT_INTERVAL		16
> +#define PSXPAD_DEFAULT_INTERVAL_MIN	8
> +#define PSXPAD_DEFAULT_INTERVAL_MAX	32
> +#define PSXPAD_DEFAULT_ADMODE		true
> +#define PSXPAD_DEFAULT_INPUT_PHYSIZE	32
> +
> +#define REVERSE_BIT(x) ((((x) & 0x80) >> 7) | (((x) & 0x40) >> 5) | (((x) & 0x20) >> 3) | (((x) & 0x10) >> 1) | (((x) & 0x08) << 1) | (((x) & 0x04) << 3) | (((x) & 0x02) << 5) | (((x) & 0x01) << 7))
> +
> +enum {
> +	PSXPAD_KEYSTATE_TYPE_DIGITAL = 0,
> +	PSXPAD_KEYSTATE_TYPE_ANALOG1,
> +#ifdef PSXPAD_ENABLE_ANALOG2
> +	PSXPAD_KEYSTATE_TYPE_ANALOG2,
> +#endif	/* PSXPAD_ENABLE_ANALOG2 */
> +	PSXPAD_KEYSTATE_TYPE_UNKNOWN
> +};
> +
> +#ifdef PSXPAD_ENABLE_ANALOG2
> +static const u8 PSX_CMD_INIT_PRESSURE[]	= {0x01, 0x40, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00};
> +static const u8 PSX_CMD_ALL_PRESSURE[]	= {0x01, 0x4F, 0x00, 0xFF, 0xFF, 0x03, 0x00, 0x00, 0x00};
> +#endif	/* PSXPAD_ENABLE_ANALOG2 */
> +static const u8 PSX_CMD_POLL[]		= {0x01, 0x42, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
> +static const u8 PSX_CMD_ENTER_CFG[]	= {0x01, 0x43, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00};
> +static const u8 PSX_CMD_EXIT_CFG[]	= {0x01, 0x43, 0x00, 0x00, 0x5A, 0x5A, 0x5A, 0x5A, 0x5A};
> +static const u8 PSX_CMD_ENABLE_MOTOR[]	= {0x01, 0x4D, 0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
> +static const u8 PSX_CMD_AD_MODE[]	= {0x01, 0x44, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00};
> +
> +struct psxpad_keystate {
> +	int type;
> +	/* PSXPAD_KEYSTATE_TYPE_DIGITAL */
> +	bool select;
> +	bool start;
> +	bool up;
> +	bool right;
> +	bool down;
> +	bool left;
> +	bool l2;
> +	bool r2;
> +	bool l1;
> +	bool r1;
> +	bool triangle;
> +	bool circle;
> +	bool cross;
> +	bool square;
> +	/* PSXPAD_KEYSTATE_TYPE_ANALOG1 */
> +	u8 l3;
> +	u8 r3;
> +	u8 lx;
> +	u8 ly;
> +	u8 rx;
> +	u8 ry;
> +#ifdef PSXPAD_ENABLE_ANALOG2
> +	/* PSXPAD_KEYSTATE_TYPE_ANALOG2 */
> +	u8 a_right;
> +	u8 a_left;
> +	u8 a_up;
> +	u8 a_down;
> +	u8 a_triangle;
> +	u8 a_circle;
> +	u8 a_cross;
> +	u8 a_square;
> +	u8 a_l1;
> +	u8 a_r1;
> +	u8 a_l2;
> +	u8 a_r2;
> +#endif	/* PSXPAD_ENABLE_ANALOG2 */
> +};
> +
> +struct psxpad {
> +	struct spi_device *spi;
> +	struct input_polled_dev *pdev;
> +	struct input_dev *idev;
> +	char phys[PSXPAD_DEFAULT_INPUT_PHYSIZE];
> +	u16 spi_delay;
> +	bool analog_mode;
> +	bool mode_lock;
> +	bool motor1enable;
> +	bool motor2enable;
> +	u8 motor1level;
> +	u8 motor2level;
> +
> +	/* for suspend/resume */
> +	bool sus_analog_mode;
> +	bool sus_mode_lock;
> +	bool sus_motor1enable;
> +	bool sus_motor2enable;
> +	u8 sus_motor1level;
> +	u8 sus_motor2level;
> +
> +	u8 spi_speed;
> +	u8 poolcmd[sizeof(PSX_CMD_POLL)];
> +	u8 response[sizeof(PSX_CMD_POLL)];
> +	u8 enablemotor[sizeof(PSX_CMD_ENABLE_MOTOR)];
> +	u8 admode[sizeof(PSX_CMD_AD_MODE)];
> +};
> +
> +static void psxpad_command(struct psxpad *pad, const u8 sendcmd[], u8 response[], const u8 sendcmdlen)
> +{
> +	struct spi_transfer *xfers;
> +	struct spi_message msg;
> +	u8 loc;
> +	u8 sendbuf[0x40];
> +
> +	if (!pad)
> +		return;
> +	if (!sendcmd)
> +		return;
> +	if (!response)
> +		return;
> +	if (sendcmdlen == 0)
> +		return;

This is not a general API exposed to random users in the kernel. We can
make sure we do not pass NULLs around, so there is no need to check for
them.

> +
> +	xfers = kzalloc(sizeof(struct spi_transfer), GFP_KERNEL);
> +	if (!xfers)
> +		return;

A single spi_transfer structure is small enough to be allocated on
stack, there is no need to allocate it from heap.

> +
> +	spi_message_init(&msg);
> +
> +	for (loc = 0; loc < sendcmdlen; loc++)
> +		sendbuf[loc] = REVERSE_BIT(sendcmd[loc]);
> +
> +	xfers->tx_buf		= sendbuf;
> +	xfers->rx_buf		= response;
> +	xfers->len		= sendcmdlen;
> +	xfers->bits_per_word	= 8;
> +	xfers->delay_usecs	= pad->spi_delay;
> +	switch (pad->spi_speed) {
> +	case PSXPAD_SPI_SPEED_250KHZ:
> +		xfers->speed_hz = 250000;
> +		break;
> +	case PSXPAD_SPI_SPEED_500KHZ:
> +		xfers->speed_hz = 500000;
> +		break;
> +	default:
> +		xfers->speed_hz = 125000;
> +		break;
> +	}

Initializing bits_per_word, delay_usecs and speed_hz by hand is normally
not needed.

> +	spi_message_add_tail(xfers, &msg);
> +	spi_sync(pad->spi, &msg);

I'd use "spi_sync_transfer(pad->spi, &msg, 1);"

> +	kfree(xfers);
> +
> +	for (loc = 0; loc < sendcmdlen; loc++)
> +		response[loc] = REVERSE_BIT(response[loc]);
> +}
> +
> +static void psxpad_setadmode(struct psxpad *pad, const bool analog_mode, const bool mode_lock)
> +{
> +	if (!pad)
> +		return;
> +
> +	pad->analog_mode	= analog_mode;
> +	pad->mode_lock	= mode_lock;
> +
> +	pad->admode[3] = pad->analog_mode	? 0x01 : 0x00;
> +	pad->admode[4] = pad->mode_lock		? 0x03 : 0x00;
> +
> +	psxpad_command(pad, PSX_CMD_ENTER_CFG, pad->response, sizeof(PSX_CMD_ENTER_CFG));
> +	psxpad_command(pad, pad->admode, pad->response, sizeof(PSX_CMD_AD_MODE));
> +#ifdef PSXPAD_ENABLE_ANALOG2
> +	psxpad_command(pad, PSX_CMD_INIT_PRESSURE, pad->response, sizeof(PSX_CMD_INIT_PRESSURE));
> +	psxpad_command(pad, PSX_CMD_ALL_PRESSURE, pad->response, sizeof(PSX_CMD_ALL_PRESSURE));
> +#endif	/* PSXPAD_ENABLE_ANALOG2 */
> +	psxpad_command(pad, PSX_CMD_EXIT_CFG, pad->response, sizeof(PSX_CMD_EXIT_CFG));
> +}
> +
> +#ifdef PSXPAD_ENABLE_FF
> +static void psxpad_setenablemotor(struct psxpad *pad, const bool motor1enable, const bool motor2enable)
> +{
> +	if (!pad)
> +		return;
> +
> +	pad->motor1enable = motor1enable;
> +	pad->motor2enable = motor2enable;
> +
> +	pad->enablemotor[3] = pad->motor1enable ? 0x00 : 0xFF;
> +	pad->enablemotor[4] = pad->motor2enable ? 0x01 : 0xFF;
> +
> +	psxpad_command(pad, PSX_CMD_ENTER_CFG, pad->response, sizeof(PSX_CMD_ENTER_CFG));
> +	psxpad_command(pad, pad->enablemotor, pad->response, sizeof(PSX_CMD_ENABLE_MOTOR));
> +	psxpad_command(pad, PSX_CMD_EXIT_CFG, pad->response, sizeof(PSX_CMD_EXIT_CFG));
> +}
> +
> +static void psxpad_setmotorlevel(struct psxpad *pad, const u8 motor1level, const u8 motor2level)
> +{
> +	if (!pad)
> +		return;
> +
> +	pad->motor1level = motor1level ? 0xFF : 0x00;
> +	pad->motor2level = motor2level;
> +
> +	pad->poolcmd[3] = pad->motor1level;
> +	pad->poolcmd[4] = pad->motor2level;
> +}
> +#endif	/* PSXPAD_ENABLE_FF */
> +
> +static void psxpad_getkeystate(struct psxpad *pad, struct psxpad_keystate *keystate)
> +{
> +	if (!pad)
> +		return;
> +	if (!keystate)
> +		return;
> +
> +	keystate->type = PSXPAD_KEYSTATE_TYPE_UNKNOWN;
> +#ifdef PSXPAD_ENABLE_ANALOG2
> +	keystate->a_right	= 0;
> +	keystate->a_left	= 0;
> +	keystate->a_up		= 0;
> +	keystate->a_down	= 0;
> +	keystate->a_triangle	= 0;
> +	keystate->a_circle	= 0;
> +	keystate->a_cross	= 0;
> +	keystate->a_square	= 0;
> +	keystate->a_l1		= 0;
> +	keystate->a_r1		= 0;
> +	keystate->a_l2		= 0;
> +	keystate->a_r2		= 0;
> +#endif	/* PSXPAD_ENABLE_ANALOG2 */
> +	keystate->rx		= 0x80;
> +	keystate->ry		= 0x80;
> +	keystate->lx		= 0x80;
> +	keystate->ly		= 0x80;
> +	keystate->l3		= false;
> +	keystate->r3		= false;
> +	keystate->select	= false;
> +	keystate->start		= false;
> +	keystate->up		= false;
> +	keystate->right		= false;
> +	keystate->down		= false;
> +	keystate->left		= false;
> +	keystate->l2		= false;
> +	keystate->r2		= false;
> +	keystate->l1		= false;
> +	keystate->r1		= false;
> +	keystate->triangle	= false;
> +	keystate->circle	= false;
> +	keystate->cross		= false;
> +	keystate->square	= false;
> +
> +	switch (pad->response[1]) {
> +#ifdef PSXPAD_ENABLE_ANALOG2
> +	case 0x79:
> +		keystate->type = PSXPAD_KEYSTATE_TYPE_ANALOG2;
> +		keystate->a_right	= pad->response[9];
> +		keystate->a_left	= pad->response[10];
> +		keystate->a_up		= pad->response[11];
> +		keystate->a_down	= pad->response[12];
> +		keystate->a_triangle	= pad->response[13];
> +		keystate->a_circle	= pad->response[14];
> +		keystate->a_cross	= pad->response[15];
> +		keystate->a_square	= pad->response[16];
> +		keystate->a_l1		= pad->response[17];
> +		keystate->a_r1		= pad->response[18];
> +		keystate->a_l2		= pad->response[19];
> +		keystate->a_r2		= pad->response[20];
> +#endif	/* PSXPAD_ENABLE_ANALOG2 */
> +	case 0x73:
> +		if (keystate->type == PSXPAD_KEYSTATE_TYPE_UNKNOWN)
> +			keystate->type = PSXPAD_KEYSTATE_TYPE_ANALOG1;
> +		keystate->rx = pad->response[5];
> +		keystate->ry = pad->response[6];
> +		keystate->lx = pad->response[7];
> +		keystate->ly = pad->response[8];
> +		keystate->l3 = (pad->response[3] & 0x02U) ? false : true;
> +		keystate->r3 = (pad->response[3] & 0x04U) ? false : true;
> +	case 0x41:
> +		if (keystate->type == PSXPAD_KEYSTATE_TYPE_UNKNOWN)
> +			keystate->type = PSXPAD_KEYSTATE_TYPE_DIGITAL;
> +		keystate->select	= (pad->response[3] & 0x01U) ? false : true;
> +		keystate->start		= (pad->response[3] & 0x08U) ? false : true;
> +		keystate->up		= (pad->response[3] & 0x10U) ? false : true;
> +		keystate->right		= (pad->response[3] & 0x20U) ? false : true;
> +		keystate->down		= (pad->response[3] & 0x40U) ? false : true;
> +		keystate->left		= (pad->response[3] & 0x80U) ? false : true;
> +		keystate->l2		= (pad->response[4] & 0x01U) ? false : true;
> +		keystate->r2		= (pad->response[4] & 0x02U) ? false : true;
> +		keystate->l1		= (pad->response[4] & 0x04U) ? false : true;
> +		keystate->r1		= (pad->response[4] & 0x08U) ? false : true;
> +		keystate->triangle	= (pad->response[4] & 0x10U) ? false : true;
> +		keystate->circle	= (pad->response[4] & 0x20U) ? false : true;
> +		keystate->cross		= (pad->response[4] & 0x40U) ? false : true;
> +		keystate->square	= (pad->response[4] & 0x80U) ? false : true;
> +	}
> +}
> +
> +static void psxpad_spi_poll_open(struct input_polled_dev *pdev)
> +{
> +	struct psxpad *pad = pdev->private;
> +
> +	pm_runtime_get_sync(&pad->spi->dev);
> +}
> +
> +static void psxpad_spi_poll_close(struct input_polled_dev *pdev)
> +{
> +	struct psxpad *pad = pdev->private;
> +
> +	pm_runtime_put_sync(&pad->spi->dev);
> +}
> +
> +static void psxpad_spi_poll(struct input_polled_dev *pdev)
> +{
> +	struct psxpad *pad = pdev->private;
> +	struct psxpad_keystate keystate;
> +
> +	psxpad_command(pad, pad->poolcmd, pad->response, sizeof(PSX_CMD_POLL));
> +	psxpad_getkeystate(pad, &keystate);
> +#ifdef PSXPAD_ENABLE_FF
> +	psxpad_setenablemotor(pad, true, true);
> +#endif	/* PSXPAD_ENABLE_FF */
> +
> +	switch (keystate.type) {
> +#ifdef PSXPAD_ENABLE_ANALOG2
> +	case PSXPAD_KEYSTATE_TYPE_ANALOG2:
> +		input_report_abs(pad->idev, ABS_PRESSURE,	keystate.a_up);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  1,	keystate.a_down);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  2,	keystate.a_left);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  3,	keystate.a_right);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  4,	keystate.a_triangle);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  5,	keystate.a_circle);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  6,	keystate.a_cross);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  7,	keystate.a_square);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  8,	keystate.a_l1);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  9,	keystate.a_r1);
> +		input_report_abs(pad->idev, ABS_PRESSURE + 10,	keystate.a_l2);
> +		input_report_abs(pad->idev, ABS_PRESSURE + 11,	keystate.a_r2);
> +		input_report_abs(pad->idev, ABS_X,		keystate.lx);
> +		input_report_abs(pad->idev, ABS_Y,		keystate.ly);
> +		input_report_abs(pad->idev, ABS_RX,		keystate.rx);
> +		input_report_abs(pad->idev, ABS_RY,		keystate.ry);
> +		input_report_key(pad->idev, BTN_DPAD_UP,	false);
> +		input_report_key(pad->idev, BTN_DPAD_DOWN,	false);
> +		input_report_key(pad->idev, BTN_DPAD_LEFT,	false);
> +		input_report_key(pad->idev, BTN_DPAD_RIGHT,	false);
> +		input_report_key(pad->idev, BTN_X,		false);
> +		input_report_key(pad->idev, BTN_A,		false);
> +		input_report_key(pad->idev, BTN_B,		false);
> +		input_report_key(pad->idev, BTN_Y,		false);
> +		input_report_key(pad->idev, BTN_TL,		false);
> +		input_report_key(pad->idev, BTN_TR,		false);
> +		input_report_key(pad->idev, BTN_TL2,		false);
> +		input_report_key(pad->idev, BTN_TR2,		false);
> +		input_report_key(pad->idev, BTN_THUMBL,		keystate.l3);
> +		input_report_key(pad->idev, BTN_THUMBR,		keystate.r3);
> +		input_report_key(pad->idev, BTN_SELECT,		keystate.select);
> +		input_report_key(pad->idev, BTN_START,		keystate.start);
> +		break;
> +#endif	/* PSXPAD_ENABLE_ANALOG2 */
> +
> +	case PSXPAD_KEYSTATE_TYPE_ANALOG1:
> +		input_report_abs(pad->idev, ABS_PRESSURE,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  1,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  2,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  3,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  4,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  5,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  6,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  7,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  8,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  9,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE + 10,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE + 11,	0);
> +		input_report_abs(pad->idev, ABS_X,		keystate.lx);
> +		input_report_abs(pad->idev, ABS_Y,		keystate.ly);
> +		input_report_abs(pad->idev, ABS_RX,		keystate.rx);
> +		input_report_abs(pad->idev, ABS_RY,		keystate.ry);
> +		input_report_key(pad->idev, BTN_DPAD_UP,	keystate.up);
> +		input_report_key(pad->idev, BTN_DPAD_DOWN,	keystate.down);
> +		input_report_key(pad->idev, BTN_DPAD_LEFT,	keystate.left);
> +		input_report_key(pad->idev, BTN_DPAD_RIGHT,	keystate.right);
> +		input_report_key(pad->idev, BTN_X,		keystate.triangle);
> +		input_report_key(pad->idev, BTN_A,		keystate.circle);
> +		input_report_key(pad->idev, BTN_B,		keystate.cross);
> +		input_report_key(pad->idev, BTN_Y,		keystate.square);
> +		input_report_key(pad->idev, BTN_TL,		keystate.l1);
> +		input_report_key(pad->idev, BTN_TR,		keystate.r1);
> +		input_report_key(pad->idev, BTN_TL2,		keystate.l2);
> +		input_report_key(pad->idev, BTN_TR2,		keystate.r2);
> +		input_report_key(pad->idev, BTN_THUMBL,		keystate.l3);
> +		input_report_key(pad->idev, BTN_THUMBR,		keystate.r3);
> +		input_report_key(pad->idev, BTN_SELECT,		keystate.select);
> +		input_report_key(pad->idev, BTN_START,		keystate.start);
> +		break;
> +
> +	case PSXPAD_KEYSTATE_TYPE_DIGITAL:
> +		input_report_abs(pad->idev, ABS_PRESSURE,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  1,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  2,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  3,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  4,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  5,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  6,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  7,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  8,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE +  9,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE + 10,	0);
> +		input_report_abs(pad->idev, ABS_PRESSURE + 11,	0);
> +		input_report_abs(pad->idev, ABS_X,		0x80);
> +		input_report_abs(pad->idev, ABS_Y,		0x80);
> +		input_report_abs(pad->idev, ABS_RX,		0x80);
> +		input_report_abs(pad->idev, ABS_RY,		0x80);
> +		input_report_key(pad->idev, BTN_DPAD_UP,	keystate.up);
> +		input_report_key(pad->idev, BTN_DPAD_DOWN,	keystate.down);
> +		input_report_key(pad->idev, BTN_DPAD_LEFT,	keystate.left);
> +		input_report_key(pad->idev, BTN_DPAD_RIGHT,	keystate.right);
> +		input_report_key(pad->idev, BTN_X,		keystate.triangle);
> +		input_report_key(pad->idev, BTN_A,		keystate.circle);
> +		input_report_key(pad->idev, BTN_B,		keystate.cross);
> +		input_report_key(pad->idev, BTN_Y,		keystate.square);
> +		input_report_key(pad->idev, BTN_TL,		keystate.l1);
> +		input_report_key(pad->idev, BTN_TR,		keystate.r1);
> +		input_report_key(pad->idev, BTN_TL2,		keystate.l2);
> +		input_report_key(pad->idev, BTN_TR2,		keystate.r2);
> +		input_report_key(pad->idev, BTN_THUMBL,		false);
> +		input_report_key(pad->idev, BTN_THUMBR,		false);
> +		input_report_key(pad->idev, BTN_SELECT,		keystate.select);
> +		input_report_key(pad->idev, BTN_START,		keystate.start);
> +		break;
> +	}
> +
> +	input_sync(pad->idev);
> +}
> +
> +#ifdef PSXPAD_ENABLE_FF
> +static int psxpad_spi_ff(struct input_dev *idev, void *data, struct ff_effect *effect)
> +{
> +	struct psxpad *pad = idev->dev.platform_data;
> +
> +	switch (effect->type) {
> +	case FF_RUMBLE:
> +		psxpad_setmotorlevel(pad, (effect->u.rumble.weak_magnitude >> 8) & 0xFFU, (effect->u.rumble.strong_magnitude >> 8) & 0xFFU);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +#endif	/* PSXPAD_ENABLE_FF */
> +
> +static int psxpad_spi_probe(struct spi_device *spi)
> +{
> +	struct psxpad *pad = NULL;
> +	struct input_polled_dev *pdev = NULL;
> +	struct input_dev *idev;
> +	int err, i;
> +
> +	pad = kzalloc(sizeof(struct psxpad), GFP_KERNEL);

Consider using devm.

> +	pdev = input_allocate_polled_device();

Does it have to be polled? Can we hook up interrupt line?

> +	if (!pad || !pdev) {
> +		pr_err("psxpad-spi: alloc failed!!\n");
> +		err = -ENOMEM;
> +		goto err_free_mem;
> +	}
> +	pdev->input->ff = NULL;
> +	for (i = 0; i < sizeof(PSX_CMD_POLL); i++)
> +		pad->poolcmd[i] = PSX_CMD_POLL[i];
> +	for (i = 0; i < sizeof(PSX_CMD_ENABLE_MOTOR); i++)
> +		pad->enablemotor[i] = PSX_CMD_ENABLE_MOTOR[i];
> +	for (i = 0; i < sizeof(PSX_CMD_AD_MODE); i++)
> +		pad->admode[i] = PSX_CMD_AD_MODE[i];
> +	pad->spi_delay = PSXPAD_DEFAULT_SPI_DELAY;
> +	pad->spi_speed = PSXPAD_DEFAULT_SPI_SPEED;
> +
> +	/* input pool device settings */
> +	pad->pdev = pdev;
> +	pad->spi = spi;
> +	pdev->private = pad;
> +	pdev->open = psxpad_spi_poll_open;
> +	pdev->close = psxpad_spi_poll_close;
> +	pdev->poll = psxpad_spi_poll;
> +	pdev->poll_interval = PSXPAD_DEFAULT_INTERVAL;
> +	pdev->poll_interval_min = PSXPAD_DEFAULT_INTERVAL_MIN;
> +	pdev->poll_interval_max = PSXPAD_DEFAULT_INTERVAL_MAX;
> +
> +	/* input device settings */
> +	idev = pdev->input;
> +	pad->idev = idev;
> +	idev->name = "PSX(PS1/2) pad";
> +	snprintf(pad->phys, PSXPAD_DEFAULT_INPUT_PHYSIZE, "%s/input", dev_name(&spi->dev));
> +	idev->id.bustype = BUS_SPI;
> +	idev->dev.parent = &spi->dev;
> +	idev->dev.platform_data = pad;
> +
> +	/* key/value map settings */
> +	__set_bit(EV_ABS, idev->evbit);
> +	input_set_abs_params(idev, ABS_X,		0, 255, 0, 0);
> +	input_set_abs_params(idev, ABS_Y,		0, 255, 0, 0);
> +	input_set_abs_params(idev, ABS_RX,		0, 255, 0, 0);
> +	input_set_abs_params(idev, ABS_RY,		0, 255, 0, 0);
> +#ifdef PSXPAD_ENABLE_ANALOG2
> +	input_set_abs_params(idev, ABS_PRESSURE,	0, 255, 0, 0);	/* up */
> +	input_set_abs_params(idev, ABS_PRESSURE +  1,	0, 255, 0, 0);	/* down */
> +	input_set_abs_params(idev, ABS_PRESSURE +  2,	0, 255, 0, 0);	/* left */
> +	input_set_abs_params(idev, ABS_PRESSURE +  3,	0, 255, 0, 0);	/* right */
> +	input_set_abs_params(idev, ABS_PRESSURE +  4,	0, 255, 0, 0);
> +	input_set_abs_params(idev, ABS_PRESSURE +  5,	0, 255, 0, 0);
> +	input_set_abs_params(idev, ABS_PRESSURE +  6,	0, 255, 0, 0);
> +	input_set_abs_params(idev, ABS_PRESSURE +  7,	0, 255, 0, 0);
> +	input_set_abs_params(idev, ABS_PRESSURE +  8,	0, 255, 0, 0);	/* L1 */
> +	input_set_abs_params(idev, ABS_PRESSURE +  9,	0, 255, 0, 0);	/* R1 */
> +	input_set_abs_params(idev, ABS_PRESSURE + 10,	0, 255, 0, 0);	/* L2 */
> +	input_set_abs_params(idev, ABS_PRESSURE + 11,	0, 255, 0, 0);	/* R2 */

No, we can't do this. Nobody would know what to do with these events.

Please make sure the mapping matches what is described in
Documentation/input/gamepad.txt

> +#endif	/* PSXPAD_ENABLE_ANALOG2 */
> +	__set_bit(EV_KEY, idev->evbit);
> +	__set_bit(BTN_DPAD_UP,		idev->keybit);
> +	__set_bit(BTN_DPAD_DOWN,	idev->keybit);
> +	__set_bit(BTN_DPAD_LEFT,	idev->keybit);
> +	__set_bit(BTN_DPAD_RIGHT,	idev->keybit);
> +	__set_bit(BTN_A,		idev->keybit);
> +	__set_bit(BTN_B,		idev->keybit);
> +	__set_bit(BTN_X,		idev->keybit);
> +	__set_bit(BTN_Y,		idev->keybit);
> +	__set_bit(BTN_TL,		idev->keybit);
> +	__set_bit(BTN_TR,		idev->keybit);
> +	__set_bit(BTN_TL2,		idev->keybit);
> +	__set_bit(BTN_TR2,		idev->keybit);
> +	__set_bit(BTN_THUMBL,		idev->keybit);
> +	__set_bit(BTN_THUMBR,		idev->keybit);
> +	__set_bit(BTN_SELECT,		idev->keybit);
> +	__set_bit(BTN_START,		idev->keybit);
> +
> +#ifdef PSXPAD_ENABLE_FF
> +	/* force feedback */
> +	input_set_capability(idev, EV_FF, FF_RUMBLE);
> +	err = input_ff_create_memless(idev, NULL, psxpad_spi_ff);
> +	if (err) {
> +		pr_err("psxpad-spi: ff alloc failed!!\n");
> +		err = -ENOMEM;
> +		goto err_free_mem;
> +	}
> +#endif	/* PSXPAD_ENABLE_FF */

The normal kernel practice is not to drop #ifdef in the middle of a
function, but rather provide stubs for various operations:

#ifdef PSXPAD_ENABLE_FF
static int psxpar_init_ff(struct psxpad *pad)
{
	int error;

	input_set_capability(idev, EV_FF, FF_RUMBLE);
	error = input_ff_create_memless(...);
	...
}

static void psxpar_deinit_ff(struct psxpad *pad)
{
...
}
...

#else
static inline int psxpar_init_ff(struct psxpad *pad)
{
	return 0;
}

static void psxpar_deinit_ff(struct psxpad *pad) { }
...
#endif

> +
> +	/* SPI settings */
> +	spi->mode = SPI_MODE_3;
> +	spi->bits_per_word = 8;
> +	spi_setup(spi);
> +
> +	/* pad settings */
> +#ifdef PSXPAD_ENABLE_FF
> +	psxpad_setmotorlevel(pad, 0, 0);
> +#endif	/* PSXPAD_ENABLE_FF */
> +
> +	/* register input pool device */
> +	err = input_register_polled_device(pdev);
> +	if (err) {
> +		pr_err("psxpad-spi: failed register!!\n");
> +		input_free_polled_device(pdev);
> +		goto err_free_mem;
> +	}
> +
> +	pm_runtime_enable(&spi->dev);
> +
> +	return 0;
> +
> + err_free_mem:
> +	if (pdev) {
> +#ifdef PSXPAD_ENABLE_FF
> +		if (pdev->input->ff)
> +			input_ff_destroy(pdev->input);
> +#endif	/* PSXPAD_ENABLE_FF */
> +		input_free_polled_device(pdev);
> +	}
> +	kfree(pad);
> +
> +	return err;
> +}
> +
> +
> +static int psxpad_spi_remove(struct spi_device *spi)
> +{
> +	struct psxpad *pad = spi_get_drvdata(spi);
> +
> +#ifdef PSXPAD_ENABLE_FF
> +	input_ff_destroy(pad->idev);
> +#endif	/* PSXPAD_ENABLE_FF */
> +	input_free_polled_device(pad->pdev);
> +	kfree(pad);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused psxpad_spi_suspend(struct device *dev)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct psxpad *pad = spi_get_drvdata(spi);
> +
> +	pad->sus_analog_mode = pad->analog_mode;
> +	pad->sus_mode_lock = pad->mode_lock;
> +	pad->sus_motor1enable = pad->motor1enable;
> +	pad->sus_motor2enable = pad->motor2enable;
> +	pad->sus_motor1level = pad->motor1level;
> +	pad->sus_motor2level = pad->motor2level;
> +
> +	psxpad_setadmode(pad, false, false);
> +#ifdef PSXPAD_ENABLE_FF
> +	psxpad_setmotorlevel(pad, 0, 0);
> +	psxpad_setenablemotor(pad, false, false);
> +#endif	/* PSXPAD_ENABLE_FF */
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused psxpad_spi_resume(struct device *dev)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct psxpad *pad = spi_get_drvdata(spi);
> +
> +	spi->mode = SPI_MODE_3;
> +	psxpad_setadmode(pad, pad->sus_analog_mode, pad->sus_mode_lock);
> +#ifdef PSXPAD_ENABLE_FF
> +	psxpad_setmotorlevel(pad, pad->sus_motor1enable, pad->sus_motor2enable);
> +	psxpad_setenablemotor(pad, pad->sus_motor1level, pad->sus_motor2level);
> +#endif	/* PSXPAD_ENABLE_FF */
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(psxpad_spi_pm, psxpad_spi_suspend, psxpad_spi_resume);
> +
> +static const struct spi_device_id psxpad_spi_id[] = {
> +	{ "psxpad-spi", 0 },

"sony,psxpad-spi" maybe?

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, psxpad_spi_id);
> +
> +static struct spi_driver psxpad_spi_driver = {
> +	.driver = {
> +		.name = "psxpad-spi",
> +		.pm = &psxpad_spi_pm,
> +	},
> +	.id_table = psxpad_spi_id,
> +	.probe   = psxpad_spi_probe,
> +	.remove  = psxpad_spi_remove,
> +};
> +
> +module_spi_driver(psxpad_spi_driver);
> +
> +MODULE_AUTHOR("AZO <typesylph@...il.com>");
> +MODULE_DESCRIPTION("PSX(Play Station 1/2) pad with SPI Bus Driver");
> +MODULE_LICENSE("GPL");


Thanks.

-- 
Dmitry

Powered by blists - more mailing lists