Re: [PATCH] input: Synaptics USB device driver

[Oliver Neukum]

Am Mittwoch, 4. Januar 2012, 10:41:03 schrieb Dmitry Torokhov:


> +static void synusb_irq(struct urb *urb)
> +{
> +	struct synusb *synusb = urb->context;
> +	int error;
> +
> +	/* Check our status in case we need to bail out early. */
> +	switch (urb->status) {
> +	case 0:
> +		usb_mark_last_busy(synusb->udev);
> +		break;
> +
> +	case -EOVERFLOW:
> +		dev_err(&synusb->intf->dev,
> +			"%s: OVERFLOW with data length %d, actual length is %d\n",
> +			__func__, SYNUSB_RECV_SIZE, urb->actual_length);

Do you really want to fall through here?

> +
> +	/* Device went away so don't keep trying to read from it. */
> +	case -ECONNRESET:
> +	case -ENOENT:
> +	case -ESHUTDOWN:
> +		return;
> +
> +	default:
> +		goto resubmit;
> +		break;
> +	}
> +
> +	if (synusb->flags & SYNUSB_STICK)
> +		synusb_report_stick(synusb);
> +	else
> +		synusb_report_touchpad(synusb);
> +
> +resubmit:
> +	error = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (error && error != -EPERM)
> +		dev_err(&synusb->intf->dev,
> +			"%s - usb_submit_urb failed with result: %d",
> +			__func__, error);
> +}
> +

[..]
> +static int synusb_open(struct input_dev *dev)
> +{
> +	struct synusb *synusb = input_get_drvdata(dev);
> +	int retval = 0;
> +
> +	dev_err(&synusb->intf->dev, "%s\n", __func__);
> +
> +	if (usb_autopm_get_interface(synusb->intf) < 0)
> +		return -EIO;
> +
> +	if (usb_submit_urb(synusb->urb, GFP_KERNEL)) {
> +		dev_err(&synusb->intf->dev,
> +			"%s - usb_submit_urb failed", __func__);
> +		retval = -EIO;
> +		goto out;
> +	}
> +
> +	synusb->intf->needs_remote_wakeup = 1;
> +
> +out:
> +	usb_autopm_put_interface(synusb->intf);
> +	dev_err(&synusb->intf->dev, "%s done: %d\n", __func__, retval);
> +	return retval;
> +}
> +
> +static void synusb_close(struct input_dev *dev)
> +{
> +	struct synusb *synusb = input_get_drvdata(dev);
> +	int autopm_error;
> +
> +	autopm_error = usb_autopm_get_interface(synusb->intf);
> +
> +	usb_kill_urb(synusb->urb);
> +	synusb->intf->needs_remote_wakeup = 0;
> +
> +	if (!autopm_error)
> +		usb_autopm_put_interface(synusb->intf);
> +}
> +
> +static int synusb_probe(struct usb_interface *intf,
> +			const struct usb_device_id *id)
> +{
> +	struct usb_device *udev = interface_to_usbdev(intf);
> +	struct usb_endpoint_descriptor *ep;
> +	struct synusb *synusb;
> +	struct input_dev *input_dev;
> +	unsigned int intf_num = intf->cur_altsetting->desc.bInterfaceNumber;
> +	unsigned int altsetting = min(intf->num_altsetting, 1U);
> +	int error;
> +
> +	error = usb_set_interface(udev, intf_num, altsetting);
> +	if (error) {
> +		dev_err(&udev->dev,
> +			"Can not set alternate setting to %i, error: %i",
> +			altsetting, error);
> +		return error;
> +	}
> +
> +	ep = synusb_get_in_endpoint(intf->cur_altsetting);
> +	if (!ep)
> +		return -ENODEV;
> +
> +	synusb = kzalloc(sizeof(*synusb), GFP_KERNEL);
> +	input_dev = input_allocate_device();
> +	if (!synusb || !input_dev) {
> +		error = -ENOMEM;
> +		goto err_free_mem;
> +	}
> +
> +	synusb->udev = udev;
> +	synusb->intf = intf;
> +	synusb->input = input_dev;
> +
> +	synusb->flags = id->driver_info;
> +	if (test_bit(SYNUSB_STICK, &synusb->flags) &&
> +	    test_bit(SYNUSB_STICK, &synusb->flags)) {

??? Voodoo ???

> +		/*
> +		 * This is a combo device, we need to leave only proper
> +		 * capability, depending on the interface.
> +		 */
> +		clear_bit(intf_num == 1 ? SYNUSB_TOUCHPAD : SYNUSB_STICK,
> +			  &synusb->flags);
> +	}
> +
> +	synusb->urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!synusb->urb) {
> +		error = -ENOMEM;
> +		goto err_free_mem;
> +	}
> +
> +	synusb->data = usb_alloc_coherent(udev, SYNUSB_RECV_SIZE, GFP_KERNEL,
> +					  &synusb->urb->transfer_dma);
> +	if (!synusb->data) {
> +		error = -ENOMEM;
> +		goto err_free_urb;
> +	}
> +
> +	usb_fill_int_urb(synusb->urb, udev,
> +			 usb_rcvintpipe(udev, ep->bEndpointAddress),
> +			 synusb->data, SYNUSB_RECV_SIZE,
> +			 synusb_irq, synusb,
> +			 ep->bInterval);
> +	synusb->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;

