diff mbox series

[v8,1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad

Message ID 20231108005337.45069-1-anshulusr@gmail.com
State Not Applicable
Headers show
Series [v8,1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Anshul Dalal Nov. 8, 2023, 12:53 a.m. UTC 
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

Comments

Thomas Weißschuh Nov. 14, 2023, 7:07 p.m. UTC | #1 
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
>
Thomas Weißschuh Nov. 14, 2023, 7:20 p.m. UTC | #2 
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/
Krzysztof Kozlowski Nov. 14, 2023, 8:32 p.m. UTC | #3 
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
Jeff LaBundy Nov. 26, 2023, 4:10 a.m. UTC | #4 
Hi Thomas and Anshul,

On Tue, Nov 14, 2023 at 08:20:46PM +0100, 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>
> > ---

[...]

> > +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?

My original comment was merely to say that this driver doesn't need an
entire binding because it is easily covered by trivial-devices.yaml. The
whole point of that binding is to save the trouble of writing a new file
such as this for trivial devices with this same set of common properties.

I don't feel strongly about _not_ adding a new binding for this device,
it just seems like unnecessary work for all involved. If the maintainers
do not mind, then I don't either :)

Taking things a step further, this driver really doesn't need to define
an of_device_id struct either, because I seem to recall that OF can still
bind to drivers with "compatible" strings specified in the i2c_device_id
struct.

At any rate, this version is most certainly broken because the compatible
string defined in this binding does not match SEESAW_DEVICE_NAME specified
in the driver's i2c_device_id struct, and there is no of_device_id struct.
So there is no way the driver would bind if this binding were followed in
earnest.

I see this has been addressed in the latest version, so I will take a look
at that one.

> 
> > +
> > +  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/

Kind regards,
Jeff LaBundy
diff mbox series

Patch

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>;
+        };
+    };