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 <linux@xxxxxxxxxxxxxx> >> Signed-off-by: Anshul Dalal <anshulusr@xxxxxxxxx> >> --- >> 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 <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-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 <anshulusr@xxxxxxxxx>"); >> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.42.0 >>
