Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1032671AbdD1CiU (ORCPT ); Thu, 27 Apr 2017 22:38:20 -0400 Received: from mail-ua0-f194.google.com ([209.85.217.194]:36573 "EHLO mail-ua0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753603AbdD1CiN (ORCPT ); Thu, 27 Apr 2017 22:38:13 -0400 MIME-Version: 1.0 From: Tomohiro Yoshidomi Date: Fri, 28 Apr 2017 11:38:11 +0900 Message-ID: Subject: Re: [PATCH] drivers: input: joystick: Add PSX (Play Station 1/2) pad with SPI driver. To: Dmitry Torokhov Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 35113 Lines: 900 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 : > 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 > > 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 >> + * 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +//#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 "); >> +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 equal Tomohiro Yoshidomi