Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S378526AbdD3Dgz (ORCPT ); Sat, 29 Apr 2017 23:36:55 -0400 Received: from mail-vk0-f65.google.com ([209.85.213.65]:34772 "EHLO mail-vk0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S378484AbdD3Dgs (ORCPT ); Sat, 29 Apr 2017 23:36:48 -0400 MIME-Version: 1.0 In-Reply-To: <20170430033522.14216-1-sylph23k@gmail.com> References: <20170430033522.14216-1-sylph23k@gmail.com> From: Tomohiro Yoshidomi Date: Sun, 30 Apr 2017 12:36:46 +0900 Message-ID: Subject: Re: [PATCH v6] Input: psxpad-spi - Add PlayStation 1/2 joypads via SPI interface Driver To: Tomohiro Yoshidomi Cc: Dmitry Torokhov , 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: 23149 Lines: 686 Mr.Torokhov I fixed source, and sent PATCH v6 to you. As Mr.Nocera said, I fixed souces does not use 'PSX' in sentences. Also, I wrote that a separate power supply is required for the motor. 2017-04-30 3:16 GMT+09:00 Dmitry Torokhov : > Hi, > > On Sat, Apr 29, 2017 at 06:40:53PM +0900, Tomohiro Yoshidomi wrote: >> PSX pads can be connected directly to the SPI bus. >> >> Signed-off-by: Tomohiro Yoshidomi > > Thank you very much for making requested changes and your patience. I > think we just need a few finishing touches and we can commit this > driver. > >> --- >> drivers/input/joystick/Kconfig | 17 ++ >> drivers/input/joystick/Makefile | 1 + >> drivers/input/joystick/psxpad-spi.c | 387 ++++++++++++++++++++++++++++++++++++ >> 3 files changed, 405 insertions(+) >> create mode 100644 drivers/input/joystick/psxpad-spi.c >> >> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig >> index 4215b538..bfbda643 100644 >> --- a/drivers/input/joystick/Kconfig >> +++ b/drivers/input/joystick/Kconfig >> @@ -330,4 +330,21 @@ 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" > > Let's call this "PSX (Play Station 1/2) gamepad on SPI bus" Fixed. "PlayStation 1/2 joypads via SPI interface" >> + depends on SPI >> + select INPUT_POLLDEV >> + help >> + Say Y here if you connect PSX (PS1/2) pad with SPI Interface. > > "Say Y here if you wish to connect PSX (PS1/2) pad via SPI interface." Fixed. "Say Y here if you wish to connect PlayStation 1/2 joypads via 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" > > Let's call it "PSX gamepad force feedback (rumble) support" Fixed. "PlayStation 1/2 joypads force feedback (rumble) support" >> + depends on JOYSTICK_PSXPAD_SPI >> + 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..9684839c >> --- /dev/null >> +++ b/drivers/input/joystick/psxpad-spi.c >> @@ -0,0 +1,387 @@ >> +/* >> + * PSX (Play Station 1/2) pad (SPI Interface) >> + * >> + * Copyright (C) 2017 Tomohiro Yoshidomi >> + * 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 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)) >> + >> +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}; >> + >> +struct psxpad { >> + struct spi_device *spi; >> + struct input_polled_dev *pdev; >> + struct input_dev *idev; >> + char phys[0x20]; >> + bool motor1enable; >> + bool motor2enable; >> + u8 motor1level; >> + u8 motor2level; >> + u8 pollcmd[sizeof(PSX_CMD_POLL)]; >> + u8 enablemotor[sizeof(PSX_CMD_ENABLE_MOTOR)]; >> + u8 sendbuf[0x20] ____cacheline_aligned; >> + u8 response[sizeof(PSX_CMD_POLL)] ____cacheline_aligned; >> +}; >> + >> +static int psxpad_command(struct psxpad *pad, const u8 sendcmd[], u8 response[], const u8 sendcmdlen) > > I wonder if we really need to pass "response" as a parameter here. Fixed. >> +{ >> + struct spi_transfer xfers = { >> + .tx_buf = pad->sendbuf, >> + .rx_buf = response, >> + .len = sendcmdlen, >> + }; >> + struct spi_message msg; >> + int err; >> + u8 loc; > > Let's call it "int i" - the standard loop variable. > >> + >> + spi_message_init(&msg); > > I do not think we need "msg" anymore. Fixed. >> + >> + for (loc = 0; loc < sendcmdlen; loc++) >> + pad->sendbuf[loc] = REVERSE_BIT(sendcmd[loc]); > > I think I already asked this, but I do not recall the answer: can we > supply commands with bits pre-reversed (i.e. specify PSX_CMD_POLL as > 0x80, 0x42, 0x00, ... and PSX_CMD_ENTER_CFG as 0x80, 0xc2, 0x00, 0x80, > 0x00, ... and so on, and reverse arguments when you supply them? See > more in psxpad_setenablemotor(). Fixed. I use MSBFIRST (not LSBFIRST) values. >> + >> + err = spi_sync_transfer(pad->spi, &xfers, 1); >> + if (err) { >> + dev_err(&pad->idev->dev, "psxpad-spi: failed SPI xfers!!\n"); >> + goto err_end; > > Just return directly here: > > return err; Fixed. >> + } >> + >> + for (loc = 0; loc < sendcmdlen; loc++) >> + response[loc] = REVERSE_BIT(response[loc]); > > We only use returned data when we poll the device. Maybe we should do > this bit-reversing outsize of psxpad_command()? Fixed. >> + >> + return 0; >> + >> + err_end: >> + >> + return err; >> +} >> + >> +#ifdef CONFIG_JOYSTICK_PSXPAD_SPI_FF >> +static void psxpad_setenablemotor(struct psxpad *pad, const bool motor1enable, const bool motor2enable) > > psxpad_control_motor() and there is really no need for "const" on scalar > arguments. Fixed. psxpad_set_motor_level() was fixed too. >> +{ >> + int err; >> + >> + pad->motor1enable = motor1enable; >> + pad->motor2enable = motor2enable; >> + >> + pad->enablemotor[3] = pad->motor1enable ? 0x00 : 0xFF; >> + pad->enablemotor[4] = pad->motor2enable ? 0x01 : 0xFF; > > Assuming that the commands are "pre-revrsed": > > memcpy(pad->sendbuf, PSX_CMD_ENTER_CFG, sizeof(PSX_CMD_ENTER_CFG); > err = psxpad_command(pad, sizeof(PSX_CMD_ENTER_CFG)); > if (err) { > dev_err(&pad->spi->dev, > "%s: failed to enter config mode: %d\n", > __func__, err); > return err; > } > > memcpy(pad->sendbuf, PSX_CMD_ENABLE_MOTOR, sizeof(PSX_CMD_ENABLE_MOTOR); > pad->sendbuf[3] = motor1enable ? 0x00 : 0xff; > pad->sendbuf[4] = motor2enable ? 0x80 : 0xff; > err = psxpad_command(pad, sizeof(PSX_CMD_ENABLE_MOTOR)); > if (err) { > dev_err(&pad->spi->dev, > "%s: ENABLE_MOTOR command failed: %d\n", > __func__, err); > return err; > } > > ... Fixed. I wrote this code with reading otherone's PSX pad's analysis informaton. The LSBFIRST values are listed in them. I implemented the value so that it is easy to read. I commented LSBFIRST value, and change to use MSBFIRST value. >> + >> + err = psxpad_command(pad, PSX_CMD_ENTER_CFG, pad->response, sizeof(PSX_CMD_ENTER_CFG)); >> + if (err) { >> + dev_err(&pad->idev->dev, "psxpad-spi: setenablemotor: enter cfg failed!!\n"); >> + return; >> + } >> + err = psxpad_command(pad, pad->enablemotor, pad->response, sizeof(PSX_CMD_ENABLE_MOTOR)); >> + if (err) { >> + dev_err(&pad->idev->dev, "psxpad-spi: setenablemotor: enable motor failed!!\n"); >> + return; >> + } >> + err = psxpad_command(pad, PSX_CMD_EXIT_CFG, pad->response, sizeof(PSX_CMD_EXIT_CFG)); >> + if (err) { >> + dev_err(&pad->idev->dev, "psxpad-spi: setenablemotor: exit config failed!!\n"); >> + return; >> + } >> +} >> + >> +static void psxpad_setmotorlevel(struct psxpad *pad, const u8 motor1level, const u8 motor2level) > > psxpad_set_motor_level. Fixed. >> +{ >> + pad->motor1level = motor1level ? 0xFF : 0x00; >> + pad->motor2level = motor2level; >> + >> + pad->pollcmd[3] = pad->motor1level; >> + pad->pollcmd[4] = pad->motor2level; > > So we need to control motors both while polling and explicitly at end of > poll? If the polling is interrupted at regular intervals, the pad will forget to set it. So I'm enabling the motor in the polling. And, I changed setting motors at first of polling. >> +} >> +#else /* CONFIG_JOYSTICK_PSXPAD_SPI_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) { } > > Please try keeping within 80 columns. > Fixed. I got it. >> +#endif /* CONFIG_JOYSTICK_PSXPAD_SPI_FF */ >> + >> +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; >> + int err; >> + >> + err = psxpad_command(pad, pad->pollcmd, pad->response, sizeof(PSX_CMD_POLL)); >> + if (err) { >> + dev_err(&pad->idev->dev, "psxpad-spi: poll: poll cmd failed!!\n"); > > Please use pad->spi->dev in error messages, so errors are associated > with a physical device and not abstract one (inputNNN). Fixed. >> + return; >> + } >> + >> + switch (pad->response[1]) { >> + case 0x73: /* analog 1 */ >> + input_report_abs(pad->idev, ABS_X, pad->response[7]); >> + input_report_abs(pad->idev, ABS_Y, pad->response[8]); >> + input_report_abs(pad->idev, ABS_RX, pad->response[5]); >> + input_report_abs(pad->idev, ABS_RY, pad->response[6]); >> + input_report_key(pad->idev, BTN_DPAD_UP, (pad->response[3] & 0x10U) ? false : true); >> + input_report_key(pad->idev, BTN_DPAD_DOWN, (pad->response[3] & 0x40U) ? false : true); > > Please use BIT() macro; there is also no need to convert to true/false > as input_report_key also does it. > > input_report_key(pad->idev, BTN_DPAD_UP, > pad->response[3] & BIT(4)); > input_report_key(pad->idev, BTN_DPAD_DOWN, > pad->response[3] & BIT(7)); > > and so on. Fixed. >> + input_report_key(pad->idev, BTN_DPAD_LEFT, (pad->response[3] & 0x80U) ? false : true); >> + input_report_key(pad->idev, BTN_DPAD_RIGHT, (pad->response[3] & 0x20U) ? false : true); >> + input_report_key(pad->idev, BTN_X, (pad->response[4] & 0x10U) ? false : true); >> + input_report_key(pad->idev, BTN_A, (pad->response[4] & 0x20U) ? false : true); >> + input_report_key(pad->idev, BTN_B, (pad->response[4] & 0x40U) ? false : true); >> + input_report_key(pad->idev, BTN_Y, (pad->response[4] & 0x80U) ? false : true); >> + input_report_key(pad->idev, BTN_TL, (pad->response[4] & 0x04U) ? false : true); >> + input_report_key(pad->idev, BTN_TR, (pad->response[4] & 0x08U) ? false : true); >> + input_report_key(pad->idev, BTN_TL2, (pad->response[4] & 0x01U) ? false : true); >> + input_report_key(pad->idev, BTN_TR2, (pad->response[4] & 0x02U) ? false : true); >> + input_report_key(pad->idev, BTN_THUMBL, (pad->response[3] & 0x02U) ? false : true); >> + input_report_key(pad->idev, BTN_THUMBR, (pad->response[3] & 0x04U) ? false : true); >> + input_report_key(pad->idev, BTN_SELECT, (pad->response[3] & 0x01U) ? false : true); >> + input_report_key(pad->idev, BTN_START, (pad->response[3] & 0x08U) ? false : true); >> + break; >> + >> + case 0x41: /* digital */ >> + 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, (pad->response[3] & 0x10U) ? false : true); >> + input_report_key(pad->idev, BTN_DPAD_DOWN, (pad->response[3] & 0x40U) ? false : true); >> + input_report_key(pad->idev, BTN_DPAD_LEFT, (pad->response[3] & 0x80U) ? false : true); >> + input_report_key(pad->idev, BTN_DPAD_RIGHT, (pad->response[3] & 0x20U) ? false : true); >> + input_report_key(pad->idev, BTN_X, (pad->response[4] & 0x10U) ? false : true); >> + input_report_key(pad->idev, BTN_A, (pad->response[4] & 0x20U) ? false : true); >> + input_report_key(pad->idev, BTN_B, (pad->response[4] & 0x40U) ? false : true); >> + input_report_key(pad->idev, BTN_Y, (pad->response[4] & 0x80U) ? false : true); >> + input_report_key(pad->idev, BTN_TL, (pad->response[4] & 0x04U) ? false : true); >> + input_report_key(pad->idev, BTN_TR, (pad->response[4] & 0x08U) ? false : true); >> + input_report_key(pad->idev, BTN_TL2, (pad->response[4] & 0x01U) ? false : true); >> + input_report_key(pad->idev, BTN_TR2, (pad->response[4] & 0x02U) ? false : true); >> + input_report_key(pad->idev, BTN_THUMBL, false); >> + input_report_key(pad->idev, BTN_THUMBR, false); >> + input_report_key(pad->idev, BTN_SELECT, (pad->response[3] & 0x01U) ? false : true); >> + input_report_key(pad->idev, BTN_START, (pad->response[3] & 0x08U) ? false : true); >> + break; >> + } >> + >> + input_sync(pad->idev); >> + psxpad_setenablemotor(pad, true, true); >> +} >> + >> +#ifdef CONFIG_JOYSTICK_PSXPAD_SPI_FF >> +static int psxpad_spi_ff(struct input_dev *idev, void *data, struct ff_effect *effect) >> +{ >> + struct psxpad *pad = idev->dev.platform_data; > > platform_data usually reserved for platform (board) code, not driver > data. > > struct input_polled_dev *pdev = input_get_drvdata(input); > struct psxpad *pad = pdev->private; Fixed. >> + >> + 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) { >> + dev_err(&pad->idev->dev, "psxpad-spi: init_ff: ff alloc failed!!\n"); >> + err = -ENOMEM; > > Please do not clobber error value returned by input_ff_create_memless(), > pass it on as is. Fixed. >> + } >> + >> + return err; >> +} >> +#else /* CONFIG_JOYSTICK_PSXPAD_SPI_FF */ >> +static inline int psxpad_spi_init_ff(struct psxpad *pad) >> +{ >> + return 0; >> +} >> +#endif /* CONFIG_JOYSTICK_PSXPAD_SPI_FF */ >> + >> +static int psxpad_spi_probe(struct spi_device *spi) >> +{ >> + struct psxpad *pad; >> + struct input_polled_dev *pdev; >> + struct input_dev *idev; >> + int err, i; >> + >> + pad = devm_kzalloc(&spi->dev, sizeof(struct psxpad), GFP_KERNEL); >> + if (!pad) { >> + err = -ENOMEM; >> + goto err_free_mem1; > > Just > return -ENOMEM; Fixed. >> + } >> + pdev = input_allocate_polled_device(); >> + if (!pdev) { >> + dev_err(&spi->dev, "psxpad-spi: probe: pdev alloc failed!!\n"); > > No need for "psxpad-spi: " prefix - dev_err adds driver name to error > messages. > >> >> + err = -ENOMEM; >> + goto err_free_mem1; > > Just > return -ENOMEM; > > devm will take care of freeing "pad" memory. That is is point of devm_* > API. > Fixed. >> + } >> + for (i = 0; i < sizeof(PSX_CMD_POLL); i++) >> + pad->pollcmd[i] = PSX_CMD_POLL[i]; >> + for (i = 0; i < sizeof(PSX_CMD_ENABLE_MOTOR); i++) >> + pad->enablemotor[i] = PSX_CMD_ENABLE_MOTOR[i]; > > I do not think you'll need this anymore if you do what I proposed with > regard to command handling. > Fixed. >> + >> + /* input poll 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; >> + /* poll interval is about 60fps */ >> + pdev->poll_interval = 16; >> + pdev->poll_interval_min = 8; >> + pdev->poll_interval_max = 32; >> + >> + /* input device settings */ >> + idev = pdev->input; >> + pad->idev = idev; >> + idev->name = "PSX (PS1/2) pad"; >> + snprintf(pad->phys, sizeof(pad->phys), "%s/input", dev_name(&spi->dev)); >> + idev->id.bustype = BUS_SPI; >> + idev->dev.parent = &spi->dev; > > Please drop - devm_input_allocate_polled_device should set it up for > you. Fixed. >> + idev->dev.platform_data = pad; > > Please drop. Fixed. >> + >> + /* key/value map settings */ >> + 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); >> + input_set_capability(idev, EV_KEY, BTN_DPAD_UP); >> + input_set_capability(idev, EV_KEY, BTN_DPAD_DOWN); >> + input_set_capability(idev, EV_KEY, BTN_DPAD_LEFT); >> + input_set_capability(idev, EV_KEY, BTN_DPAD_RIGHT); >> + input_set_capability(idev, EV_KEY, BTN_A); >> + input_set_capability(idev, EV_KEY, BTN_B); >> + input_set_capability(idev, EV_KEY, BTN_X); >> + input_set_capability(idev, EV_KEY, BTN_Y); >> + input_set_capability(idev, EV_KEY, BTN_TL); >> + input_set_capability(idev, EV_KEY, BTN_TR); >> + input_set_capability(idev, EV_KEY, BTN_TL2); >> + input_set_capability(idev, EV_KEY, BTN_TR2); >> + input_set_capability(idev, EV_KEY, BTN_THUMBL); >> + input_set_capability(idev, EV_KEY, BTN_THUMBR); >> + input_set_capability(idev, EV_KEY, BTN_SELECT); >> + input_set_capability(idev, EV_KEY, BTN_START); >> + >> + /* force feedback */ >> + err = psxpad_spi_init_ff(pad); >> + if (err) { >> + dev_err(&spi->dev, "psxpad-spi: probe: failed init ff!!\n"); >> + err = -ENOMEM; >> + goto err_free_mem2; > > return err; Fixed. >> + } >> + >> + /* SPI settings */ >> + spi->mode = SPI_MODE_3; >> + spi->bits_per_word = 8; >> + /* (PSX pad might be possible works 250kHz/500kHz) */ >> + spi->master->min_speed_hz = 125000; >> + spi->master->max_speed_hz = 125000; >> + spi_setup(spi); >> + >> + /* pad settings */ >> + psxpad_setmotorlevel(pad, 0, 0); >> + >> + /* register input poll device */ >> + err = input_register_polled_device(pdev); >> + if (err) { >> + dev_err(&spi->dev, "psxpad-spi: probe: failed register!!\n"); >> + goto err_free_mem2; > > return err; Fixed. >> + } >> + >> + pm_runtime_enable(&spi->dev); >> + >> + return 0; >> + > > Code below is not needed. > Fixed. >> + err_free_mem2: >> + input_free_polled_device(pdev); >> + >> + err_free_mem1: >> + devm_kfree(&spi->dev, pad); >> + >> + return err; >> +} >> + >> + >> +static int psxpad_spi_remove(struct spi_device *spi) >> +{ >> + return 0; >> +} > > You can remove this now empty stub. Fixed. >> + >> +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); >> + >> + psxpad_setmotorlevel(pad, 0, 0); >> + >> + return 0; >> +} >> + >> +static int __maybe_unused psxpad_spi_resume(struct device *dev) >> +{ >> + return 0; >> +} > > You can remove this empty stub. Fixed. >> + >> +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("Tomohiro Yoshidomi "); >> +MODULE_DESCRIPTION("PSX (Play Station 1/2) pad with SPI Bus Driver"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.11.0 >> > > Thanks! > > -- > Dmitry Thank you! If you have any problem, please mail me. -- Tomohiro