LinuxLists.cc - [PATCH] drivers: input: joystick: Add PSX (Play Station 1/2) pad with SPI driver.

2017-04-25 14:44:46

Subject: [PATCH] drivers: input: joystick: Add PSX (Play Station 1/2) pad with SPI driver.

PSX pads can be connected directry SPI bus.

Signed-off-by: AZO <[email protected]>
---
drivers/input/joystick/Kconfig | 16 +
drivers/input/joystick/Makefile | 1 +
drivers/input/joystick/psxpad-spi.c | 671 ++++++++++++++++++++++++++++++++++++
3 files changed, 688 insertions(+)
create mode 100644 drivers/input/joystick/psxpad-spi.c

diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index 4215b538..f49645f7 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -330,4 +330,20 @@ 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"
+ 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.
+
+config JOYSTICK_PSXPAD_SPI_FF
+ bool "PSX pad with SPI Bus rumble support"
+ depends on JOYSTICK_PSXPAD_SPI && INPUT
+ select INPUT_FF_MEMLESS
+ help
+ Say Y here if you want to take advantage of PSX pad rumble features.
+
endif
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 92dc0de9..496fd56b 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_JOYSTICK_INTERACT) += interact.o
obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
+obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o
diff --git a/drivers/input/joystick/psxpad-spi.c b/drivers/input/joystick/psxpad-spi.c
new file mode 100644
index 00000000..3d2bee6d
--- /dev/null
+++ b/drivers/input/joystick/psxpad-spi.c
@@ -0,0 +1,671 @@
+/*
+ * PSX (Play Station 1/2) pad (SPI Interface)
+ *
+ * Copyright (C) 2017 AZO <[email protected]>
+ * 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/device.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
+#ifdef CONFIG_JOYSTICK_PSXPAD_SPI_FF
+#define PSXPAD_ENABLE_FF
+#endif /* CONFIG_JOYSTICK_PSXPAD_SPI_FF */
+
+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,
+ PSXPAD_KEYSTATE_TYPE_ANALOG2,
+ PSXPAD_KEYSTATE_TYPE_UNKNOWN
+};
+
+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};
+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];
+
+ xfers = kzalloc(sizeof(struct spi_transfer), GFP_KERNEL);
+ if (!xfers)
+ return;
+
+ 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;
+ }
+ spi_sync_transfer(pad->spi, xfers, 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)
+{
+ 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)
+{
+ 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)
+{
+ pad->motor1level = motor1level ? 0xFF : 0x00;
+ pad->motor2level = motor2level;
+
+ pad->poolcmd[3] = pad->motor1level;
+ pad->poolcmd[4] = pad->motor2level;
+}
+#else /* PSXPAD_ENABLE_FF */
+static void psxpad_setenablemotor(struct psxpad *pad, const bool motor1enable, const bool motor2enable) { }
+
+static void psxpad_setmotorlevel(struct psxpad *pad, const u8 motor1level, const u8 motor2level) { }
+#endif /* PSXPAD_ENABLE_FF */
+
+static void psxpad_getkeystate(struct psxpad *pad, struct psxpad_keystate *keystate)
+{
+ 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);
+ psxpad_setenablemotor(pad, true, true);
+
+ switch (keystate.type) {
+#ifdef PSXPAD_ENABLE_ANALOG2
+ case PSXPAD_KEYSTATE_TYPE_ANALOG2:
+ input_report_abs(pad->idev, ABS_HAT0Y, keystate.a_up);
+ input_report_abs(pad->idev, ABS_HAT1Y, keystate.a_down);
+ input_report_abs(pad->idev, ABS_HAT0X, keystate.a_left);
+ input_report_abs(pad->idev, ABS_HAT1X, keystate.a_right);
+ input_report_abs(pad->idev, ABS_MISC, keystate.a_triangle);
+ input_report_abs(pad->idev, ABS_PRESSURE, keystate.a_circle);
+ input_report_abs(pad->idev, ABS_BRAKE, keystate.a_cross);
+ input_report_abs(pad->idev, ABS_THROTTLE, keystate.a_square);
+ input_report_abs(pad->idev, ABS_HAT2X, keystate.a_l1);
+ input_report_abs(pad->idev, ABS_HAT3X, keystate.a_r1);
+ input_report_abs(pad->idev, ABS_HAT2Y, keystate.a_l2);
+ input_report_abs(pad->idev, ABS_HAT3Y, 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:
+#ifdef PSXPAD_ENABLE_ANALOG2
+ input_report_abs(pad->idev, ABS_HAT0Y, 0);
+ input_report_abs(pad->idev, ABS_HAT1Y, 0);
+ input_report_abs(pad->idev, ABS_HAT0X, 0);
+ input_report_abs(pad->idev, ABS_HAT1X, 0);
+ input_report_abs(pad->idev, ABS_MISC, 0);
+ input_report_abs(pad->idev, ABS_PRESSURE, 0);
+ input_report_abs(pad->idev, ABS_BRAKE, 0);
+ input_report_abs(pad->idev, ABS_THROTTLE, 0);
+ input_report_abs(pad->idev, ABS_HAT2X, 0);
+ input_report_abs(pad->idev, ABS_HAT3X, 0);
+ input_report_abs(pad->idev, ABS_HAT2Y, 0);
+ input_report_abs(pad->idev, ABS_HAT3Y, 0);
+#endif /* PSXPAD_ENABLE_ANALOG2 */
+ 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:
+#ifdef PSXPAD_ENABLE_ANALOG2
+ input_report_abs(pad->idev, ABS_HAT0Y, 0);
+ input_report_abs(pad->idev, ABS_HAT1Y, 0);
+ input_report_abs(pad->idev, ABS_HAT0X, 0);
+ input_report_abs(pad->idev, ABS_HAT1X, 0);
+ input_report_abs(pad->idev, ABS_MISC, 0);
+ input_report_abs(pad->idev, ABS_PRESSURE, 0);
+ input_report_abs(pad->idev, ABS_BRAKE, 0);
+ input_report_abs(pad->idev, ABS_THROTTLE, 0);
+ input_report_abs(pad->idev, ABS_HAT2X, 0);
+ input_report_abs(pad->idev, ABS_HAT3X, 0);
+ input_report_abs(pad->idev, ABS_HAT2Y, 0);
+ input_report_abs(pad->idev, ABS_HAT3Y, 0);
+#endif /* PSXPAD_ENABLE_ANALOG2 */
+ 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;
+}
+
+static int psxpad_spi_init_ff(struct psxpad *pad)
+{
+ int err;
+
+ input_set_capability(pad->idev, EV_FF, FF_RUMBLE);
+ err = input_ff_create_memless(pad->idev, NULL, psxpad_spi_ff);
+ if (err) {
+ pr_err("psxpad-spi: ff alloc failed!!\n");
+ err = -ENOMEM;
+ }
+
+ return err;
+}
+
+static void psxpad_spi_deinit_ff(struct psxpad *pad)
+{
+ input_ff_destroy(pad->idev);
+}
+#else /* PSXPAD_ENABLE_FF */
+static inline int psxpad_spi_init_ff(struct psxpad *pad)
+{
+ return 0;
+}
+
+static void psxpad_spi_deinit_ff(struct psxpad *pad) { }
+#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 = devm_kzalloc(&spi->dev, sizeof(struct psxpad), GFP_KERNEL);
+ pdev = input_allocate_polled_device();
+ 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_HAT0Y, 0, 255, 0, 0); /* up */
+ input_set_abs_params(idev, ABS_HAT1Y, 0, 255, 0, 0); /* down */
+ input_set_abs_params(idev, ABS_HAT0X, 0, 255, 0, 0); /* left */
+ input_set_abs_params(idev, ABS_HAT1X, 0, 255, 0, 0); /* right */
+ input_set_abs_params(idev, ABS_MISC, 0, 255, 0, 0);
+ input_set_abs_params(idev, ABS_PRESSURE, 0, 255, 0, 0);
+ input_set_abs_params(idev, ABS_BRAKE, 0, 255, 0, 0);
+ input_set_abs_params(idev, ABS_THROTTLE, 0, 255, 0, 0);
+ input_set_abs_params(idev, ABS_HAT2X, 0, 255, 0, 0); /* L1 */
+ input_set_abs_params(idev, ABS_HAT3X, 0, 255, 0, 0); /* R1 */
+ input_set_abs_params(idev, ABS_HAT2Y, 0, 255, 0, 0); /* L2 */
+ input_set_abs_params(idev, ABS_HAT3Y, 0, 255, 0, 0); /* R2 */
+#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);
+
+ /* force feedback */
+ err = psxpad_spi_init_ff(pad);
+ if (err) {
+ err = -ENOMEM;
+ goto err_free_mem;
+ }
+
+ /* SPI settings */
+ spi->mode = SPI_MODE_3;
+ spi->bits_per_word = 8;
+ spi_setup(spi);
+
+ /* pad settings */
+ psxpad_setmotorlevel(pad, 0, 0);
+
+ /* 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) {
+ psxpad_spi_deinit_ff(pad);
+ input_free_polled_device(pdev);
+ }
+ devm_kfree(&spi->dev, pad);
+
+ return err;
+}
+
+
+static int psxpad_spi_remove(struct spi_device *spi)
+{
+ struct psxpad *pad = spi_get_drvdata(spi);
+
+ psxpad_spi_deinit_ff(pad);
+ input_free_polled_device(pad->pdev);
+ devm_kfree(&spi->dev, 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);
+ psxpad_setmotorlevel(pad, 0, 0);
+ psxpad_setenablemotor(pad, false, false);
+
+ 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);
+ psxpad_setmotorlevel(pad, pad->sus_motor1enable, pad->sus_motor2enable);
+ psxpad_setenablemotor(pad, pad->sus_motor1level, pad->sus_motor2level);
+
+ 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 },
+ { }
+};
+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 <[email protected]>");
+MODULE_DESCRIPTION("PSX (Play Station 1/2) pad with SPI Bus Driver");
+MODULE_LICENSE("GPL");
--
2.11.0

2017-04-26 09:31:56

by Antonio Ospite

[permalink] [raw]

Subject: Re: [PATCH] drivers: input: joystick: Add PSX (Play Station 1/2) pad with SPI driver.

On Tue, 25 Apr 2017 23:44:22 +0900
AZO <[email protected]> wrote:

> PSX pads can be connected directry SPI bus.
^
"directly"

and add "to the" before SPI.

>
> Signed-off-by: AZO <[email protected]>
> ---

Hi,

I haven't looked at the code but I have some general comments.

When submitting another iteration of a patch it's common practice to
mention the version in the Subject (e.g. [PATCH v3] ...), you can use:

git format-patch --subject-prefix='PATCH vX' ...

It is also useful to provide a changelog of the versions, so that
reviewers can see what the changes between the current patch and
the previous versions are. This changelog goes after the '---'
separator and before the diffstat, this way git will ignore it when the
patch is applied, it's meant for the review process and it doesn't need
to go in the commit message of the final version.

Further annotations can also go after the '---' separator.

For more details look at Documentation/SubmittingPatches in the linux
kernel tree.

Also try to use a subject line consistent with the subsystem, by
looking at the history of the files in the same directory.

In this case it could be something like:

Input: psxpad-spi - Add PSX (Play Station 1/2) pad SPI driver

Finally, script/checkpatch.pl suggests some minor issues, I tried with:

./scripts/checkpatch.pl --ignore LONG_LINE,LONG_LINE_COMMENT your.patch

You can ignore some of them, and motivate your decision in an
annotation.

Also try to comment when you do not agree with the reviewer or cannot
comply (for example about using an interrupt instead of polling).

It's fine if your English is not perfect yet, don't let that stop you :)

Ciao ciao,
Antonio

--
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

2017-04-26 11:46:22

by Tomohiro Yoshidomi

[permalink] [raw]

Subject: Re: [PATCH] drivers: input: joystick: Add PSX (Play Station 1/2) pad with SPI driver.

Mr.Ospite

Hi. Thanks to teaching me a lot.

I as much as possible comply rule for Linux developing.
I can't write my mind in English, as you said. (XD)
But, I'll continuously study more Linux and Git and English!
And I can write in C language to work some devices.

>> PSX pads can be connected directry SPI bus.
> ^
> "directly"

I'm sorry to my cheap English...

> git format-patch --subject-prefix='PATCH vX' ...

> Further annotations can also go after the '---' separator.

I wanted to tell a version and what is changes,
I looked for this writing.
I'll use this.

> For more details look at Documentation/SubmittingPatches in the linux
> kernel tree.

I will read it carefully.

> Input: psxpad-spi - Add PSX (Play Station 1/2) pad SPI driver

I'll fix next time.

> ./scripts/checkpatch.pl --ignore LONG_LINE,LONG_LINE_COMMENT your.patch

I use that script with no option.
I'll use useful options too.

> reviewers can see what the changes between the current patch and
> the previous versions are.

> Also try to comment when you do not agree with the reviewer or cannot
> comply (for example about using an interrupt instead of polling).

I get diff usually, between original source and my changed it.
But developing on Git, I know not recommend this method,
developing must as time goes by.

I just worry to send my code rightly.

I thanks you and maintainer Mr.Torokhov, patiently reception.

Regard.

---
AZO<[email protected]>

2017-04-26 17:52 GMT+09:00 Antonio Ospite <[email protected]>:
> On Tue, 25 Apr 2017 23:44:22 +0900
> AZO <[email protected]> wrote:
>
>> PSX pads can be connected directry SPI bus.
> ^
> "directly"
>
> and add "to the" before SPI.
>
>>
>> Signed-off-by: AZO <[email protected]>
>> ---
>
> Hi,
>
> I haven't looked at the code but I have some general comments.
>
> When submitting another iteration of a patch it's common practice to
> mention the version in the Subject (e.g. [PATCH v3] ...), you can use:
>
> git format-patch --subject-prefix='PATCH vX' ...
>
> It is also useful to provide a changelog of the versions, so that
> reviewers can see what the changes between the current patch and
> the previous versions are. This changelog goes after the '---'
> separator and before the diffstat, this way git will ignore it when the
> patch is applied, it's meant for the review process and it doesn't need
> to go in the commit message of the final version.
>
> Further annotations can also go after the '---' separator.
>
> For more details look at Documentation/SubmittingPatches in the linux
> kernel tree.
>
> Also try to use a subject line consistent with the subsystem, by
> looking at the history of the files in the same directory.
>
> In this case it could be something like:
>
> Input: psxpad-spi - Add PSX (Play Station 1/2) pad SPI driver
>
> Finally, script/checkpatch.pl suggests some minor issues, I tried with:
>
> ./scripts/checkpatch.pl --ignore LONG_LINE,LONG_LINE_COMMENT your.patch
>
> You can ignore some of them, and motivate your decision in an
> annotation.
>
> Also try to comment when you do not agree with the reviewer or cannot
> comply (for example about using an interrupt instead of polling).
>
> It's fine if your English is not perfect yet, don't let that stop you :)
>
> Ciao ciao,
> Antonio
>
> --
> Antonio Ospite
> https://ao2.it
> https://twitter.com/ao2it
>
> A: Because it messes up the order in which people normally read text.
> See http://en.wikipedia.org/wiki/Posting_style
> Q: Why is top-posting such a bad thing?

2017-04-27 08:17:57

by Dmitry Torokhov

[permalink] [raw]

Subject: Re: [PATCH] drivers: input: joystick: Add PSX (Play Station 1/2) pad with SPI driver.

Hi,

Thank you for making the changes. I have some more comments though.

On Tue, Apr 25, 2017 at 11:44:22PM +0900, AZO wrote:
> PSX pads can be connected directry SPI bus.
>
> Signed-off-by: AZO <[email protected]>

Please use your real name on the signoffs (apologies in advance in case
it is your proper name).

> ---
> drivers/input/joystick/Kconfig | 16 +
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/psxpad-spi.c | 671 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 688 insertions(+)
> create mode 100644 drivers/input/joystick/psxpad-spi.c
>
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index 4215b538..f49645f7 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -330,4 +330,20 @@ 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"
> + depends on INPUT_POLLDEV && SPI

INPUT_POLLDEV is meant to be "selected", so:

depends on SPI
select INPUT_POLLDEV

> + 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.
> +
> +config JOYSTICK_PSXPAD_SPI_FF
> + bool "PSX pad with SPI Bus rumble support"
> + depends on JOYSTICK_PSXPAD_SPI && INPUT

No need to depend on INPUT, everything here is already depending on it.

> + select INPUT_FF_MEMLESS
> + help
> + Say Y here if you want to take advantage of PSX pad rumble features.
> +
> endif
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 92dc0de9..496fd56b 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_JOYSTICK_INTERACT) += interact.o
> obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
> obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
> obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
> +obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o
> diff --git a/drivers/input/joystick/psxpad-spi.c b/drivers/input/joystick/psxpad-spi.c
> new file mode 100644
> index 00000000..3d2bee6d
> --- /dev/null
> +++ b/drivers/input/joystick/psxpad-spi.c
> @@ -0,0 +1,671 @@
> +/*
> + * PSX (Play Station 1/2) pad (SPI Interface)
> + *
> + * Copyright (C) 2017 AZO <[email protected]>
> + * 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/device.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

Why support for the "analog 2" packets is optional?

> +#ifdef CONFIG_JOYSTICK_PSXPAD_SPI_FF
> +#define PSXPAD_ENABLE_FF
> +#endif /* CONFIG_JOYSTICK_PSXPAD_SPI_FF */

