Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2997389AbdD2SQR (ORCPT ); Sat, 29 Apr 2017 14:16:17 -0400 Received: from mail-pg0-f66.google.com ([74.125.83.66]:36534 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S938194AbdD2SQI (ORCPT ); Sat, 29 Apr 2017 14:16:08 -0400 Date: Sat, 29 Apr 2017 11:16:04 -0700 From: Dmitry Torokhov To: Tomohiro Yoshidomi Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, sylph23k@gmail.com Subject: Re: [PATCH v5] Input: psxpad-spi - Add PSX (Play Station 1/2) pad SPI driver Message-ID: <20170429181604.GA5562@dtor-ws> References: <20170429094053.26870-1-sylph23k@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170429094053.26870-1-sylph23k@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 18846 Lines: 599 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" > + 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." > + > + 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" > + 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. > +{ > + 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. > + > + 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(). > + > + 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; > + } > + > + 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()? > + > + 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. > +{ > + 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; } ... > + > + 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. > +{ > + 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? > +} > +#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. > +#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). > + 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. > + 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; > + > + 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. > + } > + > + 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; > + } > + 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. > + } > + 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. > + > + /* 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. > + idev->dev.platform_data = pad; Please drop. > + > + /* 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; > + } > + > + /* 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; > + } > + > + pm_runtime_enable(&spi->dev); > + > + return 0; > + Code below is not needed. > + 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. > + > +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. > + > +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