2023-12-15 03:14:07

by Anshul Dalal

[permalink] [raw]
Subject: [PATCH RESEND v11 1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad

Adds bindings for the Adafruit Seesaw Gamepad.

The gamepad functions as an i2c device with the default address of 0x50
and has an IRQ pin that can be enabled in the driver to allow for a rising
edge trigger on each button press or joystick movement.

Product page:
https://www.adafruit.com/product/5743
Arduino driver:
https://github.com/adafruit/Adafruit_Seesaw

Reviewed-by: Conor Dooley <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Anshul Dalal <[email protected]>

---

Changes for v11:
- no updates

Changes for v10:
- Added interrupt-controller/irq.h header

Changes for v9:
- Added interrupt in example

Changes for v8:
- no updates

Changes for v7:
- no updates

Changes for v6:
- no updates

Changes for v5:
- Added link to the datasheet

Changes for v4:
- Fixed the URI for the id field
- Added `interrupts` property

Changes for v3:
- Updated id field to reflect updated file name from previous version
- Added `reg` property

Changes for v2:
- Renamed file to `adafruit,seesaw-gamepad.yaml`
- Removed quotes for `$id` and `$schema`
- Removed "Bindings for" from the description
- Changed node name to the generic name "joystick"
- Changed compatible to 'adafruit,seesaw-gamepad' instead of
'adafruit,seesaw_gamepad'

Previous versions:
v11: https://lore.kernel.org/lkml/[email protected]/
v10: https://lore.kernel.org/lkml/[email protected]/
v9: https://lore.kernel.org/lkml/[email protected]/
v8: https://lore.kernel.org/lkml/[email protected]/
v7: https://lore.kernel.org/lkml/[email protected]/
v6: https://lore.kernel.org/lkml/[email protected]/
v5: https://lore.kernel.org/lkml/[email protected]/
v4: https://lore.kernel.org/lkml/[email protected]/
v3: https://lore.kernel.org/linux-input/[email protected]/
v2: https://lore.kernel.org/linux-input/[email protected]/
v1: https://lore.kernel.org/linux-input/[email protected]/
---
.../input/adafruit,seesaw-gamepad.yaml | 63 +++++++++++++++++++
1 file changed, 63 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml

diff --git a/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
new file mode 100644
index 000000000000..5e86f6de6978
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/adafruit,seesaw-gamepad.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Adafruit Mini I2C Gamepad with seesaw
+
+maintainers:
+ - Anshul Dalal <[email protected]>
+
+description: |
+ Adafruit Mini I2C Gamepad
+
+ +-----------------------------+
+ | ___ |
+ | / \ (X) |
+ | | S | __ __ (Y) (A) |
+ | \___/ |ST| |SE| (B) |
+ | |
+ +-----------------------------+
+
+ S -> 10-bit precision bidirectional analog joystick
+ ST -> Start
+ SE -> Select
+ X, A, B, Y -> Digital action buttons
+
+ Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
+ Product page: https://www.adafruit.com/product/5743
+ Arduino Driver: https://github.com/adafruit/Adafruit_Seesaw
+
+properties:
+ compatible:
+ const: adafruit,seesaw-gamepad
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+ description:
+ The gamepad's IRQ pin triggers a rising edge if interrupts are enabled.
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ joystick@50 {
+ compatible = "adafruit,seesaw-gamepad";
+ interrupts = <18 IRQ_TYPE_EDGE_RISING>;
+ reg = <0x50>;
+ };
+ };
--
2.43.0



2023-12-15 03:14:45

by Anshul Dalal

[permalink] [raw]
Subject: [PATCH RESEND v11 2/2] input: joystick: driver for Adafruit Seesaw Gamepad

Adds a driver for a mini gamepad that communicates over i2c, the gamepad
has bidirectional thumb stick input and six buttons.

The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
to transmit the ADC data for the joystick and digital pin state for the
buttons. I have only implemented the functionality required to receive the
thumb stick and button state.

Steps in reading the gamepad state over i2c:
1. Reset the registers
2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
`BUTTON_MASK`: A bit-map for the six digital pins internally
connected to the joystick buttons.
3. Enable internal pullup resistors for the `BUTTON_MASK`
4. Bulk set the pin state HIGH for `BUTTON_MASK`
5. Poll the device for button and joystick state done by:
`seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`

Product page:
https://www.adafruit.com/product/5743
Arduino driver:
https://github.com/adafruit/Adafruit_Seesaw

Driver tested on RPi Zero 2W

Reviewed-by: Thomas Weißschuh <[email protected]>
Signed-off-by: Anshul Dalal <[email protected]>

---

Changes for v11:
- Removed `of_match_ptr` to prevent warning:
'seesaw_of_table' defined but not used
on non OF platforms

