All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2 v2] input: add SparkFun Qwiic Joystick driver
@ 2021-06-02 19:05 Oleh Kravchenko
  2021-06-02 19:05 ` [PATCH 2/2 v2] dt-bindings: Add vendor prefix and bindings for Qwiic Joystick Oleh Kravchenko
  2021-06-03 14:38 ` [PATCH 1/2 v2] input: add SparkFun Qwiic Joystick driver Jeff LaBundy
  0 siblings, 2 replies; 6+ messages in thread
From: Oleh Kravchenko @ 2021-06-02 19:05 UTC (permalink / raw)
  To: linux-input
  Cc: Oleh Kravchenko, Benjamin Tissoires, Device Tree mailing list,
	Dmitry Torokhov, Jiri Kosina, Patchwork Bot, Rob Herring

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>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
---

Changes:
- update code after code review

 drivers/input/joystick/Kconfig          |   9 ++
 drivers/input/joystick/Makefile         |   1 +
 drivers/input/joystick/qwiic-joystick.c | 147 ++++++++++++++++++++++++
 3 files changed, 157 insertions(+)
 create mode 100644 drivers/input/joystick/qwiic-joystick.c

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..59c0f3e6ee75
--- /dev/null
+++ b/drivers/input/joystick/qwiic-joystick.c
@@ -0,0 +1,147 @@
+// 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 DRV_NAME "qwiic-joystick"
+
+#define QWIIC_JSK_REG_VERS	0
+#define QWIIC_JSK_REG_DATA	3
+
+#define QWIIC_JSK_MAX_AXIS	GENMASK(9, 0)
+#define QWIIC_JSK_FUZZ		2
+#define QWIIC_JSK_FLAT		2
+#define QWIIC_JSK_POLL_INTERVAL	16
+#define QWIIC_JSK_POLL_MIN	8
+#define QWIIC_JSK_POLL_MAX	32
+
+struct qwiic_jsk {
+	char			phys[32];
+	struct input_dev	*dev;
+	struct i2c_client	*client;
+};
+
+struct qwiic_ver {
+	u8 addr;
+	u8 major;
+	u8 minor;
+} __packed;
+
+struct qwiic_data {
+	__be16	x;
+	__be16	y;
+	u8	thumb;
+} __packed;
+
+static void qwiic_poll(struct input_dev *input)
+{
+	struct qwiic_jsk *priv;
+	struct qwiic_data data;
+	int err;
+
+	priv = input_get_drvdata(input);
+
+	err = i2c_smbus_read_i2c_block_data(priv->client, QWIIC_JSK_REG_DATA,
+					    sizeof(data), (u8 *)&data);
+	if (err != sizeof(data))
+		return;
+
+	input_report_abs(input, ABS_X, be16_to_cpu(data.x) >> 6);
+	input_report_abs(input, ABS_Y, be16_to_cpu(data.y) >> 6);
+	input_report_key(input, BTN_THUMBL, !data.thumb);
+	input_sync(input);
+}
+
+static int qwiic_probe(struct i2c_client *client,
+		       const struct i2c_device_id *id)
+{
+	struct qwiic_jsk *priv;
+	struct qwiic_ver vers;
+	int err;
+
+	err = i2c_smbus_read_i2c_block_data(client, QWIIC_JSK_REG_VERS,
+					    sizeof(vers), (u8 *)&vers);
+	if (0 <= err && err < sizeof(vers))
+		err = -EIO;
+	if (err < 0)
+		return err;
+
+	dev_dbg(&client->dev, "SparkFun Qwiic Joystick, FW: %d.%d\n",
+		vers.major, vers.minor);
+
+	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = client;
+	snprintf(priv->phys, sizeof(priv->phys),
+		 "i2c/%s", dev_name(&client->dev));
+	i2c_set_clientdata(client, priv);
+
+	priv->dev = devm_input_allocate_device(&client->dev);
+	if (!priv->dev)
+		return -ENOMEM;
+
+	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);
+
+	err = input_setup_polling(priv->dev, qwiic_poll);
+	if (err) {
+		dev_err(&client->dev, "failed to set up polling: %d\n", err);
+		return err;
+	}
+	input_set_poll_interval(priv->dev, QWIIC_JSK_POLL_INTERVAL);
+	input_set_min_poll_interval(priv->dev, QWIIC_JSK_POLL_MIN);
+	input_set_max_poll_interval(priv->dev, QWIIC_JSK_POLL_MAX);
+
+	err = input_register_device(priv->dev);
+	if (err)
+		dev_err(&client->dev, "failed to register joystick: %d\n", err);
+
+	return err;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_qwiic_match[] = {
+	{ .compatible = "sparkfun,qwiic-joystick", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_qwiic_match);
+#endif /* CONFIG_OF */
+
+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		= DRV_NAME,
+		.of_match_table	= of_match_ptr(of_qwiic_match),
+	},
+	.id_table	= qwiic_id_table,
+	.probe		= qwiic_probe,
+};
+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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2 v2] dt-bindings: Add vendor prefix and bindings for Qwiic Joystick
  2021-06-02 19:05 [PATCH 1/2 v2] input: add SparkFun Qwiic Joystick driver Oleh Kravchenko
