Re: [PATCH v4] input: tablet: add Pegasus Notetaker tablet driver

From: Dmitry Torokhov
Date: Wed May 25 2016 - 20:29:33 EST

Hi Martin,

On Wed, May 25, 2016 at 09:44:34AM +0200, Martin Kepplinger wrote:
> This adds a driver for the Pegasus Notetaker Pen. When connected,
> this uses the Pen as an input tablet.
>
> This device was sold in various different brandings, for example
> "Pegasus Mobile Notetaker M210",
> "Genie e-note The Notetaker",
> "Staedtler Digital ballpoint pen 990 01",
> "IRISnotes Express" or
> "NEWLink Digital Note Taker".
>
> Here's an example, so that you know what we are talking about:
> http://www.staedtler.com/en/products/ink-writing-instruments/ballpoint-pens/digital-pen-990-01-digital-ballpoint-pen
>
> http://pegatech.blogspot.com/ seems to be a remaining official resource.
>
> This device can also transfer saved (offline recorded handwritten) data and
> there are userspace programs that do this, see https://launchpad.net/m210
> (Well, alternatively there are really fast scanners out there :)
>
> It's *really* fun to use as an input tablet though! So let's support this
> for everybody.
>
> There's no way to disable the device. When the pen is out of range, we just
> don't get any URBs and don't do anything.
> Like all other mouses or input tablets, we don't use runtime PM.
>
> Signed-off-by: Martin Kepplinger <martink@xxxxxxxxx>
> ---
>
> Thanks for having a look. Any more suggestions on this?
>
> revision history
> ================
> v4 use normal work queue instead of a kernel thread (thanks to Oliver Neukum)
> v3 fix reporting low pen battery and add USB list to CC
> v2 minor cleanup (remove unnecessary variables)
> v1 initial release
>
>
>
> drivers/input/tablet/Kconfig | 15 ++
> drivers/input/tablet/Makefile | 1 +
> drivers/input/tablet/pegasus_notetaker.c | 373 +++++++++++++++++++++++++++++++
> 3 files changed, 389 insertions(+)
> create mode 100644 drivers/input/tablet/pegasus_notetaker.c
>
> diff --git a/drivers/input/tablet/Kconfig b/drivers/input/tablet/Kconfig
> index 623bb9e..a2b9f97 100644
> --- a/drivers/input/tablet/Kconfig
> +++ b/drivers/input/tablet/Kconfig
> @@ -73,6 +73,21 @@ config TABLET_USB_KBTAB
> To compile this driver as a module, choose M here: the
> module will be called kbtab.
>
> +config TABLET_USB_PEGASUS
> + tristate "Pegasus Mobile Notetaker Pen input tablet support"
> + depends on USB_ARCH_HAS_HCD
> + select USB
> + help
> + Say Y here if you want to use the Pegasus Mobile Notetaker,
> + also known as:
> + Genie e-note The Notetaker,
> + Staedtler Digital ballpoint pen 990 01,
> + IRISnotes Express or
> + NEWLink Digital Note Taker.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called pegasus_notetaker.
> +
> config TABLET_SERIAL_WACOM4
> tristate "Wacom protocol 4 serial tablet support"
> select SERIO
> diff --git a/drivers/input/tablet/Makefile b/drivers/input/tablet/Makefile
> index 2e13010..200fc4e 100644
> --- a/drivers/input/tablet/Makefile
> +++ b/drivers/input/tablet/Makefile
> @@ -8,4 +8,5 @@ obj-$(CONFIG_TABLET_USB_AIPTEK) += aiptek.o
> obj-$(CONFIG_TABLET_USB_GTCO) += gtco.o
> obj-$(CONFIG_TABLET_USB_HANWANG) += hanwang.o
> obj-$(CONFIG_TABLET_USB_KBTAB) += kbtab.o
> +obj-$(CONFIG_TABLET_USB_PEGASUS) += pegasus_notetaker.o
> obj-$(CONFIG_TABLET_SERIAL_WACOM4) += wacom_serial4.o
> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
> new file mode 100644
> index 0000000..b7bf585
> --- /dev/null
> +++ b/drivers/input/tablet/pegasus_notetaker.c
> @@ -0,0 +1,373 @@
> +/*
> + * Pegasus Mobile Notetaker Pen input tablet driver
> + *
> + * Copyright (c) 2016 Martin Kepplinger <martink@xxxxxxxxx>
> + */
> +
> +/*
> + * request packet (control endpoint):
> + * |-------------------------------------|
> + * | Report ID | Nr of bytes | command |
> + * | (1 byte) | (1 byte) | (n bytes) |
> + * |-------------------------------------|
> + * | 0x02 | n | |
> + * |-------------------------------------|
> + *
> + * data packet after set xy mode command, 0x80 0xb5 0x02 0x01
> + * and pen is in range:
> + *
> + * byte byte name value (bits)
> + * --------------------------------------------
> + * 0 status 0 1 0 0 0 0 X X
> + * 1 color 0 0 0 0 H 0 S T
> + * 2 X low
> + * 3 X high
> + * 4 Y low
> + * 5 Y high
> + *
> + * X X battery state:
> + * no state reported 0x00
> + * battery low 0x01
> + * battery good 0x02
> + *
> + * H Hovering
> + * S Switch 1 (pen button)
> + * T Tip
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/hid.h>
> +#include <linux/usb/input.h>
> +
> +/* USB HID defines */
> +#define USB_REQ_GET_REPORT 0x01
> +#define USB_REQ_SET_REPORT 0x09
> +
> +#define USB_VENDOR_ID_PEGASUSTECH 0x0e20
> +#define USB_DEVICE_ID_PEGASUS_NOTETAKER_EN100 0x0101
> +
> +/* device specific defines */
> +#define NOTETAKER_REPORT_ID 0x02
> +#define NOTETAKER_SET_CMD 0x80
> +#define NOTETAKER_SET_MODE 0xb5
> +
> +#define NOTETAKER_LED_MOUSE 0x02
> +#define PEN_MODE_XY 0x01
> +
> +#define SPECIAL_COMMAND 0x80
> +#define BUTTON_PRESSED 0xb5
> +#define COMMAND_VERSION 0xa9
> +
> +/* in xy data packet */
> +#define BATTERY_NO_REPORT 0x40
> +#define BATTERY_LOW 0x41
> +#define BATTERY_GOOD 0x42
> +#define PEN_BUTTON_PRESSED BIT(1)
> +#define PEN_TIP BIT(0)
> +
> +struct pegasus {
> + unsigned char *data;
> + u8 data_len;
> + dma_addr_t data_dma;
> + struct input_dev *dev;
> + struct usb_device *usbdev;
> + struct urb *irq;
> + char name[128];
> + char phys[64];
> + struct work_struct init;
> +};
> +
> +static void pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
> +{
> + const int sizeof_buf = len * sizeof(u8) + 2;
> + int result;
> + u8 *cmd_buf;
> +
> + cmd_buf = kmalloc(sizeof_buf, GFP_KERNEL);
> + if (!cmd_buf)
> + return;
> +
> + cmd_buf[0] = NOTETAKER_REPORT_ID;
> + cmd_buf[1] = len;
> + memcpy(cmd_buf + 2, data, len);
> +
> + result = usb_control_msg(pegasus->usbdev,
> + usb_sndctrlpipe(pegasus->usbdev, 0),
> + USB_REQ_SET_REPORT,
> + USB_TYPE_VENDOR | USB_DIR_OUT,
> + 0, 0, cmd_buf, sizeof_buf,
> + USB_CTRL_SET_TIMEOUT);
> +
> + if (result != sizeof_buf)
> + dev_err(&pegasus->usbdev->dev, "control msg error\n");
> +
> + kfree(cmd_buf);
> +}
> +
> +static void pegasus_set_mode(struct pegasus *pegasus, u8 mode, u8 led)
> +{
> + static u8 cmd[4] = {NOTETAKER_SET_CMD, NOTETAKER_SET_MODE, 0x00, 0x00};

"static" is bad for drivers, what if you connect 2 devices? Do:

u8 cmd[] = { NOTETAKER_SET_CMD, NOTETAKER_SET_MODE, led, mode };

instead.

> +
> + cmd[2] = led;
> + cmd[3] = mode;
> +
> + pegasus_control_msg(pegasus, cmd, sizeof(cmd));
> +}
> +
> +static void pegasus_parse_packet(struct pegasus *pegasus)
> +{
> + unsigned char *data = pegasus->data;
> + struct input_dev *dev = pegasus->dev;
> + u16 x, y = 0;

Why do you initialize y but not x (none is actually needed).

> +
> + switch (data[0]) {
> + case SPECIAL_COMMAND:
> + /* device button pressed */
> + if (data[1] == BUTTON_PRESSED)
> + schedule_work(&pegasus->init);
> +
> + break;
> + /* xy data */
> + case BATTERY_LOW:
> + dev_warn_once(&dev->dev, "Pen battery low\n");
> + case BATTERY_NO_REPORT:
> + case BATTERY_GOOD:
> + memcpy(&x, data + 2, 2);
> + memcpy(&y, data + 4, 2);

Why not

x = le16_to_cpup(data + 2);
y = le16_to_cpup(data + 4);

> +
> + /* ignore pen up events */
> + if (x == 0 && y == 0)
> + break;
> +
> + if (data[1] & PEN_BUTTON_PRESSED) {
> + input_report_key(dev, BTN_TOUCH, 0);
> + input_report_key(dev, BTN_RIGHT, 1);
> + } else if (data[1] & PEN_TIP) {
> + input_report_key(dev, BTN_TOUCH, 1);
> + input_report_key(dev, BTN_RIGHT, 0);
> + } else {
> + input_report_key(dev, BTN_TOUCH, 0);
> + input_report_key(dev, BTN_RIGHT, 0);
> + }

Can we say:

input_report_key(dev, BTN_TOUCH, data[1] & PEN_TIP);
input_report_key(dev, BTN_RIGHT, data[1] & PEN_BUTTON_PRESSED);

instead?

> +
> + input_report_key(dev, BTN_TOOL_PEN, 1);
> + input_report_abs(dev, ABS_X, (s16)le16_to_cpu(x));
> + input_report_abs(dev, ABS_Y, le16_to_cpu(y));
> +
> + input_sync(dev);
> + break;
> + default:
> + dev_warn_once(&pegasus->usbdev->dev,
> + "unknown answer from device\n");
> + }
> +}
> +
> +static void pegasus_irq(struct urb *urb)
> +{
> + struct pegasus *pegasus = urb->context;
> + struct usb_device *dev = pegasus->usbdev;
> + int retval;
> +
> + switch (urb->status) {
> + case 0:
> + pegasus_parse_packet(pegasus);
> + usb_mark_last_busy(pegasus->usbdev);
> +
> + break;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + dev_err(&dev->dev, "%s - urb shutting down with status: %d",
> + __func__, urb->status);
> + return;
> + default:
> + dev_err(&dev->dev, "%s - nonzero urb status received: %d",
> + __func__, urb->status);
> + break;
> + }
> +
> + retval = usb_submit_urb(urb, GFP_ATOMIC);
> + if (retval)
> + dev_err(&dev->dev, "%s - usb_submit_urb failed with result %d",
> + __func__, retval);
> +}
> +
> +static void pegasus_init(struct work_struct *work)
> +{
> + struct pegasus *pegasus = container_of(work, struct pegasus, init);
> +
> + pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
> +}
> +
> +static int pegasus_open(struct input_dev *dev)
> +{
> + struct pegasus *pegasus = input_get_drvdata(dev);
> +
> + pegasus->irq->dev = pegasus->usbdev;

Is this assignment really needed?

> + if (usb_submit_urb(pegasus->irq, GFP_KERNEL))
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static void pegasus_close(struct input_dev *dev)
> +{
> + struct pegasus *pegasus = input_get_drvdata(dev);
> +
> + cancel_work_sync(&pegasus->init);
> +
> + usb_kill_urb(pegasus->irq);

First kill, then cancel, as Oliver mentioned.

> +}
> +
> +static int pegasus_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct usb_device *dev = interface_to_usbdev(intf);
> + struct usb_endpoint_descriptor *endpoint;
> + struct pegasus *pegasus;
> + struct input_dev *input_dev;
> + int error;
> + int pipe, maxp;
> +
> + /* we control interface 0 */
> + if (intf->cur_altsetting->desc.bInterfaceNumber == 1)
> + return -ENODEV;
> +
> + endpoint = &intf->cur_altsetting->endpoint[0].desc;
> +
> + pegasus = kzalloc(sizeof(*pegasus), GFP_KERNEL);
> + input_dev = input_allocate_device();
> + if (!pegasus || !input_dev) {
> + error = -ENOMEM;
> + goto err_free_mem;
> + }
> +
> + pegasus->usbdev = dev;
> + pegasus->dev = input_dev;
> +
> + pegasus->data = usb_alloc_coherent(dev, endpoint->wMaxPacketSize,
> + GFP_KERNEL, &pegasus->data_dma);
> + if (!pegasus->data) {
> + error = -ENOMEM;
> + goto err_free_mem;
> + }
> +
> + pegasus->irq = usb_alloc_urb(0, GFP_KERNEL);
> + if (!pegasus->irq) {
> + error = -ENOMEM;
> + goto err_free_mem;

No, you need to jump to err_free_dma here.

> + }
> +
> + pipe = usb_rcvintpipe(dev, endpoint->bEndpointAddress);
> + maxp = usb_maxpacket(dev, pipe, usb_pipeout(pipe));
> + pegasus->data_len = maxp;
> +
> + usb_fill_int_urb(pegasus->irq, dev, pipe, pegasus->data, maxp,
> + pegasus_irq, pegasus, endpoint->bInterval);
> +
> + pegasus->irq->transfer_dma = pegasus->data_dma;
> + pegasus->irq->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> +
> + if (dev->manufacturer)
> + strlcpy(pegasus->name, dev->manufacturer,
> + sizeof(pegasus->name));
> +
> + if (dev->product) {
> + if (dev->manufacturer)
> + strlcat(pegasus->name, " ", sizeof(pegasus->name));
> + strlcat(pegasus->name, dev->product, sizeof(pegasus->name));
> + }
> +
> + if (!strlen(pegasus->name))
> + snprintf(pegasus->name, sizeof(pegasus->name),
> + "USB Pegasus Device %04x:%04x",
> + le16_to_cpu(dev->descriptor.idVendor),
> + le16_to_cpu(dev->descriptor.idProduct));
> +
> + usb_make_path(dev, pegasus->phys, sizeof(pegasus->phys));
> + strlcat(pegasus->phys, "/input0", sizeof(pegasus->phys));
> +
> + INIT_WORK(&pegasus->init, pegasus_init);
> +
> + usb_set_intfdata(intf, pegasus);
> +
> + input_dev->name = pegasus->name;
> + input_dev->phys = pegasus->phys;
> + usb_to_input_id(dev, &input_dev->id);
> + input_dev->dev.parent = &intf->dev;
> +
> + input_set_drvdata(input_dev, pegasus);
> +
> + input_dev->open = pegasus_open;
> + input_dev->close = pegasus_close;
> +
> + __set_bit(EV_ABS, input_dev->evbit);
> + __set_bit(EV_KEY, input_dev->evbit);
> +
> + __set_bit(ABS_X, input_dev->absbit);
> + __set_bit(ABS_Y, input_dev->absbit);
> +
> + __set_bit(BTN_TOUCH, input_dev->keybit);
> + __set_bit(BTN_RIGHT, input_dev->keybit);
> + __set_bit(BTN_TOOL_PEN, input_dev->keybit);
> +
> + __set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> + __set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> +
> + input_set_abs_params(input_dev, ABS_X, -1500, 1500, 8, 0);
> + input_set_abs_params(input_dev, ABS_Y, 1600, 3000, 8, 0);
> +
> + pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
> +
> + error = input_register_device(pegasus->dev);
> + if (error)
> + goto err_free_dma;

Should be "goto err_free_urb;" which should happen before err_free_dma.

> +
> + return 0;
> +
> +err_free_dma:
> + usb_free_coherent(dev, pegasus->data_len,
> + pegasus->data, pegasus->data_dma);
> +
> + usb_free_urb(pegasus->irq);
> +err_free_mem:
> + kfree(pegasus);

You are leaking input device here.

> + usb_set_intfdata(intf, NULL);
> +
> + return error;
> +}
> +
> +static void pegasus_disconnect(struct usb_interface *intf)
> +{
> + struct pegasus *pegasus = usb_get_intfdata(intf);
> +
> + input_unregister_device(pegasus->dev);
> +
> + usb_free_urb(pegasus->irq);
> + usb_free_coherent(interface_to_usbdev(intf),
> + pegasus->data_len, pegasus->data,
> + pegasus->data_dma);
> + kfree(pegasus);
> + usb_set_intfdata(intf, NULL);
> +}
> +
> +static const struct usb_device_id pegasus_ids[] = {
> + { USB_DEVICE(USB_VENDOR_ID_PEGASUSTECH,
> + USB_DEVICE_ID_PEGASUS_NOTETAKER_EN100) },
> + { }
> +};
> +MODULE_DEVICE_TABLE(usb, pegasus_ids);
> +
> +static struct usb_driver pegasus_driver = {
> + .name = "pegasus_notetaker",
> + .probe = pegasus_probe,
> + .disconnect = pegasus_disconnect,
> + .id_table = pegasus_ids,
> +};
> +
> +module_usb_driver(pegasus_driver);
> +
> +MODULE_AUTHOR("Martin Kepplinger <martink@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Pegasus Mobile Notetaker Pen tablet driver");
> +MODULE_LICENSE("GPL");
> --
> 2.1.4
>

Thanks.

--
Dmitry