Message-ID: <CAGeZN5Ov+u0xkvngQx0mdyOfvwuOpLhuxEW4dx27uDgC6C+pUQ@mail.gmail.com>
Date:   Sun, 30 Apr 2017 12:36:46 +0900
From:   Tomohiro Yoshidomi <sylph23k@...il.com>
To:     Tomohiro Yoshidomi <typesylph@...il.com>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6] Input: psxpad-spi - Add PlayStation 1/2 joypads via
 SPI interface Driver

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 <dmitry.torokhov@...il.com>:
> 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 <sylph23k@...il.com>
>
> 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 <sylph23k@...il.com>
>> + * 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 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 <sylph23k@...il.com>");
>> +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

Powered by blists - more mailing lists