@ 2021-06-02 19:05 ` Oleh Kravchenko
  2021-06-03  7:16   ` Oleh Kravchenko
  2021-06-03 14:38 ` [PATCH 1/2 v2] input: add SparkFun Qwiic Joystick driver Jeff LaBundy
  1 sibling, 1 reply; 6+ messages in thread
From: Oleh Kravchenko @ 2021-06-02 19:05 UTC (permalink / raw)
  To: linux-input
  Cc: Oleh Kravchenko, Benjamin Tissoires, Device Tree mailing list,
	Dmitry Torokhov, Jiri Kosina, Patchwork Bot, Rob Herring

Add vendor prefix for SparkFun Electronics.
Update trivial-devices.yaml with SparkFun Qwiic Joystick description.

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>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
---

Changes:
- Separate patch for device tree bindings.

 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index a327130d1faa..706aa102d96d 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -261,6 +261,8 @@ properties:
             # Socionext SynQuacer TPM MMIO module
           - socionext,synquacer-tpm-mmio
             # i2c serial eeprom  (24cxx)
+          - sparkfun,qwiic-joystick
+            # SparkFun Qwiic Joystick (COM-15168) with i2c interface
           - st,24c256
             # Ambient Light Sensor with SMBUS/Two Wire Serial Interface
           - taos,tsl2550
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,.*":
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2 v2] dt-bindings: Add vendor prefix and bindings for Qwiic Joystick
  2021-06-02 19:05 ` [PATCH 2/2 v2] dt-bindings: Add vendor prefix and bindings for Qwiic Joystick Oleh Kravchenko
@ 2021-06-03  7:16   ` Oleh Kravchenko
  0 siblings, 0 replies; 6+ messages in thread
From: Oleh Kravchenko @ 2021-06-03  7:16 UTC (permalink / raw)
  To: Rob Herring, Dmitry Torokhov, Jeff LaBundy
  Cc: Benjamin Tissoires, linux-input, Device Tree mailing list,
	Jiri Kosina, Patchwork Bot

Hello Dmitry, Jeff, Rob,

Thank you for reviewing the previous patch.
I've fixed all your comments and split the patch into two.

Could you please review these too?

02.06.21 22:05, Oleh Kravchenko пише:
> Add vendor prefix for SparkFun Electronics.
> Update trivial-devices.yaml with SparkFun Qwiic Joystick description.
> 
> 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>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> ---
> 
> Changes:
> - Separate patch for device tree bindings.
> 
>  Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index a327130d1faa..706aa102d96d 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -261,6 +261,8 @@ properties:
>              # Socionext SynQuacer TPM MMIO module
>            - socionext,synquacer-tpm-mmio
>              # i2c serial eeprom  (24cxx)
> +          - sparkfun,qwiic-joystick
> +            # SparkFun Qwiic Joystick (COM-15168) with i2c interface
>            - st,24c256
>              # Ambient Light Sensor with SMBUS/Two Wire Serial Interface
>            - taos,tsl2550
> 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,.*":
> 

-- 
Best regards,
Oleh Kravchenko


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2 v2] input: add SparkFun Qwiic Joystick driver
  2021-06-02 19:05 [PATCH 1/2 v2] input: add SparkFun Qwiic Joystick driver Oleh Kravchenko
  2021-06-02 19:05 ` [PATCH 2/2 v2] dt-bindings: Add vendor prefix and bindings for Qwiic Joystick Oleh Kravchenko