Why do we need this indirection?

> +
> +enum {
> + PSXPAD_SPI_SPEED_125KHZ = 0,
> + PSXPAD_SPI_SPEED_250KHZ,
> + PSXPAD_SPI_SPEED_500KHZ,
> + PSXPAD_SPI_SPEED_UNKNOWN
> +};

I do not believe this definitions are needed. Just set speed explicitly
at spi_setup() time and be done.

> +
> +#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

Not used.

> +#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,
> + PSXPAD_KEYSTATE_TYPE_ANALOG2,
> + PSXPAD_KEYSTATE_TYPE_UNKNOWN
> +};
> +
> +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};
> +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)

This function really needs to be able to report errors.

> +{
> + struct spi_transfer *xfers;
> + struct spi_message msg;
> + u8 loc;
> + u8 sendbuf[0x40];

SPI transfers need buffers to be in DMA-able memory, they can't be
on-stack.

> +
> + xfers = kzalloc(sizeof(struct spi_transfer), GFP_KERNEL);
> + if (!xfers)
> + return;
> +
> + 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;
> + }
> + spi_sync_transfer(pad->spi, xfers, 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)
> +{
> + 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)
> +{
> + 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)
> +{
> + pad->motor1level = motor1level ? 0xFF : 0x00;
> + pad->motor2level = motor2level;
> +
> + pad->poolcmd[3] = pad->motor1level;
> + pad->poolcmd[4] = pad->motor2level;
> +}
> +#else /* PSXPAD_ENABLE_FF */
> +static void psxpad_setenablemotor(struct psxpad *pad, const bool motor1enable, const bool motor2enable) { }
> +
> +static void psxpad_setmotorlevel(struct psxpad *pad, const u8 motor1level, const u8 motor2level) { }
> +#endif /* PSXPAD_ENABLE_FF */
> +
> +static void psxpad_getkeystate(struct psxpad *pad, struct psxpad_keystate *keystate)
> +{
> + 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);