Changes for v10:
- no updates

Changes for v9:
- Added of_device_id

Changes for v8:
- Updated invalid references to `adafruit_seesaw` to `adafruit-seesaw`

Changes for v7:
adafruit-seesaw.c
- Fixed formatting for macro definitions
- Made SEESAW_BUTTON_MASK static const
- Removed __be16 type for x and y fields of seesaw_data
- Used sparse_keymap implementation instead of custom keymap
- Used i2c_msg instead of i2c_master_send and recv in
seesaw_register_read
- Use temporary variable `adc_data` to store data received from ADC
- Changed read_buf from u8[4] to __be32
- Use usleep_range instead of msleep
- Removed 'Reviewed-by: Thomas Weißschuh' due to large number of changes
since last review
Kconfig:
- Added `select INPUT_SPARSEKMAP`

Changes for v6:
- Added TODO
- Removed `clang-format` directives
- Namespaced device buttons
- Removed `char physical_path[32]` field from `struct seesaw_gamepad`
- Added `packed` attribute to `struct seesaw_data`
- Moved from having booleans per button to single `u32 button_state`
- Added `seesaw_button_description` array to directly associate device
buttons with respective keycodes
- Added wrapper functions `seesaw_register_` around `i2c_master_` API
- Ratelimited input error reporting with `dev_err_ratelimited`
- Removed `of_device_id`

Changes for v5:
- Added link to the datasheet
- Added debug log message when `seesaw_read_data` fails

Changes for v4:
- Changed `1UL << BUTTON_` to BIT(BUTTON_)
- Removed `hardware_id` field from `struct seesaw_gamepad`
- Removed redundant checks for the number of bytes written and received by
`i2c_master_send` and `i2c_master_recv`
- Used `get_unaligned_be32` to instantiate `u32 result` from `read_buf`
- Changed `result & (1UL << BUTTON_)` to
`test_bit(BUTTON_, (long *)&result)`
- Changed `KBUILD_MODNAME` in id-tables to `SEESAW_DEVICE_NAME`
- Fixed formatting issues
- Changed button reporting:
Since the gamepad had the action buttons in a non-standard layout:
(X)
(Y) (A)
(B)
Therefore moved to using generic directional action button event codes
instead of BTN_[ABXY].

Changes for v3:
- no updates

Changes for v2:
adafruit-seesaw.c:
- Renamed file from 'adafruit_seesaw.c'
- Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
- Changed count parameter for receiving joystick x on line 118:
`2` to `sizeof(write_buf)`
- Fixed invalid buffer size on line 123 and 126:
`data->y` to `sizeof(data->y)`
- Added comment for the `mdelay(10)` on line 169
- Changed inconsistent indentation on line 271
Kconfig:
- Fixed indentation for the help text
- Updated module name
Makefile:
- Updated module object file name
MAINTAINERS:
- Updated file name for the driver and bindings

Previous versions:
v11: https://lore.kernel.org/lkml/[email protected]/
v10: https://lore.kernel.org/lkml/[email protected]/
v9: https://lore.kernel.org/lkml/[email protected]/
v8: https://lore.kernel.org/lkml/[email protected]/
v7: https://lore.kernel.org/lkml/[email protected]/
v6: https://lore.kernel.org/lkml/[email protected]/
v5: https://lore.kernel.org/lkml/[email protected]/
v4: https://lore.kernel.org/lkml/[email protected]/
v3: https://lore.kernel.org/linux-input/[email protected]/
v2: https://lore.kernel.org/linux-input/[email protected]/
v1: https://lore.kernel.org/linux-input/[email protected]/
---
MAINTAINERS | 7 +
drivers/input/joystick/Kconfig | 10 +
drivers/input/joystick/Makefile | 1 +
drivers/input/joystick/adafruit-seesaw.c | 318 +++++++++++++++++++++++
4 files changed, 336 insertions(+)
create mode 100644 drivers/input/joystick/adafruit-seesaw.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 81d5fc0bba68..b3f101edc24b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
W: https://ez.analog.com/linux-software-drivers
F: drivers/input/touchscreen/ad7879.c

+ADAFRUIT MINI I2C GAMEPAD
+M: Anshul Dalal <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
+F: drivers/input/joystick/adafruit-seesaw.c
+
ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
M: Jiri Kosina <[email protected]>
S: Maintained
diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index ac6925ce8366..7755e5b454d2 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -412,4 +412,14 @@ config JOYSTICK_SENSEHAT
To compile this driver as a module, choose M here: the
module will be called sensehat_joystick.