@ 2021-06-03 14:38 ` Jeff LaBundy
  2021-06-03 21:54   ` Oleh Kravchenko
  1 sibling, 1 reply; 6+ messages in thread
From: Jeff LaBundy @ 2021-06-03 14:38 UTC (permalink / raw)
  To: Oleh Kravchenko
  Cc: linux-input, Benjamin Tissoires, Device Tree mailing list,
	Dmitry Torokhov, Jiri Kosina, Patchwork Bot, Rob Herring

Hi Oleh,

This cleaned up rather nicely; just a few more comments from myself.

On Wed, Jun 02, 2021 at 10:05:04PM +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>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>

Typically a binding patch should be first in the series (i.e. [1/2])
with the driver next (i.e. [2/2]) so that checkpatch does not signal
a warning about an undocumented compatible string.

> ---
> 
> Changes:
> - update code after code review
> 
>  drivers/input/joystick/Kconfig          |   9 ++
>  drivers/input/joystick/Makefile         |   1 +
>  drivers/input/joystick/qwiic-joystick.c | 147 ++++++++++++++++++++++++
>  3 files changed, 157 insertions(+)
>  create mode 100644 drivers/input/joystick/qwiic-joystick.c
> 
> 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..59c0f3e6ee75
> --- /dev/null
> +++ b/drivers/input/joystick/qwiic-joystick.c
> @@ -0,0 +1,147 @@
> +// 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>

Now that you are using byte order macros, you should explicitly include
linux/kernel.h.

> +#include <linux/module.h>
> +
> +#define DRV_NAME "qwiic-joystick"
> +
> +#define QWIIC_JSK_REG_VERS	0
> +#define QWIIC_JSK_REG_DATA	3
> +
> +#define QWIIC_JSK_MAX_AXIS	GENMASK(9, 0)
> +#define QWIIC_JSK_FUZZ		2
> +#define QWIIC_JSK_FLAT		2
> +#define QWIIC_JSK_POLL_INTERVAL	16
> +#define QWIIC_JSK_POLL_MIN	8
> +#define QWIIC_JSK_POLL_MAX	32
> +
> +struct qwiic_jsk {
> +	char			phys[32];
> +	struct input_dev	*dev;
> +	struct i2c_client	*client;
> +};

Nit: no need to align these declarations (here and a few other places).

> +
> +struct qwiic_ver {
> +	u8 addr;
> +	u8 major;
> +	u8 minor;
> +} __packed;

'addr' is unused so it can be dropped, in which case QWIIC_JSK_REG_VERS
would need to be increased to 1.

> +
> +struct qwiic_data {
> +	__be16	x;
> +	__be16	y;
> +	u8	thumb;
> +} __packed;
> +
> +static void qwiic_poll(struct input_dev *input)
> +{
> +	struct qwiic_jsk *priv;
> +	struct qwiic_data data;
> +	int err;
> +
> +	priv = input_get_drvdata(input);
> +
> +	err = i2c_smbus_read_i2c_block_data(priv->client, QWIIC_JSK_REG_DATA,
> +					    sizeof(data), (u8 *)&data);
> +	if (err != sizeof(data))
> +		return;
> +
> +	input_report_abs(input, ABS_X, be16_to_cpu(data.x) >> 6);
> +	input_report_abs(input, ABS_Y, be16_to_cpu(data.y) >> 6);
> +	input_report_key(input, BTN_THUMBL, !data.thumb);
> +	input_sync(input);
> +}
> +
> +static int qwiic_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *id)
> +{
> +	struct qwiic_jsk *priv;
> +	struct qwiic_ver vers;
> +	int err;
> +
> +	err = i2c_smbus_read_i2c_block_data(client, QWIIC_JSK_REG_VERS,
> +					    sizeof(vers), (u8 *)&vers);
> +	if (0 <= err && err < sizeof(vers))
> +		err = -EIO;
> +	if (err < 0)
> +		return err;

I think this reads a little simpler as:

if (err < 0)
	return err;
if (err != sizeof(vers))
	return -EIO;

> +
> +	dev_dbg(&client->dev, "SparkFun Qwiic Joystick, FW: %d.%d\n",
> +		vers.major, vers.minor);

These should be printed as %u.

> +
> +	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->client = client;
> +	snprintf(priv->phys, sizeof(priv->phys),
> +		 "i2c/%s", dev_name(&client->dev));
> +	i2c_set_clientdata(client, priv);
> +
> +	priv->dev = devm_input_allocate_device(&client->dev);
> +	if (!priv->dev)
> +		return -ENOMEM;
> +
> +	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);
> +
> +	err = input_setup_polling(priv->dev, qwiic_poll);
> +	if (err) {
> +		dev_err(&client->dev, "failed to set up polling: %d\n", err);
> +		return err;
> +	}
> +	input_set_poll_interval(priv->dev, QWIIC_JSK_POLL_INTERVAL);
> +	input_set_min_poll_interval(priv->dev, QWIIC_JSK_POLL_MIN);
> +	input_set_max_poll_interval(priv->dev, QWIIC_JSK_POLL_MAX);
> +
> +	err = input_register_device(priv->dev);
> +	if (err)
> +		dev_err(&client->dev, "failed to register joystick: %d\n", err);
> +
> +	return err;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_qwiic_match[] = {
> +	{ .compatible = "sparkfun,qwiic-joystick", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, of_qwiic_match);
> +#endif /* CONFIG_OF */
> +
> +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		= DRV_NAME,
> +		.of_match_table	= of_match_ptr(of_qwiic_match),
> +	},
> +	.id_table	= qwiic_id_table,
> +	.probe		= qwiic_probe,
> +};
> +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
> 

