| Message ID | 20210602190504.23076-1-oleg@kaa.org.ua (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/2,v2] input: add SparkFun Qwiic Joystick driver | expand |
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
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 >
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
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");
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