| Message ID | 20210531133243.9488-1-oleg@kaa.org.ua (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | input: add SparkFun Qwiic Joystick driver | expand |
Hi Oleh, Neat little device; some comments below. On Mon, May 31, 2021 at 04:32:43PM +0300, Oleh Kravchenko wrote: > A simple analog joystick built on Low Power ATtiny85 Microcontroller. > Directional movements are measured with two 10 kΩ potentiometers > connected with a gimbal mechanism that separates the horizontal and > vertical movements. This joystick also has a select button that is actuated > when the joystick is pressed down. > > Input events polled over the I2C bus. > > Product page: > https://www.sparkfun.com/products/15168 > Firmware and hardware sources: > https://github.com/sparkfun/Qwiic_Joystick > > Tested on RPi4B and O4-iMX-NANO boards. > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Cc: Device Tree mailing list <devicetree@vger.kernel.org> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Jiri Kosina <jikos@jikos.cz> > Cc: Patchwork Bot <patchwork-bot@kernel.org> > Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua> > --- Please use a version number (e.g. v2, v3) and add a change log under the '---' when sending new versions. > .../bindings/input/qwiic-joystick.yaml | 31 ++++ > .../devicetree/bindings/vendor-prefixes.yaml | 2 + > drivers/input/joystick/Kconfig | 9 + > drivers/input/joystick/Makefile | 1 + > drivers/input/joystick/qwiic-joystick.c | 169 ++++++++++++++++++ > 5 files changed, 212 insertions(+) > create mode 100644 Documentation/devicetree/bindings/input/qwiic-joystick.yaml > create mode 100644 drivers/input/joystick/qwiic-joystick.c Bindings must be a separate patch with Rob Herring (see MAINTAINERS) on the To: list. > > diff --git a/Documentation/devicetree/bindings/input/qwiic-joystick.yaml b/Documentation/devicetree/bindings/input/qwiic-joystick.yaml > new file mode 100644 > index 000000000000..51cadeb350f2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/input/qwiic-joystick.yaml > @@ -0,0 +1,31 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright 2021 Oleh Kravchenko > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/input/qwiic-joystick.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > + > +title: SparkFun Qwiic Joystick > + > +maintainers: > + - Oleh Kravchenko <oleg@kaa.org.ua> > + > +description: | > + Bindings for SparkFun Qwiic Joystick (COM-15168). > + https://www.sparkfun.com/products/15168 > + > +properties: > + compatible: > + const: sparkfun,qwiic-joystick > + > +required: > + - compatible 'reg' is required as well. > + > +additionalProperties: false > + > +examples: > + - | > + qwiic@20 { > + compatible = "sparkfun,qwiic-joystick"; > + reg = <0x20>; > + }; > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml > index 944b02bb96d7..a5631e68f5d1 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml > @@ -1060,6 +1060,8 @@ patternProperties: > description: Sony Corporation > "^spansion,.*": > description: Spansion Inc. > + "^sparkfun,.*": > + description: SparkFun Electronics > "^sprd,.*": > description: Spreadtrum Communications Inc. > "^sst,.*": > diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig > index 5e38899058c1..7dfe8ea90923 100644 > --- a/drivers/input/joystick/Kconfig > +++ b/drivers/input/joystick/Kconfig > @@ -372,6 +372,15 @@ config JOYSTICK_PXRC > To compile this driver as a module, choose M here: the > module will be called pxrc. > > +config JOYSTICK_QWIIC > + tristate "SparkFun Qwiic Joystick" > + depends on I2C > + help > + Say Y here if you want to use the SparkFun Qwiic Joystick. > + > + To compile this driver as a module, choose M here: the > + module will be called qwiic-joystick. > + > config JOYSTICK_FSIA6B > tristate "FlySky FS-iA6B RC Receiver" > select SERIO > diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile > index 31d720c9e493..5174b8aba2dd 100644 > --- a/drivers/input/joystick/Makefile > +++ b/drivers/input/joystick/Makefile > @@ -27,6 +27,7 @@ obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o > 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_SIDEWINDER) += sidewinder.o > obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o > obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o > diff --git a/drivers/input/joystick/qwiic-joystick.c b/drivers/input/joystick/qwiic-joystick.c > new file mode 100644 > index 000000000000..9b7ab0dd1218 > --- /dev/null > +++ b/drivers/input/joystick/qwiic-joystick.c > @@ -0,0 +1,169 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2021 Oleh Kravchenko <oleg@kaa.org.ua> > + > +/* > + * SparkFun Qwiic Joystick > + * Product page:https://www.sparkfun.com/products/15168 > + * Firmware and hardware sources:https://github.com/sparkfun/Qwiic_Joystick > + */ Nit: typically the copyright information goes in the multi-line introductory comments; there is no need for a newline after the SPDX identifier either. > + > +#include <linux/bits.h> > +#include <linux/i2c.h> > +#include <linux/input.h> > +#include <linux/module.h> > + > +#define QWIIC_JSK_REG_VERS 0 > +#define QWIIC_JSK_REG_DATA 3 > + > +#define QWIIC_JSK_MAX_AXIS GENMASK(10, 0) > +#define QWIIC_JSK_FUZZ 2 > +#define QWIIC_JSK_FLAT 2 > + > +struct qwiic_jsk { > + char phys[32]; > + struct input_dev *dev; > + struct i2c_client *i2c; > +}; > + > +struct qwiic_ver { > + u8 addr; > + u8 major; > + u8 minor; > +} __packed; > + > +struct qwiic_data { > + u8 hx; > + u8 lx; > + u8 hy; > + u8 ly; > + u8 thumb; > +} __packed; Let's use __be16 for 16-bit big-endian horizontal and vertical position. > + > +static void qwiic_poll(struct input_dev *input) > +{ > + struct qwiic_jsk *priv; > + struct qwiic_data data; > + int ret; > + int x, y, btn; No need to align these declarations. > + > + priv = input_get_drvdata(input); > + > + ret = i2c_smbus_read_i2c_block_data(priv->i2c, QWIIC_JSK_REG_DATA, > + sizeof(data), (u8 *)&data); > + if (ret == sizeof(data)) { This is personal preference, but returning early on error avoids having to indent the rest of the function. > + x = (data.hx << 8 | data.lx) >> 6; > + y = (data.hy << 8 | data.ly) >> 6; With hx/lx and hy/ly defined using __be16, you can use be16_to_cpu() to resolve the 16-bit value. A comment to explain the 6-bit shift from the documentation would be useful too. > + btn = !!!data.thumb; I don't think the triple-not is necessary as input_report_key() already squashes non-zero values to 1 with '!!'. If register 0x07's polarity is inverted, just pass !data.thumb to input_report_key(). > + > + input_report_abs(input, ABS_X, x); > + input_report_abs(input, ABS_Y, y); > + input_report_key(input, BTN_THUMBL, btn); > + > + input_sync(input); > + } > +} > + > +static int qwiic_probe(struct i2c_client *i2c, const struct i2c_device_id *id) It's much more common (and easier to read) if the i2c_client struct is passed as *client. > +{ > + struct qwiic_jsk *priv; > + struct qwiic_ver vers; > + int ret; No need to align these declarations. > + > + ret = i2c_smbus_read_i2c_block_data(i2c, QWIIC_JSK_REG_VERS, > + sizeof(vers), (u8 *)&vers); > + if (ret != sizeof(vers)) { > + ret = -EIO; > + goto err; > + } If i2c_smbus_read_i2c_block_data() returned a negative value, the original return value should be preserved. Only return -EIO if 0 <= ret < sizeof(). You can also simply return directly; there is nothing to be undone at this stage. > + > + if (i2c->addr != vers.addr) { > + dev_err(&i2c->dev, "address doesn't match!\n"); > + ret = -ENODEV; > + goto err; > + } This error should be impossible. If the address in dts does not match what the hardware is expecting, i2c_smbus_read_i2c_block_data() would have been NAK'd in the first place. > + > + dev_info(&i2c->dev, "SparkFun Qwiic Joystick, FW: %d.%d\n", > + vers.major, vers.minor); This should be dev_dbg() at most. > + > + priv = devm_kzalloc(&i2c->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) { > + ret = -ENOMEM; > + goto err; > + } Just return -ENOMEM here. > + > + priv->i2c = i2c; > + snprintf(priv->phys, sizeof(priv->phys), "i2c/%s", dev_name(&i2c->dev)); > + i2c_set_clientdata(i2c, priv); > + > + priv->dev = devm_input_allocate_device(&i2c->dev); > + if (!priv->dev) { > + dev_err(&i2c->dev, "failed to allocate input device\n"); No need for a print here. > + ret = -ENOMEM; > + goto err; Just return -ENOMEM here. > + } > + > + priv->dev->dev.parent = &i2c->dev; This is not necessary; devm_input_allocate_device() takes care of it. > + priv->dev->id.bustype = BUS_I2C; > + priv->dev->name = "SparkFun Qwiic Joystick"; > + priv->dev->phys = priv->phys; > + input_set_drvdata(priv->dev, priv); > + > + input_set_abs_params(priv->dev, ABS_X, 0, QWIIC_JSK_MAX_AXIS, > + QWIIC_JSK_FUZZ, QWIIC_JSK_FLAT); > + input_set_abs_params(priv->dev, ABS_Y, 0, QWIIC_JSK_MAX_AXIS, > + QWIIC_JSK_FUZZ, QWIIC_JSK_FLAT); > + input_set_capability(priv->dev, EV_KEY, BTN_THUMBL); > + > + ret = input_setup_polling(priv->dev, qwiic_poll); > + if (ret) { > + dev_err(&i2c->dev, "failed to set up polling: %d\n", ret); > + goto err; > + } Again, you can just return 'ret' here. > + input_set_poll_interval(priv->dev, 16); > + input_set_min_poll_interval(priv->dev, 8); > + input_set_max_poll_interval(priv->dev, 32); Is there any reason not to #define these intervals? > + > + ret = input_register_device(priv->dev); > + if (ret) > + dev_err(&i2c->dev, "failed to register joystick: %d\n", ret); > + > +err: This goto label is unnecessary. > + return ret; > +} > + > +static int qwiic_remove(struct i2c_client *i2c) > +{ > + struct qwiic_jsk *priv; > + > + priv = i2c_get_clientdata(i2c); > + input_unregister_device(priv->dev); > + > + return 0; > +} Because the input device was created with devm_input_allocate_device(), input_register_device() is managed as well. Therefore you can drop the remove callback altogether. > + > +static const struct of_device_id of_qwiic_match[] = { > + { .compatible = "sparkfun,qwiic-joystick", }, > + {}, Nit: { } > +}; > +MODULE_DEVICE_TABLE(of, of_qwiic_match); > + > +static const struct i2c_device_id qwiic_id_table[] = { > + { KBUILD_MODNAME, 0 }, > + {}, And here (see other drivers). > +}; > +MODULE_DEVICE_TABLE(i2c, qwiic_id_table); > + > +static struct i2c_driver qwiic_driver = { > + .driver = { > + .name = KBUILD_MODNAME, > + .of_match_table = of_match_ptr(of_qwiic_match), > + }, > + .id_table = qwiic_id_table, > + .probe = qwiic_probe, > + .remove = qwiic_remove, > +}; > +module_i2c_driver(qwiic_driver); No need for alignment here either. > + > +MODULE_AUTHOR("Oleh Kravchenko <oleg@kaa.org.ua>"); > +MODULE_DESCRIPTION("SparkFun Qwiic Joystick driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.26.3 > Kind regards, Jeff LaBundy
Hi Oleh, On Mon, May 31, 2021 at 04:32:43PM +0300, Oleh Kravchenko wrote: > A simple analog joystick built on Low Power ATtiny85 Microcontroller. > Directional movements are measured with two 10 kΩ potentiometers > connected with a gimbal mechanism that separates the horizontal and > vertical movements. This joystick also has a select button that is actuated > when the joystick is pressed down. > > Input events polled over the I2C bus. Thank you for the driver. > > Product page: > https://www.sparkfun.com/products/15168 > Firmware and hardware sources: > https://github.com/sparkfun/Qwiic_Joystick > > Tested on RPi4B and O4-iMX-NANO boards. > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Cc: Device Tree mailing list <devicetree@vger.kernel.org> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Jiri Kosina <jikos@jikos.cz> > Cc: Patchwork Bot <patchwork-bot@kernel.org> > Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua> > --- > .../bindings/input/qwiic-joystick.yaml | 31 ++++ > .../devicetree/bindings/vendor-prefixes.yaml | 2 + > drivers/input/joystick/Kconfig | 9 + > drivers/input/joystick/Makefile | 1 + > drivers/input/joystick/qwiic-joystick.c | 169 ++++++++++++++++++ > 5 files changed, 212 insertions(+) > create mode 100644 Documentation/devicetree/bindings/input/qwiic-joystick.yaml > create mode 100644 drivers/input/joystick/qwiic-joystick.c > > diff --git a/Documentation/devicetree/bindings/input/qwiic-joystick.yaml b/Documentation/devicetree/bindings/input/qwiic-joystick.yaml > new file mode 100644 > index 000000000000..51cadeb350f2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/input/qwiic-joystick.yaml > @@ -0,0 +1,31 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright 2021 Oleh Kravchenko > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/input/qwiic-joystick.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > + > +title: SparkFun Qwiic Joystick > + > +maintainers: > + - Oleh Kravchenko <oleg@kaa.org.ua> > + > +description: | > + Bindings for SparkFun Qwiic Joystick (COM-15168). > + https://www.sparkfun.com/products/15168 > + > +properties: > + compatible: > + const: sparkfun,qwiic-joystick Rob, is this compatible acceptable? > + > +required: > + - compatible > + > +additionalProperties: false > + > +examples: > + - | > + qwiic@20 { > + compatible = "sparkfun,qwiic-joystick"; > + reg = <0x20>; > + }; I think this device would be covered by Documentation/devicetree/bindings/trivial-devices.yaml so please add its compatible there. > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml > index 944b02bb96d7..a5631e68f5d1 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml > @@ -1060,6 +1060,8 @@ patternProperties: > description: Sony Corporation > "^spansion,.*": > description: Spansion Inc. > + "^sparkfun,.*": > + description: SparkFun Electronics This should be a separate patch, please also send to Rob Herring and device tree list. > "^sprd,.*": > description: Spreadtrum Communications Inc. > "^sst,.*": > diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig > index 5e38899058c1..7dfe8ea90923 100644 > --- a/drivers/input/joystick/Kconfig > +++ b/drivers/input/joystick/Kconfig > @@ -372,6 +372,15 @@ config JOYSTICK_PXRC > To compile this driver as a module, choose M here: the > module will be called pxrc. > > +config JOYSTICK_QWIIC > + tristate "SparkFun Qwiic Joystick" > + depends on I2C > + help > + Say Y here if you want to use the SparkFun Qwiic Joystick. > + > + To compile this driver as a module, choose M here: the > + module will be called qwiic-joystick. > + > config JOYSTICK_FSIA6B > tristate "FlySky FS-iA6B RC Receiver" > select SERIO > diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile > index 31d720c9e493..5174b8aba2dd 100644 > --- a/drivers/input/joystick/Makefile > +++ b/drivers/input/joystick/Makefile > @@ -27,6 +27,7 @@ obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o > 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_SIDEWINDER) += sidewinder.o > obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o > obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o > diff --git a/drivers/input/joystick/qwiic-joystick.c b/drivers/input/joystick/qwiic-joystick.c > new file mode 100644 > index 000000000000..9b7ab0dd1218 > --- /dev/null > +++ b/drivers/input/joystick/qwiic-joystick.c > @@ -0,0 +1,169 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2021 Oleh Kravchenko <oleg@kaa.org.ua> > + > +/* > + * SparkFun Qwiic Joystick > + * Product page:https://www.sparkfun.com/products/15168 > + * Firmware and hardware sources:https://github.com/sparkfun/Qwiic_Joystick > + */ > + > +#include <linux/bits.h> > +#include <linux/i2c.h> > +#include <linux/input.h> > +#include <linux/module.h> > + > +#define QWIIC_JSK_REG_VERS 0 > +#define QWIIC_JSK_REG_DATA 3 > + > +#define QWIIC_JSK_MAX_AXIS GENMASK(10, 0) > +#define QWIIC_JSK_FUZZ 2 > +#define QWIIC_JSK_FLAT 2 > + > +struct qwiic_jsk { > + char phys[32]; > + struct input_dev *dev; > + struct i2c_client *i2c; > +}; > + > +struct qwiic_ver { > + u8 addr; > + u8 major; > + u8 minor; > +} __packed; There is no reason to declare structure containing only bytes as packed. > + > +struct qwiic_data { > + u8 hx; > + u8 lx; > + u8 hy; > + u8 ly; > + u8 thumb; > +} __packed; > + > +static void qwiic_poll(struct input_dev *input) > +{ > + struct qwiic_jsk *priv; > + struct qwiic_data data; > + int ret; > + int x, y, btn; > + > + priv = input_get_drvdata(input); > + > + ret = i2c_smbus_read_i2c_block_data(priv->i2c, QWIIC_JSK_REG_DATA, > + sizeof(data), (u8 *)&data); > + if (ret == sizeof(data)) { > + x = (data.hx << 8 | data.lx) >> 6; Why not define data as a u8 array, and then do u16 x = be16_to_cpup(data) >> 6; u16 y = be16_to_cpup(data + 2) >> 6; Or declare coordinates as __be16 in qwiic_data (and use be16_to_cpu when accessing). > + y = (data.hy << 8 | data.ly) >> 6; > + btn = !!!data.thumb; Triple negation is too much, one should be enough. > + > + input_report_abs(input, ABS_X, x); > + input_report_abs(input, ABS_Y, y); > + input_report_key(input, BTN_THUMBL, btn); > + > + input_sync(input); > + } > +} > + > +static int qwiic_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > +{ > + struct qwiic_jsk *priv; > + struct qwiic_ver vers; > + int ret; > + > + ret = i2c_smbus_read_i2c_block_data(i2c, QWIIC_JSK_REG_VERS, > + sizeof(vers), (u8 *)&vers); > + if (ret != sizeof(vers)) { > + ret = -EIO; > + goto err; > + } > + > + if (i2c->addr != vers.addr) { > + dev_err(&i2c->dev, "address doesn't match!\n"); > + ret = -ENODEV; > + goto err; > + } > + > + dev_info(&i2c->dev, "SparkFun Qwiic Joystick, FW: %d.%d\n", > + vers.major, vers.minor); Preference is not to be too noisy, please change to dev_dbg(). > + > + priv = devm_kzalloc(&i2c->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) { > + ret = -ENOMEM; Return -ENOMEM directly, no need to use labels when you do not need to do any cleanup by hand. > + goto err; > + } > + > + priv->i2c = i2c; > + snprintf(priv->phys, sizeof(priv->phys), "i2c/%s", dev_name(&i2c->dev)); > + i2c_set_clientdata(i2c, priv); > + > + priv->dev = devm_input_allocate_device(&i2c->dev); > + if (!priv->dev) { > + dev_err(&i2c->dev, "failed to allocate input device\n"); > + ret = -ENOMEM; > + goto err; > + } > + > + priv->dev->dev.parent = &i2c->dev; Done by devm_input_allocate_device(), please drop. > + priv->dev->id.bustype = BUS_I2C; > + priv->dev->name = "SparkFun Qwiic Joystick"; > + priv->dev->phys = priv->phys; > + input_set_drvdata(priv->dev, priv); > + > + input_set_abs_params(priv->dev, ABS_X, 0, QWIIC_JSK_MAX_AXIS, > + QWIIC_JSK_FUZZ, QWIIC_JSK_FLAT); > + input_set_abs_params(priv->dev, ABS_Y, 0, QWIIC_JSK_MAX_AXIS, > + QWIIC_JSK_FUZZ, QWIIC_JSK_FLAT); > + input_set_capability(priv->dev, EV_KEY, BTN_THUMBL); > + > + ret = input_setup_polling(priv->dev, qwiic_poll); Please call variables that carry error code or 0 "error" and not "ret". > + if (ret) { > + dev_err(&i2c->dev, "failed to set up polling: %d\n", ret); > + goto err; > + } > + input_set_poll_interval(priv->dev, 16); > + input_set_min_poll_interval(priv->dev, 8); > + input_set_max_poll_interval(priv->dev, 32); > + > + ret = input_register_device(priv->dev); > + if (ret) > + dev_err(&i2c->dev, "failed to register joystick: %d\n", ret); > + > +err: > + return ret; > +} > + > +static int qwiic_remove(struct i2c_client *i2c) > +{ > + struct qwiic_jsk *priv; > + > + priv = i2c_get_clientdata(i2c); > + input_unregister_device(priv->dev); Since you are using devm_input_allocate_device() you do not need to manually unregister it, it will be done automatically. Drop this and you can remove qwiic_remove() altogether. > + > + return 0; > +} > + > +static const struct of_device_id of_qwiic_match[] = { > + { .compatible = "sparkfun,qwiic-joystick", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, of_qwiic_match); > + > +static const struct i2c_device_id qwiic_id_table[] = { > + { KBUILD_MODNAME, 0 }, > + {}, > +}; > +MODULE_DEVICE_TABLE(i2c, qwiic_id_table); > + > +static struct i2c_driver qwiic_driver = { > + .driver = { > + .name = KBUILD_MODNAME, > + .of_match_table = of_match_ptr(of_qwiic_match), You either need to guard of_qwiic_match with #ifdef CONFIG_OF, or drop of_match_ptr() and assign of_match_table directly, otherwise for certain configs you may get "defined but not used" warning. > + }, > + .id_table = qwiic_id_table, > + .probe = qwiic_probe, > + .remove = qwiic_remove, > +}; > +module_i2c_driver(qwiic_driver); > + > +MODULE_AUTHOR("Oleh Kravchenko <oleg@kaa.org.ua>"); > +MODULE_DESCRIPTION("SparkFun Qwiic Joystick driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.26.3 > Thanks.
On Mon, 31 May 2021 16:32:43 +0300, Oleh Kravchenko wrote: > A simple analog joystick built on Low Power ATtiny85 Microcontroller. > Directional movements are measured with two 10 kΩ potentiometers > connected with a gimbal mechanism that separates the horizontal and > vertical movements. This joystick also has a select button that is actuated > when the joystick is pressed down. > > Input events polled over the I2C bus. > > Product page: > https://www.sparkfun.com/products/15168 > Firmware and hardware sources: > https://github.com/sparkfun/Qwiic_Joystick > > Tested on RPi4B and O4-iMX-NANO boards. > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Cc: Device Tree mailing list <devicetree@vger.kernel.org> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Jiri Kosina <jikos@jikos.cz> > Cc: Patchwork Bot <patchwork-bot@kernel.org> > Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua> > --- > .../bindings/input/qwiic-joystick.yaml | 31 ++++ > .../devicetree/bindings/vendor-prefixes.yaml | 2 + > drivers/input/joystick/Kconfig | 9 + > drivers/input/joystick/Makefile | 1 + > drivers/input/joystick/qwiic-joystick.c | 169 ++++++++++++++++++ > 5 files changed, 212 insertions(+) > create mode 100644 Documentation/devicetree/bindings/input/qwiic-joystick.yaml > create mode 100644 drivers/input/joystick/qwiic-joystick.c > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/input/qwiic-joystick.example.dts:21.13-26: Warning (reg_format): /example-0/qwiic@20:reg: property has invalid length (4 bytes) (#address-cells == 1, #size-cells == 1) Documentation/devicetree/bindings/input/qwiic-joystick.example.dt.yaml: Warning (pci_device_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/input/qwiic-joystick.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/input/qwiic-joystick.example.dt.yaml: Warning (simple_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/input/qwiic-joystick.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/input/qwiic-joystick.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format' /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/input/qwiic-joystick.example.dt.yaml: example-0: qwiic@20:reg:0: [32] is too short From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/input/qwiic-joystick.example.dt.yaml: qwiic@20: 'reg' does not match any of the regexes: 'pinctrl-[0-9]+' From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/input/qwiic-joystick.yaml See https://patchwork.ozlabs.org/patch/1485677 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On Mon, May 31, 2021 at 11:18:29PM -0700, Dmitry Torokhov wrote: > Hi Oleh, > > On Mon, May 31, 2021 at 04:32:43PM +0300, Oleh Kravchenko wrote: > > A simple analog joystick built on Low Power ATtiny85 Microcontroller. > > Directional movements are measured with two 10 kΩ potentiometers > > connected with a gimbal mechanism that separates the horizontal and > > vertical movements. This joystick also has a select button that is actuated > > when the joystick is pressed down. > > > > Input events polled over the I2C bus. > > Thank you for the driver. > > > > > Product page: > > https://www.sparkfun.com/products/15168 > > Firmware and hardware sources: > > https://github.com/sparkfun/Qwiic_Joystick > > > > Tested on RPi4B and O4-iMX-NANO boards. > > > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > Cc: Device Tree mailing list <devicetree@vger.kernel.org> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Cc: Jiri Kosina <jikos@jikos.cz> > > Cc: Patchwork Bot <patchwork-bot@kernel.org> > > Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua> > > --- > > .../bindings/input/qwiic-joystick.yaml | 31 ++++ > > .../devicetree/bindings/vendor-prefixes.yaml | 2 + > > drivers/input/joystick/Kconfig | 9 + > > drivers/input/joystick/Makefile | 1 + > > drivers/input/joystick/qwiic-joystick.c | 169 ++++++++++++++++++ > > 5 files changed, 212 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/input/qwiic-joystick.yaml > > create mode 100644 drivers/input/joystick/qwiic-joystick.c > > > > diff --git a/Documentation/devicetree/bindings/input/qwiic-joystick.yaml b/Documentation/devicetree/bindings/input/qwiic-joystick.yaml > > new file mode 100644 > > index 000000000000..51cadeb350f2 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/input/qwiic-joystick.yaml > > @@ -0,0 +1,31 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +# Copyright 2021 Oleh Kravchenko > > +%YAML 1.2 > > +--- > > +$id: "http://devicetree.org/schemas/input/qwiic-joystick.yaml#" > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > > + > > +title: SparkFun Qwiic Joystick > > + > > +maintainers: > > + - Oleh Kravchenko <oleg@kaa.org.ua> > > + > > +description: | > > + Bindings for SparkFun Qwiic Joystick (COM-15168). > > + https://www.sparkfun.com/products/15168 > > + > > +properties: > > + compatible: > > + const: sparkfun,qwiic-joystick > > Rob, is this compatible acceptable? Yes. > > > + > > +required: > > + - compatible > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + qwiic@20 { > > + compatible = "sparkfun,qwiic-joystick"; > > + reg = <0x20>; > > + }; > > I think this device would be covered by > Documentation/devicetree/bindings/trivial-devices.yaml so please add its > compatible there. That's fine. Or not. Please make bindings a separate patch. Rob
diff --git a/Documentation/devicetree/bindings/input/qwiic-joystick.yaml b/Documentation/devicetree/bindings/input/qwiic-joystick.yaml new file mode 100644 index 000000000000..51cadeb350f2 --- /dev/null +++ b/Documentation/devicetree/bindings/input/qwiic-joystick.yaml @@ -0,0 +1,31 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright 2021 Oleh Kravchenko +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/input/qwiic-joystick.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: SparkFun Qwiic Joystick + +maintainers: + - Oleh Kravchenko <oleg@kaa.org.ua> + +description: | + Bindings for SparkFun Qwiic Joystick (COM-15168). + https://www.sparkfun.com/products/15168 + +properties: + compatible: + const: sparkfun,qwiic-joystick + +required: + - compatible + +additionalProperties: false + +examples: + - | + qwiic@20 { + compatible = "sparkfun,qwiic-joystick"; + reg = <0x20>; + }; diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index 944b02bb96d7..a5631e68f5d1 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -1060,6 +1060,8 @@ patternProperties: description: Sony Corporation "^spansion,.*": description: Spansion Inc. + "^sparkfun,.*": + description: SparkFun Electronics "^sprd,.*": description: Spreadtrum Communications Inc. "^sst,.*": diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig index 5e38899058c1..7dfe8ea90923 100644 --- a/drivers/input/joystick/Kconfig +++ b/drivers/input/joystick/Kconfig @@ -372,6 +372,15 @@ config JOYSTICK_PXRC To compile this driver as a module, choose M here: the module will be called pxrc. +config JOYSTICK_QWIIC + tristate "SparkFun Qwiic Joystick" + depends on I2C + help + Say Y here if you want to use the SparkFun Qwiic Joystick. + + To compile this driver as a module, choose M here: the + module will be called qwiic-joystick. + config JOYSTICK_FSIA6B tristate "FlySky FS-iA6B RC Receiver" select SERIO diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile index 31d720c9e493..5174b8aba2dd 100644 --- a/drivers/input/joystick/Makefile +++ b/drivers/input/joystick/Makefile @@ -27,6 +27,7 @@ obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o 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_SIDEWINDER) += sidewinder.o obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o diff --git a/drivers/input/joystick/qwiic-joystick.c b/drivers/input/joystick/qwiic-joystick.c new file mode 100644 index 000000000000..9b7ab0dd1218 --- /dev/null +++ b/drivers/input/joystick/qwiic-joystick.c @@ -0,0 +1,169 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2021 Oleh Kravchenko <oleg@kaa.org.ua> + +/* + * SparkFun Qwiic Joystick + * Product page:https://www.sparkfun.com/products/15168 + * Firmware and hardware sources:https://github.com/sparkfun/Qwiic_Joystick + */ + +#include <linux/bits.h> +#include <linux/i2c.h> +#include <linux/input.h> +#include <linux/module.h> + +#define QWIIC_JSK_REG_VERS 0 +#define QWIIC_JSK_REG_DATA 3 + +#define QWIIC_JSK_MAX_AXIS GENMASK(10, 0) +#define QWIIC_JSK_FUZZ 2 +#define QWIIC_JSK_FLAT 2 + +struct qwiic_jsk { + char phys[32]; + struct input_dev *dev; + struct i2c_client *i2c; +}; + +struct qwiic_ver { + u8 addr; + u8 major; + u8 minor; +} __packed; + +struct qwiic_data { + u8 hx; + u8 lx; + u8 hy; + u8 ly; + u8 thumb; +} __packed; + +static void qwiic_poll(struct input_dev *input) +{ + struct qwiic_jsk *priv; + struct qwiic_data data; + int ret; + int x, y, btn; + + priv = input_get_drvdata(input); + + ret = i2c_smbus_read_i2c_block_data(priv->i2c, QWIIC_JSK_REG_DATA, + sizeof(data), (u8 *)&data); + if (ret == sizeof(data)) { + x = (data.hx << 8 | data.lx) >> 6; + y = (data.hy << 8 | data.ly) >> 6; + btn = !!!data.thumb; + + input_report_abs(input, ABS_X, x); + input_report_abs(input, ABS_Y, y); + input_report_key(input, BTN_THUMBL, btn); + + input_sync(input); + } +} + +static int qwiic_probe(struct i2c_client *i2c, const struct i2c_device_id *id) +{ + struct qwiic_jsk *priv; + struct qwiic_ver vers; + int ret; + + ret = i2c_smbus_read_i2c_block_data(i2c, QWIIC_JSK_REG_VERS, + sizeof(vers), (u8 *)&vers); + if (ret != sizeof(vers)) { + ret = -EIO; + goto err; + } + + if (i2c->addr != vers.addr) { + dev_err(&i2c->dev, "address doesn't match!\n"); + ret = -ENODEV; + goto err; + } + + dev_info(&i2c->dev, "SparkFun Qwiic Joystick, FW: %d.%d\n", + vers.major, vers.minor); + + priv = devm_kzalloc(&i2c->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) { + ret = -ENOMEM; + goto err; + } + + priv->i2c = i2c; + snprintf(priv->phys, sizeof(priv->phys), "i2c/%s", dev_name(&i2c->dev)); + i2c_set_clientdata(i2c, priv); + + priv->dev = devm_input_allocate_device(&i2c->dev); + if (!priv->dev) { + dev_err(&i2c->dev, "failed to allocate input device\n"); + ret = -ENOMEM; + goto err; + } + + priv->dev->dev.parent = &i2c->dev; + priv->dev->id.bustype = BUS_I2C; + priv->dev->name = "SparkFun Qwiic Joystick"; + priv->dev->phys = priv->phys; + input_set_drvdata(priv->dev, priv); + + input_set_abs_params(priv->dev, ABS_X, 0, QWIIC_JSK_MAX_AXIS, + QWIIC_JSK_FUZZ, QWIIC_JSK_FLAT); + input_set_abs_params(priv->dev, ABS_Y, 0, QWIIC_JSK_MAX_AXIS, + QWIIC_JSK_FUZZ, QWIIC_JSK_FLAT); + input_set_capability(priv->dev, EV_KEY, BTN_THUMBL); + + ret = input_setup_polling(priv->dev, qwiic_poll); + if (ret) { + dev_err(&i2c->dev, "failed to set up polling: %d\n", ret); + goto err; + } + input_set_poll_interval(priv->dev, 16); + input_set_min_poll_interval(priv->dev, 8); + input_set_max_poll_interval(priv->dev, 32); + + ret = input_register_device(priv->dev); + if (ret) + dev_err(&i2c->dev, "failed to register joystick: %d\n", ret); + +err: + return ret; +} + +static int qwiic_remove(struct i2c_client *i2c) +{ + struct qwiic_jsk *priv; + + priv = i2c_get_clientdata(i2c); + input_unregister_device(priv->dev); + + return 0; +} + +static const struct of_device_id of_qwiic_match[] = { + { .compatible = "sparkfun,qwiic-joystick", }, + {}, +}; +MODULE_DEVICE_TABLE(of, of_qwiic_match); + +static const struct i2c_device_id qwiic_id_table[] = { + { KBUILD_MODNAME, 0 }, + {}, +}; +MODULE_DEVICE_TABLE(i2c, qwiic_id_table); + +static struct i2c_driver qwiic_driver = { + .driver = { + .name = KBUILD_MODNAME, + .of_match_table = of_match_ptr(of_qwiic_match), + }, + .id_table = qwiic_id_table, + .probe = qwiic_probe, + .remove = qwiic_remove, +}; +module_i2c_driver(qwiic_driver); + +MODULE_AUTHOR("Oleh Kravchenko <oleg@kaa.org.ua>"); +MODULE_DESCRIPTION("SparkFun Qwiic Joystick driver"); +MODULE_LICENSE("GPL v2");
A simple analog joystick built on Low Power ATtiny85 Microcontroller. Directional movements are measured with two 10 kΩ potentiometers connected with a gimbal mechanism that separates the horizontal and vertical movements. This joystick also has a select button that is actuated when the joystick is pressed down. Input events polled over the I2C bus. Product page: https://www.sparkfun.com/products/15168 Firmware and hardware sources: https://github.com/sparkfun/Qwiic_Joystick Tested on RPi4B and O4-iMX-NANO boards. Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> Cc: Device Tree mailing list <devicetree@vger.kernel.org> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Jiri Kosina <jikos@jikos.cz> Cc: Patchwork Bot <patchwork-bot@kernel.org> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua> --- .../bindings/input/qwiic-joystick.yaml | 31 ++++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + drivers/input/joystick/Kconfig | 9 + drivers/input/joystick/Makefile | 1 + drivers/input/joystick/qwiic-joystick.c | 169 ++++++++++++++++++ 5 files changed, 212 insertions(+) create mode 100644 Documentation/devicetree/bindings/input/qwiic-joystick.yaml create mode 100644 drivers/input/joystick/qwiic-joystick.c