Instead of first parsing, storing state in "pad" and then reporting, why
can't we report events as we parse the data read from the pad?

> + psxpad_setenablemotor(pad, true, true);
> +
> + switch (keystate.type) {
> +#ifdef PSXPAD_ENABLE_ANALOG2
> + case PSXPAD_KEYSTATE_TYPE_ANALOG2:
> + input_report_abs(pad->idev, ABS_HAT0Y, keystate.a_up);
> + input_report_abs(pad->idev, ABS_HAT1Y, keystate.a_down);
> + input_report_abs(pad->idev, ABS_HAT0X, keystate.a_left);
> + input_report_abs(pad->idev, ABS_HAT1X, keystate.a_right);
> + input_report_abs(pad->idev, ABS_MISC, keystate.a_triangle);
> + input_report_abs(pad->idev, ABS_PRESSURE, keystate.a_circle);
> + input_report_abs(pad->idev, ABS_BRAKE, keystate.a_cross);
> + input_report_abs(pad->idev, ABS_THROTTLE, keystate.a_square);
> + input_report_abs(pad->idev, ABS_HAT2X, keystate.a_l1);
> + input_report_abs(pad->idev, ABS_HAT3X, keystate.a_r1);
> + input_report_abs(pad->idev, ABS_HAT2Y, keystate.a_l2);
> + input_report_abs(pad->idev, ABS_HAT3Y, 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:
> +#ifdef PSXPAD_ENABLE_ANALOG2
> + input_report_abs(pad->idev, ABS_HAT0Y, 0);
> + input_report_abs(pad->idev, ABS_HAT1Y, 0);
> + input_report_abs(pad->idev, ABS_HAT0X, 0);
> + input_report_abs(pad->idev, ABS_HAT1X, 0);
> + input_report_abs(pad->idev, ABS_MISC, 0);
> + input_report_abs(pad->idev, ABS_PRESSURE, 0);
> + input_report_abs(pad->idev, ABS_BRAKE, 0);
> + input_report_abs(pad->idev, ABS_THROTTLE, 0);
> + input_report_abs(pad->idev, ABS_HAT2X, 0);
> + input_report_abs(pad->idev, ABS_HAT3X, 0);
> + input_report_abs(pad->idev, ABS_HAT2Y, 0);
> + input_report_abs(pad->idev, ABS_HAT3Y, 0);
> +#endif /* PSXPAD_ENABLE_ANALOG2 */
> + 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:
> +#ifdef PSXPAD_ENABLE_ANALOG2
> + input_report_abs(pad->idev, ABS_HAT0Y, 0);
> + input_report_abs(pad->idev, ABS_HAT1Y, 0);
> + input_report_abs(pad->idev, ABS_HAT0X, 0);
> + input_report_abs(pad->idev, ABS_HAT1X, 0);
> + input_report_abs(pad->idev, ABS_MISC, 0);
> + input_report_abs(pad->idev, ABS_PRESSURE, 0);
> + input_report_abs(pad->idev, ABS_BRAKE, 0);
> + input_report_abs(pad->idev, ABS_THROTTLE, 0);
> + input_report_abs(pad->idev, ABS_HAT2X, 0);
> + input_report_abs(pad->idev, ABS_HAT3X, 0);
> + input_report_abs(pad->idev, ABS_HAT2Y, 0);
> + input_report_abs(pad->idev, ABS_HAT3Y, 0);
> +#endif /* PSXPAD_ENABLE_ANALOG2 */
> + 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);

"play effect" handler is called with spinlock held and interrupts
disabled, you can't sleep here (and you are doing synchronous SPI
transfers which do sleep).

