Re: [PATCH 2/2 v4] input: add SparkFun Qwiic Joystick driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Oleh,

On Wed, Jun 09, 2021 at 01:31:32AM +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>
> Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> ---
> 
> Changes for v4:
> - alphabetize include headers.
> 
> Changes for v3:
> - rebase patch for device tree before code.
> 
> Changes for v2:
> - Separate patch for device tree bindings.
> 
>  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..557708e9e756
> --- /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/kernel.h>
> +#include <linux/module.h>
> +
> +#define DRV_NAME "qwiic-joystick"
> +
> +#define QWIIC_JSK_REG_VERS	1
> +#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 major;
> +	u8 minor;
> +};
> +
> +struct qwiic_data {
> +	__be16 x;
> +	__be16 y;
> +	u8 thumb;
> +} __packed;

The members of this structure are naturally aligned, so there is no need
to declare it as __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)

The probe() does not use the i2c_device_id parameter, so I will switch
it to probe_new() to avoid the temptation of using it in the future.

Please let me know if you disagree, otherwise I will go and apply (no
need to resubmit).

Thanks.

-- 
Dmitry