Re: [PATCH RESEND v11 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Jeff LaBundy
Date: Wed Dec 27 2023 - 14:43:34 EST
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 <linux@xxxxxxxxxxxxxx>
> Signed-off-by: Anshul Dalal <anshulusr@xxxxxxxxx>
>
> ---
>
> 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/20231127161158.1651716-2-anshulusr@xxxxxxxxx/
> v10: https://lore.kernel.org/lkml/20231121123409.2231115-2-anshulusr@xxxxxxxxx/
> v9: https://lore.kernel.org/lkml/20231121101751.2189965-2-anshulusr@xxxxxxxxx/
> v8: https://lore.kernel.org/lkml/20231108005337.45069-2-anshulusr@xxxxxxxxx/
> v7: https://lore.kernel.org/lkml/20231106164134.114668-2-anshulusr@xxxxxxxxx/
> v6: https://lore.kernel.org/lkml/20231027051819.81333-2-anshulusr@xxxxxxxxx/
> v5: https://lore.kernel.org/lkml/20231017034356.1436677-2-anshulusr@xxxxxxxxx/
> v4: https://lore.kernel.org/lkml/20231010184827.1213507-2-anshulusr@xxxxxxxxx/
> v3: https://lore.kernel.org/linux-input/20231008185709.2448423-2-anshulusr@xxxxxxxxx/
> v2: https://lore.kernel.org/linux-input/20231008172435.2391009-2-anshulusr@xxxxxxxxx/
> v1: https://lore.kernel.org/linux-input/20231007144052.1535417-2-anshulusr@xxxxxxxxx/
> ---
> 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 <anshulusr@xxxxxxxxx>
> +L: linux-input@xxxxxxxxxxxxxxx
> +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 <jikos@xxxxxxxxxx>
> 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 <anshulusr@xxxxxxxxx>
> + *
> + * 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 <anshulusr@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
> +MODULE_LICENSE("GPL");
> --
> 2.43.0
>
Kind regards,
Jeff LaBundy