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 <redacted>
Cc: Device Tree mailing list <redacted>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Jiri Kosina <redacted>
Cc: Patchwork Bot <patchwork-bot@kernel.org>
Signed-off-by: Oleh Kravchenko <redacted>
---

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.

'reg' is required as well.

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 [off-list ref]");
+MODULE_DESCRIPTION("SparkFun Qwiic Joystick driver");
+MODULE_LICENSE("GPL v2");
-- 
2.26.3

Kind regards,
Jeff LaBundy