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.

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.

This should be a separate patch, please also send to Rob Herring and
device tree list.

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

Thanks.

-- 
Dmitry