You need to offload adjusting motor levels to a workqueue, or look into
using asynchronous SPI transfers when handling FF playback requests.

> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int psxpad_spi_init_ff(struct psxpad *pad)
> +{
> + int err;
> +
> + input_set_capability(pad->idev, EV_FF, FF_RUMBLE);
> + err = input_ff_create_memless(pad->idev, NULL, psxpad_spi_ff);
> + if (err) {
> + pr_err("psxpad-spi: ff alloc failed!!\n");
> + err = -ENOMEM;
> + }
> +
> + return err;
> +}
> +
> +static void psxpad_spi_deinit_ff(struct psxpad *pad)
> +{
> + input_ff_destroy(pad->idev);
> +}
> +#else /* PSXPAD_ENABLE_FF */
> +static inline int psxpad_spi_init_ff(struct psxpad *pad)
> +{
> + return 0;
> +}
> +
> +static void psxpad_spi_deinit_ff(struct psxpad *pad) { }
> +#endif /* PSXPAD_ENABLE_FF */
> +
> +static int psxpad_spi_probe(struct spi_device *spi)
> +{
> + struct psxpad *pad = NULL;
> + struct input_polled_dev *pdev = NULL;

No need to initialize.

> + struct input_dev *idev;
> + int err, i;
> +
> + pad = devm_kzalloc(&spi->dev, sizeof(struct psxpad), GFP_KERNEL);
> + pdev = input_allocate_polled_device();
> + if (!pad || !pdev) {
> + pr_err("psxpad-spi: alloc failed!!\n");

Please use dev_err(). Also, since you are using managed API now, please
check the result of each allocation separately. I.e. allocate "pad" and
check, then try allocating "pdev".

> + err = -ENOMEM;
> + goto err_free_mem;
> + }
> + pdev->input->ff = NULL;

No need to set this to NULL, input core will not leave garbage there.

> + 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));

