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 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'
.../input/adafruit,seesaw-gamepad.yaml | 60 +++++++++++++++++++
1 file changed, 60 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..3f0d1c5a3b9b
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
@@ -0,0 +1,60 @@
+# 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 percision 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:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ joystick@50 {
+ compatible = "adafruit,seesaw-gamepad";
+ reg = <0x50>;
+ };
+ };
--
2.42.0
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 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
MAINTAINERS | 7 +
drivers/input/joystick/Kconfig | 9 +
drivers/input/joystick/Makefile | 1 +
drivers/input/joystick/adafruit-seesaw.c | 310 +++++++++++++++++++++++
4 files changed, 327 insertions(+)
create mode 100644 drivers/input/joystick/adafruit-seesaw.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 4cc6bf79fdd8..0595c832c248 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..df9cd1830b29 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -412,4 +412,13 @@ 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
+ 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..1aa6fbe4fda4
--- /dev/null
+++ b/drivers/input/joystick/adafruit-seesaw.c
@@ -0,0 +1,310 @@
+// 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-generic/unaligned.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#define SEESAW_DEVICE_NAME "seesaw-gamepad"
+
+#define SEESAW_STATUS_BASE 0
+#define SEESAW_GPIO_BASE 1
+#define SEESAW_ADC_BASE 9
+
+#define SEESAW_GPIO_DIRCLR_BULK 3
+#define SEESAW_GPIO_BULK 4
+#define SEESAW_GPIO_BULK_SET 5
+#define SEESAW_GPIO_PULLENSET 11
+
+#define SEESAW_STATUS_HW_ID 1
+#define SEESAW_STATUS_SWRST 127
+
+#define SEESAW_ADC_OFFSET 7
+
+#define SEESAW_BUTTON_A 5
+#define SEESAW_BUTTON_B 1
+#define SEESAW_BUTTON_X 6
+#define SEESAW_BUTTON_Y 2
+#define SEESAW_BUTTON_START 16
+#define SEESAW_BUTTON_SELECT 0
+
+#define SEESAW_ANALOG_X 14
+#define SEESAW_ANALOG_Y 15
+
+#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
+
+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 {
+ __be16 x;
+ __be16 y;
+ u32 button_state;
+} __packed;
+
+struct seesaw_button_description {
+ unsigned int code;
+ unsigned int bit;
+};
+
+static const struct seesaw_button_description seesaw_buttons[] = {
+ {
+ .code = BTN_EAST,
+ .bit = SEESAW_BUTTON_A,
+ },
+ {
+ .code = BTN_SOUTH,
+ .bit = SEESAW_BUTTON_B,
+ },
+ {
+ .code = BTN_NORTH,
+ .bit = SEESAW_BUTTON_X,
+ },
+ {
+ .code = BTN_WEST,
+ .bit = SEESAW_BUTTON_Y,
+ },
+ {
+ .code = BTN_START,
+ .bit = SEESAW_BUTTON_START,
+ },
+ {
+ .code = BTN_SELECT,
+ .bit = SEESAW_BUTTON_SELECT,
+ },
+};
+
+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 };
+
+ ret = i2c_master_send(client, register_buf, sizeof(register_buf));
+ if (ret < 0)
+ return ret;
+ ret = i2c_master_recv(client, buf, count);
+ 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;
+ u8 read_buf[4];
+
+ ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
+ read_buf, sizeof(read_buf));
+ if (ret)
+ return ret;
+
+ data->button_state = ~get_unaligned_be32(&read_buf);
+
+ ret = seesaw_register_read(client, SEESAW_ADC_BASE,
+ SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
+ (char *)&data->x, sizeof(data->x));
+ 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(data->x);
+
+ ret = seesaw_register_read(client, SEESAW_ADC_BASE,
+ SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
+ (char *)&data->y, sizeof(data->y));
+ if (ret)
+ return ret;
+ data->y = be16_to_cpu(data->y);
+
+ 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 != 0) {
+ 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 (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
+ input_report_key(input, seesaw_buttons[i].code,
+ data.button_state &
+ BIT(seesaw_buttons[i].bit));
+ }
+ input_sync(input);
+}
+
+static int seesaw_probe(struct i2c_client *client)
+{
+ int err, i;
+ u8 hardware_id;
+ struct seesaw_gamepad *seesaw;
+
+ err = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
+ SEESAW_STATUS_SWRST, 0xFF);
+ if (err)
+ return err;
+
+ /* Wait for the registers to reset before proceeding */
+ mdelay(10);
+
+ seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
+ if (!seesaw)
+ return -ENOMEM;
+
+ err = seesaw_register_read(client, SEESAW_STATUS_BASE,
+ SEESAW_STATUS_HW_ID, &hardware_id, 1);
+ if (err)
+ return err;
+
+ dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
+ hardware_id);
+
+ /* Set Pin Mode to input and enable pull-up resistors */
+ err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
+ SEESAW_GPIO_DIRCLR_BULK,
+ SEESAW_BUTTON_MASK);
+ if (err)
+ return err;
+ err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
+ SEESAW_GPIO_PULLENSET,
+ SEESAW_BUTTON_MASK);
+ if (err)
+ return err;
+ err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
+ SEESAW_GPIO_BULK_SET,
+ SEESAW_BUTTON_MASK);
+ if (err)
+ return err;
+
+ seesaw->i2c_client = client;
+ i2c_set_clientdata(client, seesaw);
+
+ 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);
+ for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
+ input_set_capability(seesaw->input_dev, EV_KEY,
+ seesaw_buttons[i].code);
+ }
+
+ err = input_setup_polling(seesaw->input_dev, seesaw_poll);
+ if (err) {
+ dev_err(&client->dev, "failed to set up polling: %d\n", err);
+ return err;
+ }
+
+ 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);
+
+ err = input_register_device(seesaw->input_dev);
+ if (err) {
+ dev_err(&client->dev, "failed to register joystick: %d\n", err);
+ return err;
+ }
+
+ return 0;
+}
+
+static const struct i2c_device_id seesaw_id_table[] = {
+ { SEESAW_DEVICE_NAME, 0 },
+ { /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
+
+static struct i2c_driver seesaw_driver = {
+ .driver = {
+ .name = SEESAW_DEVICE_NAME,
+ },
+ .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.42.0
Hi Anshul,
thanks for the reworks!
Some more comments inline.
On 2023-10-27 10:48:11+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 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
>
> MAINTAINERS | 7 +
> drivers/input/joystick/Kconfig | 9 +
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/adafruit-seesaw.c | 310 +++++++++++++++++++++++
> 4 files changed, 327 insertions(+)
> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
[..]
> diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
> new file mode 100644
> index 000000000000..1aa6fbe4fda4
> --- /dev/null
> +++ b/drivers/input/joystick/adafruit-seesaw.c
> @@ -0,0 +1,310 @@
> +// 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-generic/unaligned.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
> +
> +#define SEESAW_STATUS_BASE 0
> +#define SEESAW_GPIO_BASE 1
> +#define SEESAW_ADC_BASE 9
> +
> +#define SEESAW_GPIO_DIRCLR_BULK 3
> +#define SEESAW_GPIO_BULK 4
> +#define SEESAW_GPIO_BULK_SET 5
> +#define SEESAW_GPIO_PULLENSET 11
> +
> +#define SEESAW_STATUS_HW_ID 1
> +#define SEESAW_STATUS_SWRST 127
> +
> +#define SEESAW_ADC_OFFSET 7
> +
> +#define SEESAW_BUTTON_A 5
> +#define SEESAW_BUTTON_B 1
> +#define SEESAW_BUTTON_X 6
> +#define SEESAW_BUTTON_Y 2
> +#define SEESAW_BUTTON_START 16
> +#define SEESAW_BUTTON_SELECT 0
> +
> +#define SEESAW_ANALOG_X 14
> +#define SEESAW_ANALOG_Y 15
> +
> +#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
> +
> +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 {
> + __be16 x;
> + __be16 y;
The fact that these are big endian is now an implementation detail
hidden within seesaw_read_data(), so the __be16 can go away.
> + u32 button_state;
> +} __packed;
While this was requested by Jeff I don't think it's correct.
The struct is never received in this form from the device.
I guess he also got confused, like me, by the fact that data is directly
read into the fields of the struct.
See my comment seesaw_read_data().
> +struct seesaw_button_description {
> + unsigned int code;
> + unsigned int bit;
> +};
> +
> +static const struct seesaw_button_description seesaw_buttons[] = {
> + {
> + .code = BTN_EAST,
> + .bit = SEESAW_BUTTON_A,
> + },
> + {
> + .code = BTN_SOUTH,
> + .bit = SEESAW_BUTTON_B,
> + },
> + {
> + .code = BTN_NORTH,
> + .bit = SEESAW_BUTTON_X,
> + },
> + {
> + .code = BTN_WEST,
> + .bit = SEESAW_BUTTON_Y,
> + },
> + {
> + .code = BTN_START,
> + .bit = SEESAW_BUTTON_START,
> + },
> + {
> + .code = BTN_SELECT,
> + .bit = SEESAW_BUTTON_SELECT,
> + },
> +};
This looks very much like a sparse keymap which can be implemented with
the helpers from <linux/input/sparse-keymap.h>.
Personally I prefer each keymap entry to be on a single line (without
designated initializers, maybe with some alignment) to save a bunch of
vertical space.
> +
> +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 };
> +
> + ret = i2c_master_send(client, register_buf, sizeof(register_buf));
> + if (ret < 0)
> + return ret;
> + ret = i2c_master_recv(client, buf, count);
> + 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;
> + u8 read_buf[4];
> +
> + ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
> + read_buf, sizeof(read_buf));
> + if (ret)
> + return ret;
> +
> + data->button_state = ~get_unaligned_be32(&read_buf);
> +
> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
> + (char *)&data->x, sizeof(data->x));
Nitpick: read into a dedicated local variable instead of 'data', as it's
easier to understand.
> + 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(data->x);
> +
> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
> + (char *)&data->y, sizeof(data->y));
> + if (ret)
> + return ret;
> + data->y = be16_to_cpu(data->y);
> +
> + 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 != 0) {
Everywhere else this is just '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 (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
> + input_report_key(input, seesaw_buttons[i].code,
> + data.button_state &
> + BIT(seesaw_buttons[i].bit));
> + }
> + input_sync(input);
> +}
> +
> +static int seesaw_probe(struct i2c_client *client)
> +{
> + int err, i;
> + u8 hardware_id;
> + struct seesaw_gamepad *seesaw;
> +
> + err = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
> + SEESAW_STATUS_SWRST, 0xFF);
> + if (err)
> + return err;
> +
> + /* Wait for the registers to reset before proceeding */
> + mdelay(10);
> +
> + seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
> + if (!seesaw)
> + return -ENOMEM;
> +
> + err = seesaw_register_read(client, SEESAW_STATUS_BASE,
> + SEESAW_STATUS_HW_ID, &hardware_id, 1);
> + if (err)
> + return err;
> +
> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
> + hardware_id);
> +
> + /* Set Pin Mode to input and enable pull-up resistors */
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_DIRCLR_BULK,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_PULLENSET,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_BULK_SET,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> +
> + seesaw->i2c_client = client;
> + i2c_set_clientdata(client, seesaw);
I'm not familiar with the i2c APIs but this clientdata seems to be
unused.
> +
> + 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);
> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
> + input_set_capability(seesaw->input_dev, EV_KEY,
> + seesaw_buttons[i].code);
> + }
> +
> + err = input_setup_polling(seesaw->input_dev, seesaw_poll);
> + if (err) {
> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
> + return err;
> + }
> +
> + input_set_poll_interval(seesaw->input_dev,
> + SEESAW_GAMEPAD_POLL_INTERVAL);
Why the linebreak on this line and not on the ones below?
> + input_set_max_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MAX);
> + input_set_min_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MIN);
> +
> + err = input_register_device(seesaw->input_dev);
> + if (err) {
> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id seesaw_id_table[] = {
> + { SEESAW_DEVICE_NAME, 0 },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
> +
> +static struct i2c_driver seesaw_driver = {
> + .driver = {
> + .name = SEESAW_DEVICE_NAME,
> + },
> + .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.42.0
>
Hi Anshul,
On Fri, Oct 27, 2023 at 10:48:11AM +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 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
>
> MAINTAINERS | 7 +
> drivers/input/joystick/Kconfig | 9 +
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/adafruit-seesaw.c | 310 +++++++++++++++++++++++
> 4 files changed, 327 insertions(+)
> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4cc6bf79fdd8..0595c832c248 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..df9cd1830b29 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -412,4 +412,13 @@ 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
> + 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..1aa6fbe4fda4
> --- /dev/null
> +++ b/drivers/input/joystick/adafruit-seesaw.c
> @@ -0,0 +1,310 @@
> +// 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-generic/unaligned.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
> +
> +#define SEESAW_STATUS_BASE 0
> +#define SEESAW_GPIO_BASE 1
> +#define SEESAW_ADC_BASE 9
> +
> +#define SEESAW_GPIO_DIRCLR_BULK 3
> +#define SEESAW_GPIO_BULK 4
> +#define SEESAW_GPIO_BULK_SET 5
> +#define SEESAW_GPIO_PULLENSET 11
> +
> +#define SEESAW_STATUS_HW_ID 1
> +#define SEESAW_STATUS_SWRST 127
> +
> +#define SEESAW_ADC_OFFSET 7
> +
> +#define SEESAW_BUTTON_A 5
> +#define SEESAW_BUTTON_B 1
> +#define SEESAW_BUTTON_X 6
> +#define SEESAW_BUTTON_Y 2
> +#define SEESAW_BUTTON_START 16
> +#define SEESAW_BUTTON_SELECT 0
> +
> +#define SEESAW_ANALOG_X 14
> +#define SEESAW_ANALOG_Y 15
> +
> +#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
> +
> +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 {
> + __be16 x;
> + __be16 y;
> + u32 button_state;
> +} __packed;
> +
> +struct seesaw_button_description {
> + unsigned int code;
> + unsigned int bit;
> +};
> +
> +static const struct seesaw_button_description seesaw_buttons[] = {
> + {
> + .code = BTN_EAST,
> + .bit = SEESAW_BUTTON_A,
> + },
> + {
> + .code = BTN_SOUTH,
> + .bit = SEESAW_BUTTON_B,
> + },
> + {
> + .code = BTN_NORTH,
> + .bit = SEESAW_BUTTON_X,
> + },
> + {
> + .code = BTN_WEST,
> + .bit = SEESAW_BUTTON_Y,
> + },
> + {
> + .code = BTN_START,
> + .bit = SEESAW_BUTTON_START,
> + },
> + {
> + .code = BTN_SELECT,
> + .bit = SEESAW_BUTTON_SELECT,
> + },
> +};
> +
> +static int seesaw_register_read(struct i2c_client *client, u8 register_high,
> + u8 register_low, char *buf, int count)
I am curious why we have 2 u8s instead of u16 that we send as __be16?
> +{
> + int ret;
> + u8 register_buf[2] = { register_high, register_low };
> +
> + ret = i2c_master_send(client, register_buf, sizeof(register_buf));
> + if (ret < 0)
> + return ret;
> + ret = i2c_master_recv(client, buf, count);
I am curious can this be an i2c_transfer() with read/write messages
instead (so 1 transaction)?
> + 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;
> + u8 read_buf[4];
Why is this u8 buffer and not __be32?
> +
> + ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
> + read_buf, sizeof(read_buf));
> + if (ret)
> + return ret;
> +
> + data->button_state = ~get_unaligned_be32(&read_buf);
If you use __be32 you would not need to use get_unaligned_be32.
> +
> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
> + (char *)&data->x, sizeof(data->x));
> + 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(data->x);
> +
> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
> + (char *)&data->y, sizeof(data->y));
> + if (ret)
> + return ret;
> + data->y = be16_to_cpu(data->y);
This is endianness violation and sparse should have warned you about
this. A variable can either carry BE data, LE data, or CPU-endianness
data, but not both. I'd recommend combining seesaw_read_data() with
seesaw_poll() into something like seesaw_report_events() and using
temporaries for x and y and button data, and do not store them in the
private structure at all.
> +
> + 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 != 0) {
> + 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 (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
> + input_report_key(input, seesaw_buttons[i].code,
> + data.button_state &
> + BIT(seesaw_buttons[i].bit));
> + }
> + input_sync(input);
> +}
> +
> +static int seesaw_probe(struct i2c_client *client)
> +{
> + int err, i;
> + u8 hardware_id;
> + struct seesaw_gamepad *seesaw;
> +
> + err = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
> + SEESAW_STATUS_SWRST, 0xFF);
> + if (err)
> + return err;
> +
> + /* Wait for the registers to reset before proceeding */
> + mdelay(10);
Can this be usleep_range() instead? No need to block CPU.
> +
> + seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
> + if (!seesaw)
> + return -ENOMEM;
> +
> + err = seesaw_register_read(client, SEESAW_STATUS_BASE,
> + SEESAW_STATUS_HW_ID, &hardware_id, 1);
sizeof(hardware_id)
> + if (err)
> + return err;
> +
> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
> + hardware_id);
> +
> + /* Set Pin Mode to input and enable pull-up resistors */
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_DIRCLR_BULK,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_PULLENSET,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_BULK_SET,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> +
> + seesaw->i2c_client = client;
> + i2c_set_clientdata(client, seesaw);
> +
> + 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);
> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
> + input_set_capability(seesaw->input_dev, EV_KEY,
> + seesaw_buttons[i].code);
> + }
> +
> + err = input_setup_polling(seesaw->input_dev, seesaw_poll);
> + if (err) {
> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
> + return err;
> + }
> +
> + 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);
> +
> + err = input_register_device(seesaw->input_dev);
> + if (err) {
> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id seesaw_id_table[] = {
> + { SEESAW_DEVICE_NAME, 0 },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
> +
> +static struct i2c_driver seesaw_driver = {
> + .driver = {
> + .name = SEESAW_DEVICE_NAME,
> + },
> + .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.42.0
>
Thanks.
--
Dmitry
Hello Dmitry,
Thanks for the review, the requested changes will be added in the next
patch version. I have added a few comments below:
On 10/30/23 12:26, Dmitry Torokhov wrote:
> Hi Anshul,
>
> On Fri, Oct 27, 2023 at 10:48:11AM +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 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
>>
>> MAINTAINERS | 7 +
>> drivers/input/joystick/Kconfig | 9 +
>> drivers/input/joystick/Makefile | 1 +
>> drivers/input/joystick/adafruit-seesaw.c | 310 +++++++++++++++++++++++
>> 4 files changed, 327 insertions(+)
>> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 4cc6bf79fdd8..0595c832c248 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..df9cd1830b29 100644
>> --- a/drivers/input/joystick/Kconfig
>> +++ b/drivers/input/joystick/Kconfig
>> @@ -412,4 +412,13 @@ 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
>> + 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..1aa6fbe4fda4
>> --- /dev/null
>> +++ b/drivers/input/joystick/adafruit-seesaw.c
>> @@ -0,0 +1,310 @@
>> +// 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-generic/unaligned.h>
>> +#include <linux/bits.h>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/input.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
>> +
>> +#define SEESAW_STATUS_BASE 0
>> +#define SEESAW_GPIO_BASE 1
>> +#define SEESAW_ADC_BASE 9
>> +
>> +#define SEESAW_GPIO_DIRCLR_BULK 3
>> +#define SEESAW_GPIO_BULK 4
>> +#define SEESAW_GPIO_BULK_SET 5
>> +#define SEESAW_GPIO_PULLENSET 11
>> +
>> +#define SEESAW_STATUS_HW_ID 1
>> +#define SEESAW_STATUS_SWRST 127
>> +
>> +#define SEESAW_ADC_OFFSET 7
>> +
>> +#define SEESAW_BUTTON_A 5
>> +#define SEESAW_BUTTON_B 1
>> +#define SEESAW_BUTTON_X 6
>> +#define SEESAW_BUTTON_Y 2
>> +#define SEESAW_BUTTON_START 16
>> +#define SEESAW_BUTTON_SELECT 0
>> +
>> +#define SEESAW_ANALOG_X 14
>> +#define SEESAW_ANALOG_Y 15
>> +
>> +#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
>> +
>> +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 {
>> + __be16 x;
>> + __be16 y;
>> + u32 button_state;
>> +} __packed;
>> +
>> +struct seesaw_button_description {
>> + unsigned int code;
>> + unsigned int bit;
>> +};
>> +
>> +static const struct seesaw_button_description seesaw_buttons[] = {
>> + {
>> + .code = BTN_EAST,
>> + .bit = SEESAW_BUTTON_A,
>> + },
>> + {
>> + .code = BTN_SOUTH,
>> + .bit = SEESAW_BUTTON_B,
>> + },
>> + {
>> + .code = BTN_NORTH,
>> + .bit = SEESAW_BUTTON_X,
>> + },
>> + {
>> + .code = BTN_WEST,
>> + .bit = SEESAW_BUTTON_Y,
>> + },
>> + {
>> + .code = BTN_START,
>> + .bit = SEESAW_BUTTON_START,
>> + },
>> + {
>> + .code = BTN_SELECT,
>> + .bit = SEESAW_BUTTON_SELECT,
>> + },
>> +};
>> +
>> +static int seesaw_register_read(struct i2c_client *client, u8 register_high,
>> + u8 register_low, char *buf, int count)
>
> I am curious why we have 2 u8s instead of u16 that we send as __be16?
>
That's done to be consistent with the manufacturer's implementation of
the seesaw framework from the Arduino driver:
bool read(uint8_t regHigh, uint8_t regLow, uint8_t *buf, uint8_t num,
uint16_t delay = 250);
The spec uses register_high as a namespace for the actual register you
want to work with: register_low.
For example when reading for the hardware id of the device, we have
`SEESAW_STATUS_BASE` [0x00] as the register_high and
`SEESAW_STATUS_HW_ID` [0x01] as the register_low which could also be
`SEESAW_STATUS_VERSION` [0x02] if instead wanted to get the framework
version the device is running.
Changing the register_high to say `SEESAW_TIMER_BASE` [0x08] and
register_low to `SEESAW_TIMER_FREQ` [0x02] would now have the chip
output the timer frequency.
>> +{
>> + int ret;
>> + u8 register_buf[2] = { register_high, register_low };
>> +
>> + ret = i2c_master_send(client, register_buf, sizeof(register_buf));
>> + if (ret < 0)
>> + return ret;
>> + ret = i2c_master_recv(client, buf, count);
>
> I am curious can this be an i2c_transfer() with read/write messages
> instead (so 1 transaction)?
>
Yes! here's what that could look like:
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;
I prefer this to the prior, let me know if I should go ahead with adding
this change.
>> + 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;
>> + u8 read_buf[4];
>
> Why is this u8 buffer and not __be32?
>
>> +
>> + ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
>> + read_buf, sizeof(read_buf));
>> + if (ret)
>> + return ret;
>> +
>> + data->button_state = ~get_unaligned_be32(&read_buf);
>
> If you use __be32 you would not need to use get_unaligned_be32.
>
In my testing on a Raspberry Pi Zero 2 W (arm64), that
get_unaligned_be32 still seems to be required even with read_buf as __be32.
>
>> +
>> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
>> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
>> + (char *)&data->x, sizeof(data->x));
>> + 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(data->x);
>> +
>> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
>> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
>> + (char *)&data->y, sizeof(data->y));
>> + if (ret)
>> + return ret;
>> + data->y = be16_to_cpu(data->y);
>
> This is endianness violation and sparse should have warned you about
> this. A variable can either carry BE data, LE data, or CPU-endianness
> data, but not both.
Would reading the data into an __be16 and then using be16_to_cpu() to
assign it to data->y and data->x be an acceptable solution?
> I'd recommend combining seesaw_read_data() with
> seesaw_poll() into something like seesaw_report_events() and using
> temporaries for x and y and button data, and do not store them in the
> private structure at all.
>
The `struct seesaw_data` here is already temporary in seesaw_poll()
which then gets populated by seesaw_read_data(). I think the separation
of both functions is a better approach since it provides a convenient
error handler in case anything with the hardware goes wrong as:
err = seesaw_read_data(private->i2c_client, &data);
if (err) {
dev_err_ratelimited(&input->dev,
"failed to read joystick state: %d\n", err);
return;
}
>> +
>> + 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 != 0) {
>> + 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 (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
>> + input_report_key(input, seesaw_buttons[i].code,
>> + data.button_state &
>> + BIT(seesaw_buttons[i].bit));
>> + }
>> + input_sync(input);
>> +}
>> +
>> +static int seesaw_probe(struct i2c_client *client)
>> +{
>> + int err, i;
>> + u8 hardware_id;
>> + struct seesaw_gamepad *seesaw;
>> +
>> + err = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
>> + SEESAW_STATUS_SWRST, 0xFF);
>> + if (err)
>> + return err;
>> +
>> + /* Wait for the registers to reset before proceeding */
>> + mdelay(10);
>
> Can this be usleep_range() instead? No need to block CPU.
>
>> +
>> + seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
>> + if (!seesaw)
>> + return -ENOMEM;
>> +
>> + err = seesaw_register_read(client, SEESAW_STATUS_BASE,
>> + SEESAW_STATUS_HW_ID, &hardware_id, 1);
>
> sizeof(hardware_id)
>
>> + if (err)
>> + return err;
>> +
>> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
>> + hardware_id);
>> +
>> + /* Set Pin Mode to input and enable pull-up resistors */
>> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
>> + SEESAW_GPIO_DIRCLR_BULK,
>> + SEESAW_BUTTON_MASK);
>> + if (err)
>> + return err;
>> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
>> + SEESAW_GPIO_PULLENSET,
>> + SEESAW_BUTTON_MASK);
>> + if (err)
>> + return err;
>> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
>> + SEESAW_GPIO_BULK_SET,
>> + SEESAW_BUTTON_MASK);
>> + if (err)
>> + return err;
>> +
>> + seesaw->i2c_client = client;
>> + i2c_set_clientdata(client, seesaw);
>> +
>> + 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);
>> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
>> + input_set_capability(seesaw->input_dev, EV_KEY,
>> + seesaw_buttons[i].code);
>> + }
>> +
>> + err = input_setup_polling(seesaw->input_dev, seesaw_poll);
>> + if (err) {
>> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
>> + return err;
>> + }
>> +
>> + 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);
>> +
>> + err = input_register_device(seesaw->input_dev);
>> + if (err) {
>> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
>> + return err;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id seesaw_id_table[] = {
>> + { SEESAW_DEVICE_NAME, 0 },
>> + { /* Sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
>> +
>> +static struct i2c_driver seesaw_driver = {
>> + .driver = {
>> + .name = SEESAW_DEVICE_NAME,
>> + },
>> + .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.42.0
>>
>
> Thanks.
>
Hello Thomas,
Thanks for the review! The requested changes will be addressed in the
next patch version though I had a few comments below:
On 10/27/23 11:44, Thomas Weißschuh wrote:
> Hi Anshul,
>
> thanks for the reworks!
>
> Some more comments inline.
>
> On 2023-10-27 10:48:11+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 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
>>
>> MAINTAINERS | 7 +
>> drivers/input/joystick/Kconfig | 9 +
>> drivers/input/joystick/Makefile | 1 +
>> drivers/input/joystick/adafruit-seesaw.c | 310 +++++++++++++++++++++++
>> 4 files changed, 327 insertions(+)
>> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
>
> [..]
>
>> diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
>> new file mode 100644
>> index 000000000000..1aa6fbe4fda4
>> --- /dev/null
>> +++ b/drivers/input/joystick/adafruit-seesaw.c
>> @@ -0,0 +1,310 @@
>> +// 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-generic/unaligned.h>
>> +#include <linux/bits.h>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/input.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
>> +
>> +#define SEESAW_STATUS_BASE 0
>> +#define SEESAW_GPIO_BASE 1
>> +#define SEESAW_ADC_BASE 9
>> +
>> +#define SEESAW_GPIO_DIRCLR_BULK 3
>> +#define SEESAW_GPIO_BULK 4
>> +#define SEESAW_GPIO_BULK_SET 5
>> +#define SEESAW_GPIO_PULLENSET 11
>> +
>> +#define SEESAW_STATUS_HW_ID 1
>> +#define SEESAW_STATUS_SWRST 127
>> +
>> +#define SEESAW_ADC_OFFSET 7
>> +
>> +#define SEESAW_BUTTON_A 5
>> +#define SEESAW_BUTTON_B 1
>> +#define SEESAW_BUTTON_X 6
>> +#define SEESAW_BUTTON_Y 2
>> +#define SEESAW_BUTTON_START 16
>> +#define SEESAW_BUTTON_SELECT 0
>> +
>> +#define SEESAW_ANALOG_X 14
>> +#define SEESAW_ANALOG_Y 15
>> +
>> +#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
>> +
>> +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 {
>> + __be16 x;
>> + __be16 y;
>
> The fact that these are big endian is now an implementation detail
> hidden within seesaw_read_data(), so the __be16 can go away.
>
>> + u32 button_state;
>> +} __packed;
>
> While this was requested by Jeff I don't think it's correct.
> The struct is never received in this form from the device.
> I guess he also got confused, like me, by the fact that data is directly
> read into the fields of the struct.
>
> See my comment seesaw_read_data().
>
>> +struct seesaw_button_description {
>> + unsigned int code;
>> + unsigned int bit;
>> +};
>> +
>> +static const struct seesaw_button_description seesaw_buttons[] = {
>> + {
>> + .code = BTN_EAST,
>> + .bit = SEESAW_BUTTON_A,
>> + },
>> + {
>> + .code = BTN_SOUTH,
>> + .bit = SEESAW_BUTTON_B,
>> + },
>> + {
>> + .code = BTN_NORTH,
>> + .bit = SEESAW_BUTTON_X,
>> + },
>> + {
>> + .code = BTN_WEST,
>> + .bit = SEESAW_BUTTON_Y,
>> + },
>> + {
>> + .code = BTN_START,
>> + .bit = SEESAW_BUTTON_START,
>> + },
>> + {
>> + .code = BTN_SELECT,
>> + .bit = SEESAW_BUTTON_SELECT,
>> + },
>> +};
>
> This looks very much like a sparse keymap which can be implemented with
> the helpers from <linux/input/sparse-keymap.h>.
>
When going through the API provided by sparse-keymap, I could only see
the use for sparse_keymap_report_entry here. Which leads to the
following refactored code:
static const struct key_entry seesaw_buttons_new[] = {
{KE_KEY, SEESAW_BUTTON_A, {BTN_SOUTH}},
{KE_KEY, SEESAW_BUTTON_B, {BTN_EAST}},
...
};
for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) {
sparse_keymap_report_entry(input, &seesaw_buttons_new[i],
data.button_state & BIT(seesaw_buttons_new[i].code),
false);
}
I don't think this significantly improves the code unless you had some
other way to use the API in mind.
> Personally I prefer each keymap entry to be on a single line (without
> designated initializers, maybe with some alignment) to save a bunch of
> vertical space.
>
>> +
>> +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 };
>> +
>> + ret = i2c_master_send(client, register_buf, sizeof(register_buf));
>> + if (ret < 0)
>> + return ret;
>> + ret = i2c_master_recv(client, buf, count);
>> + 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;
>> + u8 read_buf[4];
>> +
>> + ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
>> + read_buf, sizeof(read_buf));
>> + if (ret)
>> + return ret;
>> +
>> + data->button_state = ~get_unaligned_be32(&read_buf);
>> +
>> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
>> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
>> + (char *)&data->x, sizeof(data->x));
>
> Nitpick: read into a dedicated local variable instead of 'data', as it's
> easier to understand.
>
>> + 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(data->x);
>> +
>> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
>> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
>> + (char *)&data->y, sizeof(data->y));
>> + if (ret)
>> + return ret;
>> + data->y = be16_to_cpu(data->y);
>> +
>> + 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 != 0) {
>
> Everywhere else this is just '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 (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
>> + input_report_key(input, seesaw_buttons[i].code,
>> + data.button_state &
>> + BIT(seesaw_buttons[i].bit));
>> + }
>> + input_sync(input);
>> +}
>> +
>> +static int seesaw_probe(struct i2c_client *client)
>> +{
>> + int err, i;
>> + u8 hardware_id;
>> + struct seesaw_gamepad *seesaw;
>> +
>> + err = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
>> + SEESAW_STATUS_SWRST, 0xFF);
>> + if (err)
>> + return err;
>> +
>> + /* Wait for the registers to reset before proceeding */
>> + mdelay(10);
>> +
>> + seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
>> + if (!seesaw)
>> + return -ENOMEM;
>> +
>> + err = seesaw_register_read(client, SEESAW_STATUS_BASE,
>> + SEESAW_STATUS_HW_ID, &hardware_id, 1);
>> + if (err)
>> + return err;
>> +
>> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
>> + hardware_id);
>> +
>> + /* Set Pin Mode to input and enable pull-up resistors */
>> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
>> + SEESAW_GPIO_DIRCLR_BULK,
>> + SEESAW_BUTTON_MASK);
>> + if (err)
>> + return err;
>> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
>> + SEESAW_GPIO_PULLENSET,
>> + SEESAW_BUTTON_MASK);
>> + if (err)
>> + return err;
>> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
>> + SEESAW_GPIO_BULK_SET,
>> + SEESAW_BUTTON_MASK);
>> + if (err)
>> + return err;
>> +
>> + seesaw->i2c_client = client;
>> + i2c_set_clientdata(client, seesaw);
>
> I'm not familiar with the i2c APIs but this clientdata seems to be
> unused.
>
>> +
>> + 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);
>> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
>> + input_set_capability(seesaw->input_dev, EV_KEY,
>> + seesaw_buttons[i].code);
>> + }
>> +
>> + err = input_setup_polling(seesaw->input_dev, seesaw_poll);
>> + if (err) {
>> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
>> + return err;
>> + }
>> +
>> + input_set_poll_interval(seesaw->input_dev,
>> + SEESAW_GAMEPAD_POLL_INTERVAL);
>
> Why the linebreak on this line and not on the ones below?
>
It was clang-format trying to prevent the line from being 81 characters
long, would be fixed in the next patch.
>> + input_set_max_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MAX);
>> + input_set_min_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MIN);
>> +
>> + err = input_register_device(seesaw->input_dev);
>> + if (err) {
>> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
>> + return err;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id seesaw_id_table[] = {
>> + { SEESAW_DEVICE_NAME, 0 },
>> + { /* Sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
>> +
>> +static struct i2c_driver seesaw_driver = {
>> + .driver = {
>> + .name = SEESAW_DEVICE_NAME,
>> + },
>> + .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.42.0
>>
Oct 31, 2023 03:10:50 Anshul Dalal <[email protected]>:
> Hello Thomas,
>
> Thanks for the review! The requested changes will be addressed in the
> next patch version though I had a few comments below:
>
> On 10/27/23 11:44, Thomas Weißschuh wrote:
>> Hi Anshul,
>>
>> thanks for the reworks!
>>
>> Some more comments inline.
>>
>> On 2023-10-27 10:48:11+0530, Anshul Dalal wrote:
[..]
>>> +struct seesaw_button_description {
>>> + unsigned int code;
>>> + unsigned int bit;
>>> +};
>>> +
>>> +static const struct seesaw_button_description seesaw_buttons[] = {
>>> + {
>>> + .code = BTN_EAST,
>>> + .bit = SEESAW_BUTTON_A,
>>> + },
>>> + {
>>> + .code = BTN_SOUTH,
>>> + .bit = SEESAW_BUTTON_B,
>>> + },
>>> + {
>>> + .code = BTN_NORTH,
>>> + .bit = SEESAW_BUTTON_X,
>>> + },
>>> + {
>>> + .code = BTN_WEST,
>>> + .bit = SEESAW_BUTTON_Y,
>>> + },
>>> + {
>>> + .code = BTN_START,
>>> + .bit = SEESAW_BUTTON_START,
>>> + },
>>> + {
>>> + .code = BTN_SELECT,
>>> + .bit = SEESAW_BUTTON_SELECT,
>>> + },
>>> +};
>>
>> This looks very much like a sparse keymap which can be implemented with
>> the helpers from <linux/input/sparse-keymap.h>.
>>
>
> When going through the API provided by sparse-keymap, I could only see
> the use for sparse_keymap_report_entry here. Which leads to the
> following refactored code:
>
> static const struct key_entry seesaw_buttons_new[] = {
> {KE_KEY, SEESAW_BUTTON_A, {BTN_SOUTH}},
> {KE_KEY, SEESAW_BUTTON_B, {BTN_EAST}},
No braces I think.
> ...
> };
>
> for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) {
> sparse_keymap_report_entry(input, &seesaw_buttons_new[i],
> data.button_state & BIT(seesaw_buttons_new[i].code),
> false);
> }
>
> I don't think this significantly improves the code unless you had some
> other way to use the API in mind.
I thought about sparse_keymap_setup() and sparse_keymap_report_event().
It does not significantly change the code but would be a standard API.
Thomas
On 10/31/23 07:53, Thomas Weißschuh wrote:
> Oct 31, 2023 03:10:50 Anshul Dalal <[email protected]>:
>
>> Hello Thomas,
>>
>> Thanks for the review! The requested changes will be addressed in the
>> next patch version though I had a few comments below:
>>
>> On 10/27/23 11:44, Thomas Weißschuh wrote:
>>> Hi Anshul,
>>>
>>> thanks for the reworks!
>>>
>>> Some more comments inline.
>>>
>>> On 2023-10-27 10:48:11+0530, Anshul Dalal wrote:
>
> [..]
>
>>>> +struct seesaw_button_description {
>>>> + unsigned int code;
>>>> + unsigned int bit;
>>>> +};
>>>> +
>>>> +static const struct seesaw_button_description seesaw_buttons[] = {
>>>> + {
>>>> + .code = BTN_EAST,
>>>> + .bit = SEESAW_BUTTON_A,
>>>> + },
>>>> + {
>>>> + .code = BTN_SOUTH,
>>>> + .bit = SEESAW_BUTTON_B,
>>>> + },
>>>> + {
>>>> + .code = BTN_NORTH,
>>>> + .bit = SEESAW_BUTTON_X,
>>>> + },
>>>> + {
>>>> + .code = BTN_WEST,
>>>> + .bit = SEESAW_BUTTON_Y,
>>>> + },
>>>> + {
>>>> + .code = BTN_START,
>>>> + .bit = SEESAW_BUTTON_START,
>>>> + },
>>>> + {
>>>> + .code = BTN_SELECT,
>>>> + .bit = SEESAW_BUTTON_SELECT,
>>>> + },
>>>> +};
>>>
>>> This looks very much like a sparse keymap which can be implemented with
>>> the helpers from <linux/input/sparse-keymap.h>.
>>>
>>
>> When going through the API provided by sparse-keymap, I could only see
>> the use for sparse_keymap_report_entry here. Which leads to the
>> following refactored code:
>>
>> static const struct key_entry seesaw_buttons_new[] = {
>> {KE_KEY, SEESAW_BUTTON_A, {BTN_SOUTH}},
>> {KE_KEY, SEESAW_BUTTON_B, {BTN_EAST}},
>
> No braces I think.
>
Since the last field in key_entry is a union, the braces seem to be
required.
>> ...
>> };
>>
>> for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) {
>> sparse_keymap_report_entry(input, &seesaw_buttons_new[i],
>> data.button_state & BIT(seesaw_buttons_new[i].code),
>> false);
>> }
>>
>> I don't think this significantly improves the code unless you had some
>> other way to use the API in mind.
>
> I thought about sparse_keymap_setup() and sparse_keymap_report_event().
>
> It does not significantly change the code but would be a standard API.
>
Thanks for pointing me in the right direction, do you think the
following implementation of the API is acceptable for the driver. Since
I couldn't find a driver for any similar device using the API in this
manner.
inside seesaw_probe():
err = sparse_keymap_setup(seesaw->input_dev, seesaw_buttons_new, NULL);
if (err) {
dev_err(&client->dev,
"failed to set up input device keymap: %d\n", err);
return err;
}
inside seesaw_poll():
for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) {
if (!sparse_keymap_report_event(
input, seesaw_buttons_new[i].code,
data.button_state & BIT(seesaw_buttons_new[i].code),
false)) {
dev_err_ratelimited(
&input->dev,
"failed to report event for keycode: %d",
seesaw_buttons_new[i].keycode);
return;
}
}
> Thomas
Best regards,
Anshul
Hi Anshul,
On 2023-11-01 09:50:36+0530, Anshul Dalal wrote:
> On 10/31/23 07:53, Thomas Weißschuh wrote:
> > Oct 31, 2023 03:10:50 Anshul Dalal <[email protected]>:
> >> Thanks for the review! The requested changes will be addressed in the
> >> next patch version though I had a few comments below:
> >>
> >> On 10/27/23 11:44, Thomas Weißschuh wrote:
> >>> Hi Anshul,
> >>>
> >>> thanks for the reworks!
> >>>
> >>> Some more comments inline.
> >>>
> >>> On 2023-10-27 10:48:11+0530, Anshul Dalal wrote:
> >
> > [..]
> >
> >>>> +struct seesaw_button_description {
> >>>> + unsigned int code;
> >>>> + unsigned int bit;
> >>>> +};
> >>>> +
> >>>> +static const struct seesaw_button_description seesaw_buttons[] = {
> >>>> + {
> >>>> + .code = BTN_EAST,
> >>>> + .bit = SEESAW_BUTTON_A,
> >>>> + },
> >>>> + {
> >>>> + .code = BTN_SOUTH,
> >>>> + .bit = SEESAW_BUTTON_B,
> >>>> + },
> >>>> + {
> >>>> + .code = BTN_NORTH,
> >>>> + .bit = SEESAW_BUTTON_X,
> >>>> + },
> >>>> + {
> >>>> + .code = BTN_WEST,
> >>>> + .bit = SEESAW_BUTTON_Y,
> >>>> + },
> >>>> + {
> >>>> + .code = BTN_START,
> >>>> + .bit = SEESAW_BUTTON_START,
> >>>> + },
> >>>> + {
> >>>> + .code = BTN_SELECT,
> >>>> + .bit = SEESAW_BUTTON_SELECT,
> >>>> + },
> >>>> +};
> >>>
> >>> This looks very much like a sparse keymap which can be implemented with
> >>> the helpers from <linux/input/sparse-keymap.h>.
> >>>
> >>
> >> When going through the API provided by sparse-keymap, I could only see
> >> the use for sparse_keymap_report_entry here. Which leads to the
> >> following refactored code:
> >>
> >> static const struct key_entry seesaw_buttons_new[] = {
> >> {KE_KEY, SEESAW_BUTTON_A, {BTN_SOUTH}},
> >> {KE_KEY, SEESAW_BUTTON_B, {BTN_EAST}},
> >
> > No braces I think.
> >
>
> Since the last field in key_entry is a union, the braces seem to be
> required.
Indeed.
To make the union more visible explicit this could be done:
{ KE_KEY, SEESAW_BUTTON_A, .keycode = BTN_SOUTH }
>
> >> ...
> >> };
> >>
> >> for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) {
> >> sparse_keymap_report_entry(input, &seesaw_buttons_new[i],
> >> data.button_state & BIT(seesaw_buttons_new[i].code),
> >> false);
> >> }
> >>
> >> I don't think this significantly improves the code unless you had some
> >> other way to use the API in mind.
> >
> > I thought about sparse_keymap_setup() and sparse_keymap_report_event().
> >
> > It does not significantly change the code but would be a standard API.
> >
>
> Thanks for pointing me in the right direction, do you think the
> following implementation of the API is acceptable for the driver. Since
> I couldn't find a driver for any similar device using the API in this
> manner.
>
> inside seesaw_probe():
>
> err = sparse_keymap_setup(seesaw->input_dev, seesaw_buttons_new, NULL);
> if (err) {
> dev_err(&client->dev,
> "failed to set up input device keymap: %d\n", err);
> return err;
> }
Yes, and it replaces the calls to input_set_capability().
> inside seesaw_poll():
>
> for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) {
> if (!sparse_keymap_report_event(
> input, seesaw_buttons_new[i].code,
> data.button_state & BIT(seesaw_buttons_new[i].code),
> false)) {
> dev_err_ratelimited(
> &input->dev,
> "failed to report event for keycode: %d",
> seesaw_buttons_new[i].keycode);
> return;
> }
> }
for_each_set_bit(i, (long *)&SEESAW_BUTTON_MASK, BITS_PER_TYPE(SEESAW_BUTTON_MASK))
sparse_keymap_report_event(input, BIT(i), data.button_state & BIT(i), false);
The sparse keymap takes care of the translation.
Notes:
SEESAW_BUTTON_MASK is now an actual variable instead of a macro.
It should be 'static const' in that case.
When using the sparse keymap APIs the driver also needs to depend on
INPUT_SPARSEKMAP.
Thomas
Hi Thomas and Anshul,
On Fri, Oct 27, 2023 at 06:14:58AM +0000, Thomas Wei?schuh wrote:
[...]
> > +struct seesaw_data {
> > + __be16 x;
> > + __be16 y;
>
> The fact that these are big endian is now an implementation detail
> hidden within seesaw_read_data(), so the __be16 can go away.
>
> > + u32 button_state;
> > +} __packed;
>
> While this was requested by Jeff I don't think it's correct.
> The struct is never received in this form from the device.
> I guess he also got confused, like me, by the fact that data is directly
> read into the fields of the struct.
Apologies for some confusion on my part here; Thomas is correct and
my comment can be disregarded. Originally I thought this struct was
mapped to a continguous range of registers, but after studying the
documentation some more, that is clearly not the case :)
Kind regards,
Jeff LaBundy