Kind regards,
Jeff LaBundy

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2 v2] input: add SparkFun Qwiic Joystick driver
  2021-06-03 14:38 ` [PATCH 1/2 v2] input: add SparkFun Qwiic Joystick driver Jeff LaBundy
@ 2021-06-03 21:54   ` Oleh Kravchenko
  2021-06-06  1:49     ` Jeff LaBundy
  0 siblings, 1 reply; 6+ messages in thread
From: Oleh Kravchenko @ 2021-06-03 21:54 UTC (permalink / raw)
  To: Jeff LaBundy
  Cc: linux-input, Benjamin Tissoires, Device Tree mailing list,
	Dmitry Torokhov, Jiri Kosina, Patchwork Bot, Rob Herring

Hello Jeff,
thank you for your code-review.

Could you please clarify a few things below?
Just for my better understanding.


03.06.21 17:38, Jeff LaBundy пише:
> Hi Oleh,
> 
> This cleaned up rather nicely; just a few more comments from myself.
> 
> On Wed, Jun 02, 2021 at 10:05:04PM +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>
>> Cc: Rob Herring <robh@kernel.org>
>> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> 
> Typically a binding patch should be first in the series (i.e. [1/2])
> with the driver next (i.e. [2/2]) so that checkpatch does not signal
> a warning about an undocumented compatible string.
> 
>> ---
>>
>> Changes:
>> - update code after code review
>>
>>  drivers/input/joystick/Kconfig          |   9 ++
>>  drivers/input/joystick/Makefile         |   1 +
>>  drivers/input/joystick/qwiic-joystick.c | 147 ++++++++++++++++++++++++
>>  3 files changed, 157 insertions(+)
>>  create mode 100644 drivers/input/joystick/qwiic-joystick.c
>>
>> 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..59c0f3e6ee75
>> --- /dev/null
>> +++ b/drivers/input/joystick/qwiic-joystick.c
>> @@ -0,0 +1,147 @@
>> +// 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>
> 
> Now that you are using byte order macros, you should explicitly include
> linux/kernel.h.

Where is requirement is described?
I've found a lot of drivers without including linux/kernel.h but linux/bits.h

>> +#include <linux/module.h>
>> +
>> +#define DRV_NAME "qwiic-joystick"
>> +
>> +#define QWIIC_JSK_REG_VERS	0
>> +#define QWIIC_JSK_REG_DATA	3
>> +
>> +#define QWIIC_JSK_MAX_AXIS	GENMASK(9, 0)
>> +#define QWIIC_JSK_FUZZ		2
>> +#define QWIIC_JSK_FLAT		2
>> +#define QWIIC_JSK_POLL_INTERVAL	16
>> +#define QWIIC_JSK_POLL_MIN	8
>> +#define QWIIC_JSK_POLL_MAX	32
>> +
>> +struct qwiic_jsk {
>> +	char			phys[32];
>> +	struct input_dev	*dev;
>> +	struct i2c_client	*client;
>> +};
> 
> Nit: no need to align these declarations (here and a few other places).
> 
>> +
>> +struct qwiic_ver {
>> +	u8 addr;
>> +	u8 major;
>> +	u8 minor;
>> +} __packed;
> 
> 'addr' is unused so it can be dropped, in which case QWIIC_JSK_REG_VERS
> would need to be increased to 1.
> 
>> +
>> +struct qwiic_data {
>> +	__be16	x;
>> +	__be16	y;
>> +	u8	thumb;
>> +} __packed;
>> +
>> +static void qwiic_poll(struct input_dev *input)
>> +{
>> +	struct qwiic_jsk *priv;
>> +	struct qwiic_data data;
>> +	int err;
>> +
>> +	priv = input_get_drvdata(input);
>> +
>> +	err = i2c_smbus_read_i2c_block_data(priv->client, QWIIC_JSK_REG_DATA,
>> +					    sizeof(data), (u8 *)&data);
>> +	if (err != sizeof(data))
>> +		return;
>> +
>> +	input_report_abs(input, ABS_X, be16_to_cpu(data.x) >> 6);
>> +	input_report_abs(input, ABS_Y, be16_to_cpu(data.y) >> 6);
>> +	input_report_key(input, BTN_THUMBL, !data.thumb);
>> +	input_sync(input);
>> +}
>> +
>> +static int qwiic_probe(struct i2c_client *client,
>> +		       const struct i2c_device_id *id)
>> +{
>> +	struct qwiic_jsk *priv;
>> +	struct qwiic_ver vers;
>> +	int err;
>> +
>> +	err = i2c_smbus_read_i2c_block_data(client, QWIIC_JSK_REG_VERS,
>> +					    sizeof(vers), (u8 *)&vers);
>> +	if (0 <= err && err < sizeof(vers))
>> +		err = -EIO;
>> +	if (err < 0)
>> +		return err;
> 
> I think this reads a little simpler as:
> 
> if (err < 0)
> 	return err;
> if (err != sizeof(vers))
> 	return -EIO;
> 

Can we sure that i2c_smbus_read_i2c_block_data() will read exact numbers of bytes?
And if it read less, will return it an error?

>> +
>> +	dev_dbg(&client->dev, "SparkFun Qwiic Joystick, FW: %d.%d\n",
>> +		vers.major, vers.minor);
> 
> These should be printed as %u.
> 
>> +
>> +	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->client = client;
>> +	snprintf(priv->phys, sizeof(priv->phys),
>> +		 "i2c/%s", dev_name(&client->dev));
>> +	i2c_set_clientdata(client, priv);
>> +
>> +	priv->dev = devm_input_allocate_device(&client->dev);
>> +	if (!priv->dev)
>> +		return -ENOMEM;
>> +
>> +	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);
>> +
>> +	err = input_setup_polling(priv->dev, qwiic_poll);
>> +	if (err) {
>> +		dev_err(&client->dev, "failed to set up polling: %d\n", err);
>> +		return err;
>> +	}
>> +	input_set_poll_interval(priv->dev, QWIIC_JSK_POLL_INTERVAL);
>> +	input_set_min_poll_interval(priv->dev, QWIIC_JSK_POLL_MIN);
>> +	input_set_max_poll_interval(priv->dev, QWIIC_JSK_POLL_MAX);
>> +
>> +	err = input_register_device(priv->dev);
>> +	if (err)
>> +		dev_err(&client->dev, "failed to register joystick: %d\n", err);
>> +
>> +	return err;
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id of_qwiic_match[] = {
>> +	{ .compatible = "sparkfun,qwiic-joystick", },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, of_qwiic_match);
>> +#endif /* CONFIG_OF */
>> +
>> +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		= DRV_NAME,
>> +		.of_match_table	= of_match_ptr(of_qwiic_match),
>> +	},
>> +	.id_table	= qwiic_id_table,
>> +	.probe		= qwiic_probe,
>> +};
>> +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
>>
> 
> Kind regards,
> Jeff LaBundy
> 

-- 
Best regards,
Oleh Kravchenko


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2 v2] input: add SparkFun Qwiic Joystick driver
  2021-06-03 21:54   ` Oleh Kravchenko