sizeof(pad->phys) and get rid of PSXPAD_DEFAULT_INPUT_PHYSIZE.

> + 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);

Not really needed, input_set_abs_params will end up setting EV_ABS bit.

> + 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_HAT0Y, 0, 255, 0, 0); /* up */
> + input_set_abs_params(idev, ABS_HAT1Y, 0, 255, 0, 0); /* down */
> + input_set_abs_params(idev, ABS_HAT0X, 0, 255, 0, 0); /* left */
> + input_set_abs_params(idev, ABS_HAT1X, 0, 255, 0, 0); /* right */
> + input_set_abs_params(idev, ABS_MISC, 0, 255, 0, 0);
> + input_set_abs_params(idev, ABS_PRESSURE, 0, 255, 0, 0);
> + input_set_abs_params(idev, ABS_BRAKE, 0, 255, 0, 0);
> + input_set_abs_params(idev, ABS_THROTTLE, 0, 255, 0, 0);
> + input_set_abs_params(idev, ABS_HAT2X, 0, 255, 0, 0); /* L1 */
> + input_set_abs_params(idev, ABS_HAT3X, 0, 255, 0, 0); /* R1 */
> + input_set_abs_params(idev, ABS_HAT2Y, 0, 255, 0, 0); /* L2 */
> + input_set_abs_params(idev, ABS_HAT3Y, 0, 255, 0, 0); /* R2 */
> +#endif /* PSXPAD_ENABLE_ANALOG2 */
> + __set_bit(EV_KEY, idev->evbit);

> + __set_bit(BTN_DPAD_UP, idev->keybit);

I prefer using input_set_capability(idev, EV_KEY, BTN_DPAD_UP);

Then we can drop __set_bit(EV_KEY, idev->evbit);

