Hi Martin, On Fri, May 27, 2016 at 11:46:21AM +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 reviewing! Dmitry's and Oliver's changes to v4 made it even > smaller now. All is tested again and should apply to any recent tree. > If not, please just complain. Couple of minor comments: > + > +static void pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len) > +{ > + const int sizeof_buf = len * sizeof(u8) + 2; Just do "len + 2". > +static void pegasus_parse_packet(struct pegasus *pegasus) > +{ > + unsigned char *data = pegasus->data; > + struct input_dev *dev = pegasus->dev; > + u16 x, y; > + > + 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: > + x = le16_to_cpup((__le16 *)&data[2]); > + y = le16_to_cpup((__le16 *)&data[4]); > + > + /* ignore pen up events */ > + if (x == 0 && y == 0) Why are we ignoring pen-up events? I'd at least send EV_KEY/BTN_TOOL_PEN/0 and maybe the rest of BTN* events. > + break; > + > + input_report_key(dev, BTN_TOUCH, data[1] & PEN_TIP); > + input_report_key(dev, BTN_RIGHT, data[1] & PEN_BUTTON_PRESSED); > + input_report_key(dev, BTN_TOOL_PEN, 1); > + input_report_abs(dev, ABS_X, (s16)x); > + input_report_abs(dev, ABS_Y, 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); Since the driver does not use runtime-PM I do not think you need to call usb_mark_last_busy(). > +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; Please add: /* Sanity check that the device has an endpoint */ if (usbinterface->altsetting[0].desc.bNumEndpoints < 1) { dev_err(&usbinterface->dev, "Invalid number of endpoints\n"); return -EINVAL; } Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe kernel-testers" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
