Re: [PATCH] drivers: input: joystick: Add PSX (Play Station 1/2) pad with SPI driver.
From: Dmitry Torokhov
Date: Thu Apr 27 2017 - 04:18:01 EST
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 <typesylph@xxxxxxxxx>
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 <typesylph@xxxxxxxxx>
> + * 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 <typesylph@xxxxxxxxx>");
> +MODULE_DESCRIPTION("PSX (Play Station 1/2) pad with SPI Bus Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.11.0
>
Thanks.
--
Dmitry