> + __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);
> +
> + /* force feedback */
> + err = psxpad_spi_init_ff(pad);
> + if (err) {
> + err = -ENOMEM;
> + goto err_free_mem;
> + }
> +
> + /* SPI settings */
> + spi->mode = SPI_MODE_3;
> + spi->bits_per_word = 8;
> + spi_setup(spi);
> +
> + /* pad settings */
> + psxpad_setmotorlevel(pad, 0, 0);
> +
> + /* 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) {
> + psxpad_spi_deinit_ff(pad);
> + input_free_polled_device(pdev);
> + }
> + devm_kfree(&spi->dev, pad);
> +
> + return err;
> +}
> +
> +
> +static int psxpad_spi_remove(struct spi_device *spi)
> +{
> + struct psxpad *pad = spi_get_drvdata(spi);
> +
> + psxpad_spi_deinit_ff(pad);
> + input_free_polled_device(pad->pdev);
> + devm_kfree(&spi->dev, pad);

No need to call devm_kfree or input_free_polled_device, neither here,
nor in probe. They will be called automatically when needed. That is the
point of using managed APIs.

Calling psxpad_spi_deinit_ff() is not really needed either. FF will be
properly destroyed by the input core when input device is destroyed.

> +
> + 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);
> + psxpad_setmotorlevel(pad, 0, 0);
> + psxpad_setenablemotor(pad, false, false);
> +
> + 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;

Why do we need to set/adjust it here?

> + psxpad_setadmode(pad, pad->sus_analog_mode, pad->sus_mode_lock);
> + psxpad_setmotorlevel(pad, pad->sus_motor1enable, pad->sus_motor2enable);
> + psxpad_setenablemotor(pad, pad->sus_motor1level, pad->sus_motor2level);
> +
> + 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 },
> + { }
> +};
> +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 <[email protected]>");
> +MODULE_DESCRIPTION("PSX (Play Station 1/2) pad with SPI Bus Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.11.0
>

Thanks.

--
Dmitry

2017-04-28 02:38:20

by Tomohiro Yoshidomi

[permalink] [raw]

Subject: Re: [PATCH] drivers: input: joystick: Add PSX (Play Station 1/2) pad with SPI driver.

Mr. Torokhov

Hi. I'm AZO = Tomohiro Yoshidomi.

I use handle name in my work.
I'll use my real name in Linux development.
I'm sorry to confuse you.

I fixed my source to PATCH V4, and sent you.
Please check and let me know if you notice further.

2017-04-27 17:17 GMT+09:00 Dmitry Torokhov <[email protected]>:
> Hi,
>
> Thank you for making the changes. I have some more comments though.
>
> On Tue, Apr 25, 2017 at 11:44:22PM +0900, AZO wrote:
>> PSX pads can be connected directry SPI bus.
>>
>> Signed-off-by: AZO <[email protected]>
>
> Please use your real name on the signoffs (apologies in advance in case
> it is your proper name).

Fixed.
Signed-off with my real name.

>> ---
>> drivers/input/joystick/Kconfig | 16 +
>> drivers/input/joystick/Makefile | 1 +
>> drivers/input/joystick/psxpad-spi.c | 671 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 688 insertions(+)
>> create mode 100644 drivers/input/joystick/psxpad-spi.c
>>
>> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
>> index 4215b538..f49645f7 100644
>> --- a/drivers/input/joystick/Kconfig
>> +++ b/drivers/input/joystick/Kconfig
>> @@ -330,4 +330,20 @@ 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"
>> + depends on INPUT_POLLDEV && SPI
>
> INPUT_POLLDEV is meant to be "selected", so:
>
> depends on SPI
> select INPUT_POLLDEV

Fixed.

>
>> + 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.
>> +
>> +config JOYSTICK_PSXPAD_SPI_FF
>> + bool "PSX pad with SPI Bus rumble support"
>> + depends on JOYSTICK_PSXPAD_SPI && INPUT
>
> No need to depend on INPUT, everything here is already depending on it.

Fixed.

>> + select INPUT_FF_MEMLESS
>> + help
>> + Say Y here if you want to take advantage of PSX pad rumble features.
>> +
>> endif
>> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
>> index 92dc0de9..496fd56b 100644
>> --- a/drivers/input/joystick/Makefile
>> +++ b/drivers/input/joystick/Makefile
>> @@ -21,6 +21,7 @@ obj-$(CONFIG_JOYSTICK_INTERACT) += interact.o
>> obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
>> obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
>> obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
>> +obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
>> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
>> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
>> obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o
>> diff --git a/drivers/input/joystick/psxpad-spi.c b/drivers/input/joystick/psxpad-spi.c
>> new file mode 100644
>> index 00000000..3d2bee6d
>> --- /dev/null
>> +++ b/drivers/input/joystick/psxpad-spi.c
>> @@ -0,0 +1,671 @@
>> +/*
>> + * PSX (Play Station 1/2) pad (SPI Interface)
>> + *
>> + * Copyright (C) 2017 AZO <[email protected]>
>> + * 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/device.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
>
> Why support for the "analog 2" packets is optional?

PSX pad (DUALSHOCK2) has DPAD and NSWE analog pressure buttons.
I tried to use micro-controller AVR, those analog value can be gotten.
But Linux (Raspberry Pi) couldn't get analog value rightly.

Since it may support in the future, I will leave codes.

>> +#ifdef CONFIG_JOYSTICK_PSXPAD_SPI_FF
>> +#define PSXPAD_ENABLE_FF
>> +#endif /* CONFIG_JOYSTICK_PSXPAD_SPI_FF */
>
> Why do we need this indirection?

Fixed.
Only use 'CONFIG_JOYSTICK_PSXPAD_SPI_FF'.

>> +
>> +enum {
>> + PSXPAD_SPI_SPEED_125KHZ = 0,
>> + PSXPAD_SPI_SPEED_250KHZ,
>> + PSXPAD_SPI_SPEED_500KHZ,
>> + PSXPAD_SPI_SPEED_UNKNOWN
>> +};
>
> I do not believe this definitions are needed. Just set speed explicitly
> at spi_setup() time and be done.

Fixed.
I can set speed_hz at spi_setup().
I deleted those writing was.

>> +
>> +#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
>
> Not used.

Fixed.

>> +#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,
>> + PSXPAD_KEYSTATE_TYPE_ANALOG2,
>> + PSXPAD_KEYSTATE_TYPE_UNKNOWN
>> +};
>> +
>> +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};
>> +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)
>
> This function really needs to be able to report errors.

Fixed.
I fixed this function has return value.
And caller use return value, if error, output message.

