Message ID | 20231108005337.45069-1-anshulusr@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [v8,1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad | expand |
On 2023-11-08 06:23:36+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. With or without the tiny nitpicks mentioned below: Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> > 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. [..] > diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c > new file mode 100644 > index 000000000000..8e8ef26a585f > --- /dev/null > +++ b/drivers/input/joystick/adafruit-seesaw.c > @@ -0,0 +1,315 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com> > + * > + * 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 <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 > + > +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; This field is not used, but it doesn't hurt. If you remove the field you can get rid of the whole struct itself. > + struct i2c_client *i2c_client; > +}; > + > +struct seesaw_data { > + u16 x; > + u16 y; > + u32 button_state; > +}; > + > +struct seesaw_button_description { > + unsigned int code; > + unsigned int bit; > +}; Not used anymore. > + > +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 }, No comma after sentinel element. > +}; > + > +static int seesaw_register_read(struct i2c_client *client, u8 register_high, > + u8 register_low, char *buf, int count) > +{ > + int ret; > + u8 register_buf[2] = { register_high, register_low }; > + > + struct i2c_msg message_buf[2] = { > + { > + .addr = client->addr, > + .flags = client->flags, > + .len = sizeof(register_buf), > + .buf = register_buf Commas after normal last elements. > + }, > + { > + .addr = client->addr, > + .flags = client->flags | I2C_M_RD, > + .len = count, > + .buf = buf Same as above. > + }, > + }; > + ret = i2c_transfer(client->adapter, message_buf, > + ARRAY_SIZE(message_buf)); > + > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int seesaw_register_write_u8(struct i2c_client *client, u8 register_high, > + u8 register_low, u8 value) > +{ > + int ret; > + u8 write_buf[3] = { register_high, register_low, value }; > + > + ret = i2c_master_send(client, write_buf, sizeof(write_buf)); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int seesaw_register_write_u32(struct i2c_client *client, > + u8 register_high, u8 register_low, > + u32 value) > +{ > + int ret; > + u8 write_buf[6] = { register_high, register_low }; > + > + put_unaligned_be32(value, write_buf + 2); > + ret = i2c_master_send(client, write_buf, sizeof(write_buf)); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data) > +{ > + int ret; > + __be16 adc_data; > + __be32 read_buf; > + > + ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK, > + (char *)&read_buf, sizeof(read_buf)); > + if (ret) > + return ret; > + > + data->button_state = ~be32_to_cpu(read_buf); > + > + ret = seesaw_register_read(client, SEESAW_ADC_BASE, > + SEESAW_ADC_OFFSET + SEESAW_ANALOG_X, > + (char *)&adc_data, sizeof(adc_data)); > + if (ret) > + return ret; > + /* > + * ADC reads left as max and right as 0, must be reversed since kernel > + * expects reports in opposite order. > + */ > + data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(adc_data); > + > + ret = seesaw_register_read(client, SEESAW_ADC_BASE, > + SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y, > + (char *)&adc_data, sizeof(adc_data)); > + if (ret) > + return ret; > + data->y = be16_to_cpu(adc_data); > + > + return 0; > +} > + > +static void seesaw_poll(struct input_dev *input) > +{ > + int err, i; > + struct seesaw_gamepad *private = input_get_drvdata(input); > + struct seesaw_data data; > + > + err = seesaw_read_data(private->i2c_client, &data); > + if (err) { > + dev_err_ratelimited(&input->dev, > + "failed to read joystick state: %d\n", err); > + return; > + } > + > + input_report_abs(input, ABS_X, data.x); > + input_report_abs(input, ABS_Y, data.y); > + > + for_each_set_bit(i, (long *)&SEESAW_BUTTON_MASK, > + BITS_PER_TYPE(SEESAW_BUTTON_MASK)) { > + if (!sparse_keymap_report_event( > + input, i, data.button_state & BIT(i), false)) { > + dev_err_ratelimited(&input->dev, > + "failed to report keymap event"); > + return; I would drop this return. Report as much data as possible. > + }; > + } > + > + 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(10000, 15000); This would be a very tiny bit clearer: usleep_range(10 * MSEC_PER_USEC, 15 * MSEC_PER_USEC); > + > + seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL); > + if (!seesaw) > + return -ENOMEM; > + > + ret = seesaw_register_read(client, SEESAW_STATUS_BASE, > + SEESAW_STATUS_HW_ID, &hardware_id, > + sizeof(hardware_id)); > + if (ret) > + return ret; > + > + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n", > + hardware_id); > + > + /* Set Pin Mode to input and enable pull-up resistors */ > + ret = seesaw_register_write_u32(client, SEESAW_GPIO_BASE, > + SEESAW_GPIO_DIRCLR_BULK, > + SEESAW_BUTTON_MASK); > + if (ret) > + return ret; > + ret = seesaw_register_write_u32(client, SEESAW_GPIO_BASE, > + SEESAW_GPIO_PULLENSET, > + SEESAW_BUTTON_MASK); > + if (ret) > + return ret; > + ret = seesaw_register_write_u32(client, SEESAW_GPIO_BASE, > + SEESAW_GPIO_BULK_SET, > + SEESAW_BUTTON_MASK); > + if (ret) > + return ret; > + > + seesaw->i2c_client = client; > + seesaw->input_dev = devm_input_allocate_device(&client->dev); > + if (!seesaw->input_dev) > + return -ENOMEM; > + > + seesaw->input_dev->id.bustype = BUS_I2C; > + seesaw->input_dev->name = "Adafruit Seesaw Gamepad"; > + seesaw->input_dev->phys = "i2c/" SEESAW_DEVICE_NAME; > + input_set_drvdata(seesaw->input_dev, seesaw); > + input_set_abs_params(seesaw->input_dev, ABS_X, 0, > + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ, > + SEESAW_JOYSTICK_FLAT); > + input_set_abs_params(seesaw->input_dev, ABS_Y, 0, > + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ, > + SEESAW_JOYSTICK_FLAT); > + > + ret = sparse_keymap_setup(seesaw->input_dev, seesaw_buttons_new, NULL); > + if (ret) { > + dev_err(&client->dev, > + "failed to set up input device keymap: %d\n", ret); > + return ret; > + } > + > + ret = input_setup_polling(seesaw->input_dev, seesaw_poll); > + if (ret) { > + dev_err(&client->dev, "failed to set up polling: %d\n", ret); > + return ret; > + } > + > + input_set_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_INTERVAL); > + input_set_max_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MAX); > + input_set_min_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MIN); > + > + ret = input_register_device(seesaw->input_dev); > + if (ret) { > + dev_err(&client->dev, "failed to register joystick: %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static const struct i2c_device_id seesaw_id_table[] = { > + { SEESAW_DEVICE_NAME, 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@gmail.com>"); > +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver"); > +MODULE_LICENSE("GPL"); > -- > 2.42.0 >
On 2023-11-08 06:23:35+0530, Anshul Dalal wrote: > 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 <conor.dooley@microchip.com> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Signed-off-by: Anshul Dalal <anshulusr@gmail.com> > --- > > Changes for v8: > - no updates > > Changes for v7: > - no updates > > Changes for v6: > - no updates > > Changes for v5: > - Added link to the datasheet > > Changes for v4: > - Fixed the URI for the id field > - Added `interrupts` property > > Changes for v3: > - Updated id field to reflect updated file name from previous version > - Added `reg` property > > Changes for v2: > - Renamed file to `adafruit,seesaw-gamepad.yaml` > - Removed quotes for `$id` and `$schema` > - Removed "Bindings for" from the description > - Changed node name to the generic name "joystick" > - Changed compatible to 'adafruit,seesaw-gamepad' instead of > 'adafruit,seesaw_gamepad' > --- > .../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 <anshulusr@gmail.com> > + > +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 I don't really have any clue about devicetree, but shouldn't the actual driver have an id-table for this "compatible"? It had one up to v5 of the patchset. Jeff had some comments about the OF aspect [0], but to me the state now seems incorrect. Maybe the DT can be dropped completely? Jeff, could you advise? > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + description: > + The gamepad's IRQ pin triggers a rising edge if interrupts are enabled. Interrupts are not supported yet by the driver. Should the property be there already? > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + joystick@50 { > + compatible = "adafruit,seesaw-gamepad"; > + reg = <0x50>; > + }; > + }; > -- > 2.42.0 [0] https://lore.kernel.org/lkml/ZTWza+S+t+UZKlwu@nixie71/
On 14/11/2023 20:20, Thomas Weißschuh wrote: > On 2023-11-08 06:23:35+0530, Anshul Dalal wrote: >> 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 <conor.dooley@microchip.com> >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Signed-off-by: Anshul Dalal <anshulusr@gmail.com> >> --- >> >> Changes for v8: >> - no updates >> >> Changes for v7: >> - no updates >> >> Changes for v6: >> - no updates >> >> Changes for v5: >> - Added link to the datasheet >> >> Changes for v4: >> - Fixed the URI for the id field >> - Added `interrupts` property >> >> Changes for v3: >> - Updated id field to reflect updated file name from previous version >> - Added `reg` property >> >> Changes for v2: >> - Renamed file to `adafruit,seesaw-gamepad.yaml` >> - Removed quotes for `$id` and `$schema` >> - Removed "Bindings for" from the description >> - Changed node name to the generic name "joystick" >> - Changed compatible to 'adafruit,seesaw-gamepad' instead of >> 'adafruit,seesaw_gamepad' >> --- >> .../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 <anshulusr@gmail.com> >> + >> +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 > > I don't really have any clue about devicetree, but shouldn't the actual > driver have an id-table for this "compatible"? It should, why was it dropped? > > It had one up to v5 of the patchset. > > Jeff had some comments about the OF aspect [0], but to me the state now > seems incorrect. > Maybe the DT can be dropped completely? > > Jeff, could you advise? > >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + maxItems: 1 >> + description: >> + The gamepad's IRQ pin triggers a rising edge if interrupts are enabled. > > Interrupts are not supported yet by the driver. > Should the property be there already? Bindings describe the hardware, no the driver. If the hardware has interrupt line, it should be described here. Best regards, Krzysztof
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 <anshulusr@gmail.com> + +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>; + }; + };