According to the comment in the original driver you must submit the URB.
Are you sure not doing so is save?

> +
> +	if (udev->manufacturer)
> +		strlcpy(synusb->name, udev->manufacturer,
> +			sizeof(synusb->name));
> +
> +	if (udev->product) {
> +		if (udev->manufacturer)
> +			strlcat(synusb->name, " ", sizeof(synusb->name));
> +		strlcat(synusb->name, udev->product, sizeof(synusb->name));
> +	}
> +
> +	if (!strlen(synusb->name))
> +		snprintf(synusb->name, sizeof(synusb->name),
> +			 "USB Synaptics Device %04x:%04x",
> +			 le16_to_cpu(udev->descriptor.idVendor),
> +			 le16_to_cpu(udev->descriptor.idProduct));
> +
> +	if (synusb->flags & SYNUSB_STICK)
> +		strlcat(synusb->name, " (Stick) ", sizeof(synusb->name));
> +
> +	usb_make_path(udev, synusb->phys, sizeof(synusb->phys));
> +	strlcat(synusb->phys, "/input0", sizeof(synusb->phys));
> +
> +	input_dev->name = synusb->name; // synusb_get_name(synusb);
> +	input_dev->phys = synusb->phys;
> +	usb_to_input_id(udev, &input_dev->id);
> +	input_dev->dev.parent = &synusb->intf->dev;
> +
> +	input_dev->open = synusb_open;
> +	input_dev->close = synusb_close;
> +
> +	input_set_drvdata(input_dev, synusb);
> +
> +	__set_bit(EV_ABS, input_dev->evbit);
> +	__set_bit(EV_KEY, input_dev->evbit);
> +
> +	if (synusb->flags & SYNUSB_STICK) {
> +		__set_bit(EV_REL, input_dev->evbit);
> +		__set_bit(REL_X, input_dev->relbit);
> +		__set_bit(REL_Y, input_dev->relbit);
> +		input_set_abs_params(input_dev, ABS_PRESSURE, 0, 127, 0, 0);
> +	} else {
> +		input_set_abs_params(input_dev, ABS_X, xmin, xmax, 0, 0);
> +		input_set_abs_params(input_dev, ABS_Y, ymin, ymax, 0, 0);
> +		input_set_abs_params(input_dev, ABS_PRESSURE, 0, 255, 0, 0);
> +		input_set_abs_params(input_dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
> +		__set_bit(BTN_TOUCH, input_dev->keybit);
> +		__set_bit(BTN_TOOL_FINGER, input_dev->keybit);
> +		__set_bit(BTN_TOOL_DOUBLETAP, input_dev->keybit);
> +		__set_bit(BTN_TOOL_TRIPLETAP, input_dev->keybit);
> +	}
> +
> +	__set_bit(BTN_LEFT, input_dev->keybit);
> +	__set_bit(BTN_RIGHT, input_dev->keybit);
> +	__set_bit(BTN_MIDDLE, input_dev->keybit);
> +	if (synusb->flags & SYNUSB_AUXDISPLAY)
> +		__set_bit(BTN_MISC, input_dev->keybit);
> +
> +	error = input_register_device(input_dev);
> +	if (error) {
> +		dev_err(&udev->dev, "Failed to register input device, error %d\n",
> +			error);
> +		goto err_free_dma;
> +	}
> +
> +	usb_set_intfdata(intf, synusb);
> +	return 0;
> +
> +err_free_dma:
> +	usb_free_coherent(udev, SYNUSB_RECV_SIZE, synusb->data,
> +			  synusb->urb->transfer_dma);
> +err_free_urb:
> +	usb_free_urb(synusb->urb);
> +err_free_mem:
> +	input_free_device(input_dev);
> +	kfree(synusb);
> +
> +	return error;
> +}
> +

[..]
> +static struct usb_driver synusb_driver = {
> +	.name		= "synaptics_usb",
> +	.probe		= synusb_probe,
> +	.disconnect	= synusb_disconnect,
> +	.id_table	= synusb_idtable,
> +	.suspend	= synusb_suspend,
> +	.resume		= synusb_resume,
> +	.reset_resume	= synusb_reset_resume,

You've killed the support for reset. That is not cool.

> +	.supports_autosuspend = 1,
> +};

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/