+config JOYSTICK_SEESAW
+ tristate "Adafruit Mini I2C Gamepad with Seesaw"
+ depends on I2C
+ select INPUT_SPARSEKMAP
+ help
+ Say Y here if you want to use the Adafruit Mini I2C Gamepad.
+
+ To compile this driver as a module, choose M here: the module will be
+ called adafruit-seesaw.
+
endif
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 3937535f0098..9976f596a920 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
+obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit-seesaw.o
obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
new file mode 100644
index 000000000000..0f4a54c9519e
--- /dev/null
+++ b/drivers/input/joystick/adafruit-seesaw.c
@@ -0,0 +1,318 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023 Anshul Dalal <[email protected]>
+ *
+ * Driver for Adafruit Mini I2C Gamepad
+ *
+ * Based on the work of:
+ * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
+ *
+ * Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
+ * Product page: https://www.adafruit.com/product/5743
+ * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
+ *
+ * TODO:
+ * - Add interrupt support
+ */
+
+#include <asm/unaligned.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#define SEESAW_DEVICE_NAME "seesaw-gamepad"
+
+#define SEESAW_STATUS_BASE 0x00
+#define SEESAW_GPIO_BASE 0x01
+#define SEESAW_ADC_BASE 0x09
+
+#define SEESAW_GPIO_DIRCLR_BULK 0x03
+#define SEESAW_GPIO_BULK 0x04
+#define SEESAW_GPIO_BULK_SET 0x05
+#define SEESAW_GPIO_PULLENSET 0x0b
+
+#define SEESAW_STATUS_HW_ID 0x01
+#define SEESAW_STATUS_SWRST 0x7f
+
+#define SEESAW_ADC_OFFSET 0x07
+
+#define SEESAW_BUTTON_A 0x05
+#define SEESAW_BUTTON_B 0x01
+#define SEESAW_BUTTON_X 0x06
+#define SEESAW_BUTTON_Y 0x02
+#define SEESAW_BUTTON_START 0x10
+#define SEESAW_BUTTON_SELECT 0x00
+
+#define SEESAW_ANALOG_X 0x0e
+#define SEESAW_ANALOG_Y 0x0f
+
+#define SEESAW_JOYSTICK_MAX_AXIS 1023
+#define SEESAW_JOYSTICK_FUZZ 2
+#define SEESAW_JOYSTICK_FLAT 4
+
+#define SEESAW_GAMEPAD_POLL_INTERVAL 16
+#define SEESAW_GAMEPAD_POLL_MIN 8
+#define SEESAW_GAMEPAD_POLL_MAX 32
+
+#define MSEC_PER_USEC 1000
+
+static const u32 SEESAW_BUTTON_MASK =
+ BIT(SEESAW_BUTTON_A) | BIT(SEESAW_BUTTON_B) | BIT(SEESAW_BUTTON_X) |
+ BIT(SEESAW_BUTTON_Y) | BIT(SEESAW_BUTTON_START) |
+ BIT(SEESAW_BUTTON_SELECT);
+
+struct seesaw_gamepad {
+ struct input_dev *input_dev;
+ struct i2c_client *i2c_client;
+};
+
+struct seesaw_data {
+ u16 x;
+ u16 y;
+ u32 button_state;
+};
+
+static const struct key_entry seesaw_buttons_new[] = {
+ { KE_KEY, SEESAW_BUTTON_A, .keycode = BTN_SOUTH },
+ { KE_KEY, SEESAW_BUTTON_B, .keycode = BTN_EAST },
+ { KE_KEY, SEESAW_BUTTON_X, .keycode = BTN_NORTH },
+ { KE_KEY, SEESAW_BUTTON_Y, .keycode = BTN_WEST },
+ { KE_KEY, SEESAW_BUTTON_START, .keycode = BTN_START },
+ { KE_KEY, SEESAW_BUTTON_SELECT, .keycode = BTN_SELECT },
+ { KE_END, 0 }
+};
+
+static int seesaw_register_read(struct i2c_client *client, u8 register_high,
+ u8 register_low, char *buf, int count)
+{
+ int ret;
+ u8 register_buf[2] = { register_high, register_low };
+
+ struct i2c_msg message_buf[2] = {
+ {
+ .addr = client->addr,
+ .flags = client->flags,
+ .len = sizeof(register_buf),
+ .buf = register_buf,
+ },
+ {
+ .addr = client->addr,
+ .flags = client->flags | I2C_M_RD,
+ .len = count,
+ .buf = buf,
+ },
+ };
+ ret = i2c_transfer(client->adapter, message_buf,
+ ARRAY_SIZE(message_buf));
+
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int seesaw_register_write_u8(struct i2c_client *client, u8 register_high,
+ u8 register_low, u8 value)
+{
+ int ret;
+ u8 write_buf[3] = { register_high, register_low, value };
+
+ ret = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int seesaw_register_write_u32(struct i2c_client *client,
+ u8 register_high, u8 register_low,
+ u32 value)
+{
+ int ret;
+ u8 write_buf[6] = { register_high, register_low };
+
+ put_unaligned_be32(value, write_buf + 2);
+ ret = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
+{
+ int ret;
+ __be16 adc_data;
+ __be32 read_buf;
+
+ ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
+ (char *)&read_buf, sizeof(read_buf));
+ if (ret)
+ return ret;
+
+ data->button_state = ~be32_to_cpu(read_buf);
+
+ ret = seesaw_register_read(client, SEESAW_ADC_BASE,
+ SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
+ (char *)&adc_data, sizeof(adc_data));
+ if (ret)
+ return ret;
+ /*
+ * ADC reads left as max and right as 0, must be reversed since kernel
+ * expects reports in opposite order.
+ */
+ data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(adc_data);
+
+ ret = seesaw_register_read(client, SEESAW_ADC_BASE,
+ SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
+ (char *)&adc_data, sizeof(adc_data));
+ if (ret)
+ return ret;
+ data->y = be16_to_cpu(adc_data);
+
+ return 0;
+}
+
+static void seesaw_poll(struct input_dev *input)
+{
+ int err, i;
+ struct seesaw_gamepad *private = input_get_drvdata(input);
+ struct seesaw_data data;
+
+ err = seesaw_read_data(private->i2c_client, &data);
+ if (err) {
+ dev_err_ratelimited(&input->dev,
+ "failed to read joystick state: %d\n", err);
+ return;
+ }
+
+ input_report_abs(input, ABS_X, data.x);
+ input_report_abs(input, ABS_Y, data.y);
+
+ for_each_set_bit(i, (long *)&SEESAW_BUTTON_MASK,
+ BITS_PER_TYPE(SEESAW_BUTTON_MASK)) {
+ if (!sparse_keymap_report_event(
+ input, i, data.button_state & BIT(i), false)) {
+ dev_err_ratelimited(&input->dev,
+ "failed to report keymap event");
+ };
+ }
+
+ input_sync(input);
+}
+
+static int seesaw_probe(struct i2c_client *client)
+{
+ int ret;
+ u8 hardware_id;
+ struct seesaw_gamepad *seesaw;
+
+ ret = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
+ SEESAW_STATUS_SWRST, 0xFF);
+ if (ret)
+ return ret;
+
+ /* Wait for the registers to reset before proceeding */
+ usleep_range(10 * MSEC_PER_USEC, 15 * MSEC_PER_USEC);
+
+ seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
+ if (!seesaw)
+ return -ENOMEM;
+
+ ret = seesaw_register_read(client, SEESAW_STATUS_BASE,
+ SEESAW_STATUS_HW_ID, &hardware_id,
+ sizeof(hardware_id));
+ if (ret)
+ return ret;
+
+ dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
+ hardware_id);
+
+ /* Set Pin Mode to input and enable pull-up resistors */
+ ret = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
+ SEESAW_GPIO_DIRCLR_BULK,
+ SEESAW_BUTTON_MASK);
+ if (ret)
+ return ret;
+ ret = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
+ SEESAW_GPIO_PULLENSET,
+ SEESAW_BUTTON_MASK);
+ if (ret)
+ return ret;
+ ret = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
+ SEESAW_GPIO_BULK_SET,
+ SEESAW_BUTTON_MASK);
+ if (ret)
+ return ret;
+
+ seesaw->i2c_client = client;
+ seesaw->input_dev = devm_input_allocate_device(&client->dev);
+ if (!seesaw->input_dev)
+ return -ENOMEM;
+
+ seesaw->input_dev->id.bustype = BUS_I2C;
+ seesaw->input_dev->name = "Adafruit Seesaw Gamepad";
+ seesaw->input_dev->phys = "i2c/" SEESAW_DEVICE_NAME;
+ input_set_drvdata(seesaw->input_dev, seesaw);
+ input_set_abs_params(seesaw->input_dev, ABS_X, 0,
+ SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
+ SEESAW_JOYSTICK_FLAT);
+ input_set_abs_params(seesaw->input_dev, ABS_Y, 0,
+ SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
+ SEESAW_JOYSTICK_FLAT);
+
+ ret = sparse_keymap_setup(seesaw->input_dev, seesaw_buttons_new, NULL);
+ if (ret) {
+ dev_err(&client->dev,
+ "failed to set up input device keymap: %d\n", ret);
+ return ret;
+ }
+
+ ret = input_setup_polling(seesaw->input_dev, seesaw_poll);
+ if (ret) {
+ dev_err(&client->dev, "failed to set up polling: %d\n", ret);
+ return ret;
+ }
+
+ input_set_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_INTERVAL);
+ input_set_max_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MAX);
+ input_set_min_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MIN);
+
+ ret = input_register_device(seesaw->input_dev);
+ if (ret) {
+ dev_err(&client->dev, "failed to register joystick: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct i2c_device_id seesaw_id_table[] = {
+ { SEESAW_DEVICE_NAME },
+ { /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
+
+static const struct of_device_id seesaw_of_table[] = {
+ { .compatible = "adafruit,seesaw-gamepad"},
+ { /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, seesaw_of_table);
+
+static struct i2c_driver seesaw_driver = {
+ .driver = {
+ .name = SEESAW_DEVICE_NAME,
+ .of_match_table = seesaw_of_table,
+ },
+ .id_table = seesaw_id_table,
+ .probe = seesaw_probe,
+};
+module_i2c_driver(seesaw_driver);
+
+MODULE_AUTHOR("Anshul Dalal <[email protected]>");
+MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
+MODULE_LICENSE("GPL");
--
2.43.0


2023-12-27 19:43:33

by Jeff LaBundy

[permalink] [raw]
Subject: Re: [PATCH RESEND v11 2/2] input: joystick: driver for Adafruit Seesaw Gamepad

Hi Anshul,

Looking great so far, just a few more comments.

On Fri, Dec 15, 2023 at 08:41:23AM +0530, Anshul Dalal wrote:
> Adds a driver for a mini gamepad that communicates over i2c, the gamepad
> has bidirectional thumb stick input and six buttons.
>
> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
> to transmit the ADC data for the joystick and digital pin state for the
> buttons. I have only implemented the functionality required to receive the
> thumb stick and button state.
>
> Steps in reading the gamepad state over i2c:
> 1. Reset the registers
> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
> `BUTTON_MASK`: A bit-map for the six digital pins internally
> connected to the joystick buttons.
> 3. Enable internal pullup resistors for the `BUTTON_MASK`
> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
> 5. Poll the device for button and joystick state done by:
> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> Driver tested on RPi Zero 2W
>
> Reviewed-by: Thomas Wei?schuh <[email protected]>
> Signed-off-by: Anshul Dalal <[email protected]>
>
> ---
>
> Changes for v11:
> - Removed `of_match_ptr` to prevent warning:
> 'seesaw_of_table' defined but not used
> on non OF platforms
>
> Changes for v10:
> - no updates
>
> Changes for v9:
> - Added of_device_id
>
> Changes for v8:
> - Updated invalid references to `adafruit_seesaw` to `adafruit-seesaw`
>
> Changes for v7:
> adafruit-seesaw.c
> - Fixed formatting for macro definitions
> - Made SEESAW_BUTTON_MASK static const
> - Removed __be16 type for x and y fields of seesaw_data
> - Used sparse_keymap implementation instead of custom keymap
> - Used i2c_msg instead of i2c_master_send and recv in
> seesaw_register_read
> - Use temporary variable `adc_data` to store data received from ADC
> - Changed read_buf from u8[4] to __be32
> - Use usleep_range instead of msleep
> - Removed 'Reviewed-by: Thomas Wei?schuh' due to large number of changes
> since last review
> Kconfig:
> - Added `select INPUT_SPARSEKMAP`
>
> Changes for v6:
> - Added TODO
> - Removed `clang-format` directives
> - Namespaced device buttons
> - Removed `char physical_path[32]` field from `struct seesaw_gamepad`
> - Added `packed` attribute to `struct seesaw_data`
> - Moved from having booleans per button to single `u32 button_state`
> - Added `seesaw_button_description` array to directly associate device
> buttons with respective keycodes
> - Added wrapper functions `seesaw_register_` around `i2c_master_` API
> - Ratelimited input error reporting with `dev_err_ratelimited`
> - Removed `of_device_id`
>
> Changes for v5:
> - Added link to the datasheet
> - Added debug log message when `seesaw_read_data` fails
>
> Changes for v4:
> - Changed `1UL << BUTTON_` to BIT(BUTTON_)
> - Removed `hardware_id` field from `struct seesaw_gamepad`
> - Removed redundant checks for the number of bytes written and received by
> `i2c_master_send` and `i2c_master_recv`
> - Used `get_unaligned_be32` to instantiate `u32 result` from `read_buf`
> - Changed `result & (1UL << BUTTON_)` to
> `test_bit(BUTTON_, (long *)&result)`
> - Changed `KBUILD_MODNAME` in id-tables to `SEESAW_DEVICE_NAME`
> - Fixed formatting issues
> - Changed button reporting:
> Since the gamepad had the action buttons in a non-standard layout:
> (X)
> (Y) (A)
> (B)
> Therefore moved to using generic directional action button event codes
> instead of BTN_[ABXY].
>
> Changes for v3:
> - no updates
>
> Changes for v2:
> adafruit-seesaw.c:
> - Renamed file from 'adafruit_seesaw.c'
> - Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
> - Changed count parameter for receiving joystick x on line 118:
> `2` to `sizeof(write_buf)`
> - Fixed invalid buffer size on line 123 and 126:
> `data->y` to `sizeof(data->y)`
> - Added comment for the `mdelay(10)` on line 169
> - Changed inconsistent indentation on line 271
> Kconfig:
> - Fixed indentation for the help text
> - Updated module name
> Makefile:
> - Updated module object file name
> MAINTAINERS:
> - Updated file name for the driver and bindings
>
> Previous versions:
> v11: https://lore.kernel.org/lkml/[email protected]/
> v10: https://lore.kernel.org/lkml/[email protected]/
> v9: https://lore.kernel.org/lkml/[email protected]/
> v8: https://lore.kernel.org/lkml/[email protected]/
> v7: https://lore.kernel.org/lkml/[email protected]/
> v6: https://lore.kernel.org/lkml/[email protected]/
> v5: https://lore.kernel.org/lkml/[email protected]/
> v4: https://lore.kernel.org/lkml/[email protected]/
> v3: https://lore.kernel.org/linux-input/[email protected]/
> v2: https://lore.kernel.org/linux-input/[email protected]/
> v1: https://lore.kernel.org/linux-input/[email protected]/
> ---
> MAINTAINERS | 7 +
> drivers/input/joystick/Kconfig | 10 +
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/adafruit-seesaw.c | 318 +++++++++++++++++++++++
> 4 files changed, 336 insertions(+)
> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 81d5fc0bba68..b3f101edc24b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
> W: https://ez.analog.com/linux-software-drivers
> F: drivers/input/touchscreen/ad7879.c
>
> +ADAFRUIT MINI I2C GAMEPAD
> +M: Anshul Dalal <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
> +F: drivers/input/joystick/adafruit-seesaw.c
> +
> ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
> M: Jiri Kosina <[email protected]>
> S: Maintained
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index ac6925ce8366..7755e5b454d2 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -412,4 +412,14 @@ config JOYSTICK_SENSEHAT
> To compile this driver as a module, choose M here: the
> module will be called sensehat_joystick.
>
> +config JOYSTICK_SEESAW
> + tristate "Adafruit Mini I2C Gamepad with Seesaw"
> + depends on I2C
> + select INPUT_SPARSEKMAP
> + help
> + Say Y here if you want to use the Adafruit Mini I2C Gamepad.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called adafruit-seesaw.
> +
> endif
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 3937535f0098..9976f596a920 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
> +obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit-seesaw.o
> obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
> new file mode 100644
> index 000000000000..0f4a54c9519e
> --- /dev/null
> +++ b/drivers/input/joystick/adafruit-seesaw.c
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2023 Anshul Dalal <[email protected]>
> + *
> + * Driver for Adafruit Mini I2C Gamepad
> + *
> + * Based on the work of:
> + * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
> + *
> + * Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
> + * Product page: https://www.adafruit.com/product/5743
> + * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
> + *
> + * TODO:
> + * - Add interrupt support
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
> +
> +#define SEESAW_STATUS_BASE 0x00
> +#define SEESAW_GPIO_BASE 0x01
> +#define SEESAW_ADC_BASE 0x09
> +
> +#define SEESAW_GPIO_DIRCLR_BULK 0x03
> +#define SEESAW_GPIO_BULK 0x04
> +#define SEESAW_GPIO_BULK_SET 0x05
> +#define SEESAW_GPIO_PULLENSET 0x0b
> +
> +#define SEESAW_STATUS_HW_ID 0x01
> +#define SEESAW_STATUS_SWRST 0x7f
> +
> +#define SEESAW_ADC_OFFSET 0x07
> +
> +#define SEESAW_BUTTON_A 0x05
> +#define SEESAW_BUTTON_B 0x01
> +#define SEESAW_BUTTON_X 0x06
> +#define SEESAW_BUTTON_Y 0x02
> +#define SEESAW_BUTTON_START 0x10
> +#define SEESAW_BUTTON_SELECT 0x00
> +
> +#define SEESAW_ANALOG_X 0x0e
> +#define SEESAW_ANALOG_Y 0x0f
> +
> +#define SEESAW_JOYSTICK_MAX_AXIS 1023
> +#define SEESAW_JOYSTICK_FUZZ 2
> +#define SEESAW_JOYSTICK_FLAT 4
> +
> +#define SEESAW_GAMEPAD_POLL_INTERVAL 16
> +#define SEESAW_GAMEPAD_POLL_MIN 8
> +#define SEESAW_GAMEPAD_POLL_MAX 32

Normally we want to include units in #defines that represent actual units,
e.g. SEESAW_GAMEPAD_POLL_INTERVAL_MS.

> +
> +#define MSEC_PER_USEC 1000

I think you meant USEC_PER_MSEC here, which is already defined. But I think
you should just drop it entirely; more on that below.

> +
> +static const u32 SEESAW_BUTTON_MASK =
> + BIT(SEESAW_BUTTON_A) | BIT(SEESAW_BUTTON_B) | BIT(SEESAW_BUTTON_X) |
> + BIT(SEESAW_BUTTON_Y) | BIT(SEESAW_BUTTON_START) |
> + BIT(SEESAW_BUTTON_SELECT);
> +
> +struct seesaw_gamepad {
> + struct input_dev *input_dev;
> + struct i2c_client *i2c_client;
> +};
> +
> +struct seesaw_data {
> + u16 x;
> + u16 y;
> + u32 button_state;
> +};
> +
> +static const struct key_entry seesaw_buttons_new[] = {
> + { KE_KEY, SEESAW_BUTTON_A, .keycode = BTN_SOUTH },
> + { KE_KEY, SEESAW_BUTTON_B, .keycode = BTN_EAST },
> + { KE_KEY, SEESAW_BUTTON_X, .keycode = BTN_NORTH },
> + { KE_KEY, SEESAW_BUTTON_Y, .keycode = BTN_WEST },
> + { KE_KEY, SEESAW_BUTTON_START, .keycode = BTN_START },
> + { KE_KEY, SEESAW_BUTTON_SELECT, .keycode = BTN_SELECT },
> + { KE_END, 0 }
> +};
> +
> +static int seesaw_register_read(struct i2c_client *client, u8 register_high,
> + u8 register_low, char *buf, int count)
> +{
> + int ret;
> + u8 register_buf[2] = { register_high, register_low };
> +

This method of passing "high" and "low" 8-bit registers is unnatural. It
seems this device effectively has a 16-bit register map, so this function
should simply accept a u16 for the register address, and then declare the
following:

__be16 register_buf = cpu_to_be16(register);

Finally, just #define all the register addresses to fit this model instead
of splitting them into a base and offset as the Arduino code appears to do.
For example, the GPIO registers would look like the following:

#define SEESAW_GPIO_BULK 0x0104
#define SEESAW_GPIO_BULK_SET 0x0105
#define SEESAW_GPIO_PULLENSET 0x010b

It seems the ADC and STATUS groups can follow suit as well.

> + struct i2c_msg message_buf[2] = {
> + {
> + .addr = client->addr,
> + .flags = client->flags,
> + .len = sizeof(register_buf),
> + .buf = register_buf,
> + },
> + {
> + .addr = client->addr,
> + .flags = client->flags | I2C_M_RD,
> + .len = count,
> + .buf = buf,

You shouldn't burden all callers of seesaw_register_read() with having to cast
their data values to (char *) outside of this function; it defeats the purpose
of having a helper. Please redefine buf as a *void, then do the following here:

.buf = (u8 *)&buf,

> + },
> + };

Please add a newline between declarations and code.

> + ret = i2c_transfer(client->adapter, message_buf,
> + ARRAY_SIZE(message_buf));
> +

Nit: extraneous newline.

> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int seesaw_register_write_u8(struct i2c_client *client, u8 register_high,
> + u8 register_low, u8 value)
> +{
> + int ret;
> + u8 write_buf[3] = { register_high, register_low, value };

Same idea here. Simply accept a u16 for the register address, and then do
the following:

put_unaligned_be16(register, write_buf);
write_buf[sizeof(register)] = value;

> +
> + ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int seesaw_register_write_u32(struct i2c_client *client,
> + u8 register_high, u8 register_low,
> + u32 value)
> +{
> + int ret;
> + u8 write_buf[6] = { register_high, register_low };
> +
> + put_unaligned_be32(value, write_buf + 2);

And here:

put_unaligned_be16(register, write_buf);
put_unaligned_be32(value, sizeof(register));

> + ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
> +{
> + int ret;
> + __be16 adc_data;
> + __be32 read_buf;
> +
> + ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
> + (char *)&read_buf, sizeof(read_buf));
> + if (ret)
> + return ret;

Normally in the input subsystem, we use 'error' for return values that can only
be zero (success) or a negative error code (failure). Here, we have a mix of 'ret'
and 'err'. It's fine to use 'ret' for the above calls to i2c_* API, but please
use 'error' for the rest.

> +
> + data->button_state = ~be32_to_cpu(read_buf);
> +
> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
> + (char *)&adc_data, sizeof(adc_data));
> + if (ret)
> + return ret;
> + /*
> + * ADC reads left as max and right as 0, must be reversed since kernel
> + * expects reports in opposite order.
> + */
> + data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(adc_data);
> +
> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
> + (char *)&adc_data, sizeof(adc_data));
> + if (ret)
> + return ret;
> + data->y = be16_to_cpu(adc_data);
> +
> + return 0;
> +}
> +
> +static void seesaw_poll(struct input_dev *input)
> +{
> + int err, i;
> + struct seesaw_gamepad *private = input_get_drvdata(input);
> + struct seesaw_data data;
> +
> + err = seesaw_read_data(private->i2c_client, &data);
> + if (err) {
> + dev_err_ratelimited(&input->dev,
> + "failed to read joystick state: %d\n", err);
> + return;
> + }
> +
> + input_report_abs(input, ABS_X, data.x);
> + input_report_abs(input, ABS_Y, data.y);
> +
> + for_each_set_bit(i, (long *)&SEESAW_BUTTON_MASK,
> + BITS_PER_TYPE(SEESAW_BUTTON_MASK)) {
> + if (!sparse_keymap_report_event(
> + input, i, data.button_state & BIT(i), false)) {

This line break is difficult to read; please include at least one variable
on top, like the following:

if (!sparse_keymap_report_event(input, i,
data.button_state & BIT(I),
false))
dev_err_ratelimited(...);

Note that curly braces are not required here.

> + dev_err_ratelimited(&input->dev,
> + "failed to report keymap event");
> + };
> + }
> +
> + input_sync(input);
> +}
> +
> +static int seesaw_probe(struct i2c_client *client)
> +{
> + int ret;
> + u8 hardware_id;
> + struct seesaw_gamepad *seesaw;
> +
> + ret = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
> + SEESAW_STATUS_SWRST, 0xFF);
> + if (ret)
> + return ret;
> +
> + /* Wait for the registers to reset before proceeding */
> + usleep_range(10 * MSEC_PER_USEC, 15 * MSEC_PER_USEC);

It's perfectly fine, and quite common, to simply write these out as 10000
and 15000.

> +
> + seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
> + if (!seesaw)
> + return -ENOMEM;
> +
> + ret = seesaw_register_read(client, SEESAW_STATUS_BASE,
> + SEESAW_STATUS_HW_ID, &hardware_id,
> + sizeof(hardware_id));
> + if (ret)
> + return ret;
> +
> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
> + hardware_id);
> +
> + /* Set Pin Mode to input and enable pull-up resistors */
> + ret = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_DIRCLR_BULK,
> + SEESAW_BUTTON_MASK);
> + if (ret)
> + return ret;
> + ret = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_PULLENSET,
> + SEESAW_BUTTON_MASK);
> + if (ret)
> + return ret;
> + ret = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_BULK_SET,
> + SEESAW_BUTTON_MASK);
> + if (ret)
> + return ret;
> +
> + seesaw->i2c_client = client;
> + seesaw->input_dev = devm_input_allocate_device(&client->dev);
> + if (!seesaw->input_dev)
> + return -ENOMEM;
> +
> + seesaw->input_dev->id.bustype = BUS_I2C;
> + seesaw->input_dev->name = "Adafruit Seesaw Gamepad";
> + seesaw->input_dev->phys = "i2c/" SEESAW_DEVICE_NAME;
> + input_set_drvdata(seesaw->input_dev, seesaw);
> + input_set_abs_params(seesaw->input_dev, ABS_X, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> + input_set_abs_params(seesaw->input_dev, ABS_Y, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> +
> + ret = sparse_keymap_setup(seesaw->input_dev, seesaw_buttons_new, NULL);
> + if (ret) {
> + dev_err(&client->dev,
> + "failed to set up input device keymap: %d\n", ret);
> + return ret;
> + }
> +
> + ret = input_setup_polling(seesaw->input_dev, seesaw_poll);
> + if (ret) {
> + dev_err(&client->dev, "failed to set up polling: %d\n", ret);
> + return ret;
> + }
> +
> + input_set_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_INTERVAL);
> + input_set_max_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MAX);
> + input_set_min_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MIN);
> +
> + ret = input_register_device(seesaw->input_dev);
> + if (ret) {
> + dev_err(&client->dev, "failed to register joystick: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id seesaw_id_table[] = {
> + { SEESAW_DEVICE_NAME },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
> +
> +static const struct of_device_id seesaw_of_table[] = {
> + { .compatible = "adafruit,seesaw-gamepad"},
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, seesaw_of_table);
> +
> +static struct i2c_driver seesaw_driver = {
> + .driver = {
> + .name = SEESAW_DEVICE_NAME,
> + .of_match_table = seesaw_of_table,
> + },
> + .id_table = seesaw_id_table,
> + .probe = seesaw_probe,
> +};
> +module_i2c_driver(seesaw_driver);
> +
> +MODULE_AUTHOR("Anshul Dalal <[email protected]>");
> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
> +MODULE_LICENSE("GPL");
> --
> 2.43.0
>

Kind regards,
Jeff LaBundy