>> +{
>> + struct spi_transfer *xfers;
>> + struct spi_message msg;
>> + u8 loc;
>> + u8 sendbuf[0x40];
>
> SPI transfers need buffers to be in DMA-able memory, they can't be
> on-stack.

Fixed.
'sendbuf' and 'response' locate on cache aligned.

And 'xfers' can be located on stack. Dynamic allocation was removed.

>> +
>> + xfers = kzalloc(sizeof(struct spi_transfer), GFP_KERNEL);
>> + if (!xfers)
>> + return;
>> +
>> + 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;
>> + }
>> + spi_sync_transfer(pad->spi, xfers, 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)
>> +{
>> + 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)
>> +{
>> + 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)
>> +{
>> + pad->motor1level = motor1level ? 0xFF : 0x00;
>> + pad->motor2level = motor2level;
>> +
>> + pad->poolcmd[3] = pad->motor1level;
>> + pad->poolcmd[4] = pad->motor2level;
>> +}
>> +#else /* PSXPAD_ENABLE_FF */
>> +static void psxpad_setenablemotor(struct psxpad *pad, const bool motor1enable, const bool motor2enable) { }
>> +
>> +static void psxpad_setmotorlevel(struct psxpad *pad, const u8 motor1level, const u8 motor2level) { }
>> +#endif /* PSXPAD_ENABLE_FF */
>> +
>> +static void psxpad_getkeystate(struct psxpad *pad, struct psxpad_keystate *keystate)
>> +{
>> + 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);
>
> Instead of first parsing, storing state in "pad" and then reporting, why
> can't we report events as we parse the data read from the pad?

Fixed.
Reporting with poll response data directly.
Then 'psxpad_getkeystate()' and 'struct psxpad_keystate' was deleted.

>> + psxpad_setenablemotor(pad, true, true);
>> +
>> + switch (keystate.type) {
>> +#ifdef PSXPAD_ENABLE_ANALOG2
>> + case PSXPAD_KEYSTATE_TYPE_ANALOG2:
>> + input_report_abs(pad->idev, ABS_HAT0Y, keystate.a_up);
>> + input_report_abs(pad->idev, ABS_HAT1Y, keystate.a_down);
>> + input_report_abs(pad->idev, ABS_HAT0X, keystate.a_left);
>> + input_report_abs(pad->idev, ABS_HAT1X, keystate.a_right);
>> + input_report_abs(pad->idev, ABS_MISC, keystate.a_triangle);
>> + input_report_abs(pad->idev, ABS_PRESSURE, keystate.a_circle);
>> + input_report_abs(pad->idev, ABS_BRAKE, keystate.a_cross);
>> + input_report_abs(pad->idev, ABS_THROTTLE, keystate.a_square);
>> + input_report_abs(pad->idev, ABS_HAT2X, keystate.a_l1);
>> + input_report_abs(pad->idev, ABS_HAT3X, keystate.a_r1);
>> + input_report_abs(pad->idev, ABS_HAT2Y, keystate.a_l2);
>> + input_report_abs(pad->idev, ABS_HAT3Y, 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:
>> +#ifdef PSXPAD_ENABLE_ANALOG2
>> + input_report_abs(pad->idev, ABS_HAT0Y, 0);
>> + input_report_abs(pad->idev, ABS_HAT1Y, 0);
>> + input_report_abs(pad->idev, ABS_HAT0X, 0);
>> + input_report_abs(pad->idev, ABS_HAT1X, 0);
>> + input_report_abs(pad->idev, ABS_MISC, 0);
>> + input_report_abs(pad->idev, ABS_PRESSURE, 0);
>> + input_report_abs(pad->idev, ABS_BRAKE, 0);
>> + input_report_abs(pad->idev, ABS_THROTTLE, 0);
>> + input_report_abs(pad->idev, ABS_HAT2X, 0);
>> + input_report_abs(pad->idev, ABS_HAT3X, 0);
>> + input_report_abs(pad->idev, ABS_HAT2Y, 0);
>> + input_report_abs(pad->idev, ABS_HAT3Y, 0);
>> +#endif /* PSXPAD_ENABLE_ANALOG2 */
>> + 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:
>> +#ifdef PSXPAD_ENABLE_ANALOG2
>> + input_report_abs(pad->idev, ABS_HAT0Y, 0);
>> + input_report_abs(pad->idev, ABS_HAT1Y, 0);
>> + input_report_abs(pad->idev, ABS_HAT0X, 0);
>> + input_report_abs(pad->idev, ABS_HAT1X, 0);
>> + input_report_abs(pad->idev, ABS_MISC, 0);
>> + input_report_abs(pad->idev, ABS_PRESSURE, 0);
>> + input_report_abs(pad->idev, ABS_BRAKE, 0);
>> + input_report_abs(pad->idev, ABS_THROTTLE, 0);
>> + input_report_abs(pad->idev, ABS_HAT2X, 0);
>> + input_report_abs(pad->idev, ABS_HAT3X, 0);
>> + input_report_abs(pad->idev, ABS_HAT2Y, 0);
>> + input_report_abs(pad->idev, ABS_HAT3Y, 0);
>> +#endif /* PSXPAD_ENABLE_ANALOG2 */
>> + 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);
>
> "play effect" handler is called with spinlock held and interrupts
> disabled, you can't sleep here (and you are doing synchronous SPI
> transfers which do sleep).
>
> You need to offload adjusting motor levels to a workqueue, or look into
> using asynchronous SPI transfers when handling FF playback requests.

psxpad_setmotorlevel() is only set memory value. Not SPI tranfers.
FF effect starts (FF value is used) at psxpad_spi_pool().