@ 2021-06-06  1:49     ` Jeff LaBundy
  0 siblings, 0 replies; 6+ messages in thread
From: Jeff LaBundy @ 2021-06-06  1:49 UTC (permalink / raw)
  To: Oleh Kravchenko
  Cc: linux-input, Benjamin Tissoires, Device Tree mailing list,
	Dmitry Torokhov, Jiri Kosina, Patchwork Bot, Rob Herring

Hi Oleh,

On Fri, Jun 04, 2021 at 12:54:42AM +0300, Oleh Kravchenko wrote:
> Hello Jeff,
> thank you for your code-review.
> 
> Could you please clarify a few things below?
> Just for my better understanding.
> 
> 
> 03.06.21 17:38, Jeff LaBundy пише:
> > Hi Oleh,
> > 
> > This cleaned up rather nicely; just a few more comments from myself.
> > 
> > On Wed, Jun 02, 2021 at 10:05:04PM +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>
> >> Cc: Rob Herring <robh@kernel.org>
> >> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> > 
> > Typically a binding patch should be first in the series (i.e. [1/2])
> > with the driver next (i.e. [2/2]) so that checkpatch does not signal
> > a warning about an undocumented compatible string.
> > 
> >> ---
> >>
> >> Changes:
> >> - update code after code review
> >>
> >>  drivers/input/joystick/Kconfig          |   9 ++
> >>  drivers/input/joystick/Makefile         |   1 +
> >>  drivers/input/joystick/qwiic-joystick.c | 147 ++++++++++++++++++++++++
> >>  3 files changed, 157 insertions(+)
> >>  create mode 100644 drivers/input/joystick/qwiic-joystick.c
> >>
> >> 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..59c0f3e6ee75
> >> --- /dev/null
> >> +++ b/drivers/input/joystick/qwiic-joystick.c
> >> @@ -0,0 +1,147 @@
> >> +// 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>
> > 
> > Now that you are using byte order macros, you should explicitly include
> > linux/kernel.h.
> 
> Where is requirement is described?
> I've found a lot of drivers without including linux/kernel.h but linux/bits.h

It's actually asm/byteorder.h that defines these; I mention linux/kernel.h
as it includes it anyway. I think either is fine for this driver.

> 
> >> +#include <linux/module.h>
> >> +
> >> +#define DRV_NAME "qwiic-joystick"
> >> +
> >> +#define QWIIC_JSK_REG_VERS	0
> >> +#define QWIIC_JSK_REG_DATA	3
> >> +
> >> +#define QWIIC_JSK_MAX_AXIS	GENMASK(9, 0)
> >> +#define QWIIC_JSK_FUZZ		2
> >> +#define QWIIC_JSK_FLAT		2
> >> +#define QWIIC_JSK_POLL_INTERVAL	16
> >> +#define QWIIC_JSK_POLL_MIN	8
> >> +#define QWIIC_JSK_POLL_MAX	32
> >> +
> >> +struct qwiic_jsk {
> >> +	char			phys[32];
> >> +	struct input_dev	*dev;
> >> +	struct i2c_client	*client;
> >> +};
> > 
> > Nit: no need to align these declarations (here and a few other places).
> > 
> >> +
> >> +struct qwiic_ver {
> >> +	u8 addr;
> >> +	u8 major;
> >> +	u8 minor;
> >> +} __packed;
> > 
> > 'addr' is unused so it can be dropped, in which case QWIIC_JSK_REG_VERS
> > would need to be increased to 1.
> > 
> >> +
> >> +struct qwiic_data {
> >> +	__be16	x;
> >> +	__be16	y;
> >> +	u8	thumb;
> >> +} __packed;
> >> +
> >> +static void qwiic_poll(struct input_dev *input)
> >> +{
> >> +	struct qwiic_jsk *priv;
> >> +	struct qwiic_data data;
> >> +	int err;
> >> +
> >> +	priv = input_get_drvdata(input);
> >> +
> >> +	err = i2c_smbus_read_i2c_block_data(priv->client, QWIIC_JSK_REG_DATA,
> >> +					    sizeof(data), (u8 *)&data);
> >> +	if (err != sizeof(data))
> >> +		return;
> >> +
> >> +	input_report_abs(input, ABS_X, be16_to_cpu(data.x) >> 6);
> >> +	input_report_abs(input, ABS_Y, be16_to_cpu(data.y) >> 6);
> >> +	input_report_key(input, BTN_THUMBL, !data.thumb);
> >> +	input_sync(input);
> >> +}
> >> +
> >> +static int qwiic_probe(struct i2c_client *client,
> >> +		       const struct i2c_device_id *id)
> >> +{
> >> +	struct qwiic_jsk *priv;
> >> +	struct qwiic_ver vers;
> >> +	int err;
> >> +
> >> +	err = i2c_smbus_read_i2c_block_data(client, QWIIC_JSK_REG_VERS,
> >> +					    sizeof(vers), (u8 *)&vers);
> >> +	if (0 <= err && err < sizeof(vers))
> >> +		err = -EIO;
> >> +	if (err < 0)
> >> +		return err;
> > 
> > I think this reads a little simpler as:
> > 
> > if (err < 0)
> > 	return err;
> > if (err != sizeof(vers))
> > 	return -EIO;
> > 
> 
> Can we sure that i2c_smbus_read_i2c_block_data() will read exact numbers of bytes?
> And if it read less, will return it an error?

I think it's highly unlikely an adapter would perform an incomplete transfer
without returning an error. However, this behavior is technically defined by
the adapter itself.

As such, I think it's prudent to validate the entire range of return values.
If for some reason i2c_smbus_read_i2c_block_data() returned 1, vers.minor is
garbage and you must bail or retry.

My point from your original patch was that you should not squash every value
below sizeof(vers) as -EIO. In case i2c_smbus_read_i2c_block_data() returned
a different negative value, you should return that from probe.

> 
> >> +
> >> +	dev_dbg(&client->dev, "SparkFun Qwiic Joystick, FW: %d.%d\n",
> >> +		vers.major, vers.minor);
> > 
> > These should be printed as %u.
> > 
> >> +
> >> +	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> >> +	if (!priv)
> >> +		return -ENOMEM;
> >> +
> >> +	priv->client = client;
> >> +	snprintf(priv->phys, sizeof(priv->phys),
> >> +		 "i2c/%s", dev_name(&client->dev));
> >> +	i2c_set_clientdata(client, priv);
> >> +
> >> +	priv->dev = devm_input_allocate_device(&client->dev);
> >> +	if (!priv->dev)
> >> +		return -ENOMEM;
> >> +
> >> +	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);
> >> +
> >> +	err = input_setup_polling(priv->dev, qwiic_poll);
> >> +	if (err) {
> >> +		dev_err(&client->dev, "failed to set up polling: %d\n", err);
> >> +		return err;
> >> +	}
> >> +	input_set_poll_interval(priv->dev, QWIIC_JSK_POLL_INTERVAL);
> >> +	input_set_min_poll_interval(priv->dev, QWIIC_JSK_POLL_MIN);
> >> +	input_set_max_poll_interval(priv->dev, QWIIC_JSK_POLL_MAX);
> >> +
> >> +	err = input_register_device(priv->dev);
> >> +	if (err)
> >> +		dev_err(&client->dev, "failed to register joystick: %d\n", err);
> >> +
> >> +	return err;
> >> +}
> >> +
> >> +#ifdef CONFIG_OF
> >> +static const struct of_device_id of_qwiic_match[] = {
> >> +	{ .compatible = "sparkfun,qwiic-joystick", },
> >> +	{ },
> >> +};
> >> +MODULE_DEVICE_TABLE(of, of_qwiic_match);
> >> +#endif /* CONFIG_OF */
> >> +
> >> +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		= DRV_NAME,
> >> +		.of_match_table	= of_match_ptr(of_qwiic_match),
> >> +	},
> >> +	.id_table	= qwiic_id_table,
> >> +	.probe		= qwiic_probe,
> >> +};
> >> +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
> >>
> > 
> > Kind regards,
> > Jeff LaBundy
> > 
> 
> -- 
> Best regards,
> Oleh Kravchenko
> 

Kind regards,
Jeff LaBundy

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-06-06  1:49 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-02 19:05 [PATCH 1/2 v2] input: add SparkFun Qwiic Joystick driver Oleh Kravchenko
2021-06-02 19:05 ` [PATCH 2/2 v2] dt-bindings: Add vendor prefix and bindings for Qwiic Joystick Oleh Kravchenko
2021-06-03  7:16   ` Oleh Kravchenko
2021-06-03 14:38 ` [PATCH 1/2 v2] input: add SparkFun Qwiic Joystick driver Jeff LaBundy
2021-06-03 21:54   ` Oleh Kravchenko
2021-06-06  1:49     ` Jeff LaBundy

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.