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

[Dmitry Torokhov]

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