>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int psxpad_spi_init_ff(struct psxpad *pad)
>> +{
>> + int err;
>> +
>> + input_set_capability(pad->idev, EV_FF, FF_RUMBLE);
>> + err = input_ff_create_memless(pad->idev, NULL, psxpad_spi_ff);
>> + if (err) {
>> + pr_err("psxpad-spi: ff alloc failed!!\n");
>> + err = -ENOMEM;
>> + }
>> +
>> + return err;
>> +}
>> +
>> +static void psxpad_spi_deinit_ff(struct psxpad *pad)
>> +{
>> + input_ff_destroy(pad->idev);
>> +}
>> +#else /* PSXPAD_ENABLE_FF */
>> +static inline int psxpad_spi_init_ff(struct psxpad *pad)
>> +{
>> + return 0;
>> +}
>> +
>> +static void psxpad_spi_deinit_ff(struct psxpad *pad) { }
>> +#endif /* PSXPAD_ENABLE_FF */
>> +
>> +static int psxpad_spi_probe(struct spi_device *spi)
>> +{
>> + struct psxpad *pad = NULL;
>> + struct input_polled_dev *pdev = NULL;
>
> No need to initialize.

Fixed.

>> + struct input_dev *idev;
>> + int err, i;
>> +
>> + pad = devm_kzalloc(&spi->dev, sizeof(struct psxpad), GFP_KERNEL);
>> + pdev = input_allocate_polled_device();
>> + if (!pad || !pdev) {
>> + pr_err("psxpad-spi: alloc failed!!\n");
>
> Please use dev_err(). Also, since you are using managed API now, please
> check the result of each allocation separately. I.e. allocate "pad" and
> check, then try allocating "pdev".

Fixed.
This file's output log uses dev_err().
And check allocation separately.

>> + err = -ENOMEM;
>> + goto err_free_mem;
>> + }
>> + pdev->input->ff = NULL;
>
> No need to set this to NULL, input core will not leave garbage there.

Fixed.

>> + 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));
>
> sizeof(pad->phys) and get rid of PSXPAD_DEFAULT_INPUT_PHYSIZE.

Fixed.

>> + 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);
>
> Not really needed, input_set_abs_params will end up setting EV_ABS bit.

Fixed.

>> + 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_HAT0Y, 0, 255, 0, 0); /* up */
>> + input_set_abs_params(idev, ABS_HAT1Y, 0, 255, 0, 0); /* down */
>> + input_set_abs_params(idev, ABS_HAT0X, 0, 255, 0, 0); /* left */
>> + input_set_abs_params(idev, ABS_HAT1X, 0, 255, 0, 0); /* right */
>> + input_set_abs_params(idev, ABS_MISC, 0, 255, 0, 0);
>> + input_set_abs_params(idev, ABS_PRESSURE, 0, 255, 0, 0);
>> + input_set_abs_params(idev, ABS_BRAKE, 0, 255, 0, 0);
>> + input_set_abs_params(idev, ABS_THROTTLE, 0, 255, 0, 0);
>> + input_set_abs_params(idev, ABS_HAT2X, 0, 255, 0, 0); /* L1 */
>> + input_set_abs_params(idev, ABS_HAT3X, 0, 255, 0, 0); /* R1 */
>> + input_set_abs_params(idev, ABS_HAT2Y, 0, 255, 0, 0); /* L2 */
>> + input_set_abs_params(idev, ABS_HAT3Y, 0, 255, 0, 0); /* R2 */
>> +#endif /* PSXPAD_ENABLE_ANALOG2 */
>> + __set_bit(EV_KEY, idev->evbit);
>
>
>> + __set_bit(BTN_DPAD_UP, idev->keybit);
>
> I prefer using input_set_capability(idev, EV_KEY, BTN_DPAD_UP);
>
> Then we can drop __set_bit(EV_KEY, idev->evbit);

Fixed.

>> + __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);
>> +
>> + /* force feedback */
>> + err = psxpad_spi_init_ff(pad);
>> + if (err) {
>> + err = -ENOMEM;
>> + goto err_free_mem;
>> + }
>> +
>> + /* SPI settings */
>> + spi->mode = SPI_MODE_3;
>> + spi->bits_per_word = 8;
>> + spi_setup(spi);
>> +
>> + /* pad settings */
>> + psxpad_setmotorlevel(pad, 0, 0);
>> +
>> + /* 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) {
>> + psxpad_spi_deinit_ff(pad);
>> + input_free_polled_device(pdev);
>> + }
>> + devm_kfree(&spi->dev, pad);
>> +
>> + return err;
>> +}
>> +
>> +
>> +static int psxpad_spi_remove(struct spi_device *spi)
>> +{
>> + struct psxpad *pad = spi_get_drvdata(spi);
>> +
>> + psxpad_spi_deinit_ff(pad);
>> + input_free_polled_device(pad->pdev);
>> + devm_kfree(&spi->dev, pad);
>
> No need to call devm_kfree or input_free_polled_device, neither here,
> nor in probe. They will be called automatically when needed. That is the
> point of using managed APIs.
>
> Calling psxpad_spi_deinit_ff() is not really needed either. FF will be
> properly destroyed by the input core when input device is destroyed.

Fixed.

>> +
>> + 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);
>> + psxpad_setmotorlevel(pad, 0, 0);
>> + psxpad_setenablemotor(pad, false, false);
>> +
>> + 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;
>
> Why do we need to set/adjust it here?

Fixed.
My miss.

>> + psxpad_setadmode(pad, pad->sus_analog_mode, pad->sus_mode_lock);
>> + psxpad_setmotorlevel(pad, pad->sus_motor1enable, pad->sus_motor2enable);
>> + psxpad_setenablemotor(pad, pad->sus_motor1level, pad->sus_motor2level);
>> +
>> + 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 },
>> + { }
>> +};
>> +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 <[email protected]>");
>> +MODULE_DESCRIPTION("PSX (Play Station 1/2) pad with SPI Bus Driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.11.0
>>
>
> Thanks.
>
> --
> Dmitry

I'm amazed at your insight!!
The code has become wonderful.

Thanks.

---
AZO <[email protected]>

equal

Tomohiro Yoshidomi <[email protected]>