Re: [PATCH 1/1] HID: Add full support for logitech Unifying receivers

Hi everyone, please find my comments Jiri's remarks below:

Thanks,
Nestor

On Fri, Jun 24, 2011 at 2:27 PM, Jiri Kosina [off-list ref] wrote:

On Fri, 13 May 2011, Nestor Lopez Casado wrote:

quoted

Up to now, when Logitech Unifying receivers are connected to a Linux
based system, a single keyboard and a single mouse are presented to the
HID Layer, even if the Unifying receiver can pair up to six compatible
devices. The Unifying receiver by default multiplexes all incoming
events (from multiple keyboards/mice) into these two.

Sorry that it took me too long to review the driver.

Generally, I like it. The question I have -- apparently the receiver works
in a crippled mode even without the driver. Therefore, I'd suggest to put
the hid_have_special_driver[] uncodinitionally, the same way we do for
other devices.

The receiver does work in a simpler "crippled" mode where all input
from all paired devices are multiplexed through a single keyboard
interface and a single mouse interface. All Linux based systems
already support Unifying devices via the standard hid driver. This
will suffice for most users. The added functionality that this driver
provides goes to users that would like to differentiate input coming
from different input devices paired to the same Unifying receiver.
(Some use cases: Gaming on the same PC with two input devices, more
than one keyboard w/ different layouts, applying different ballistics
to different cursor control devices, etc)

I understand the inappropriateness of the conditionals on the specific
hid driver table, but removing them would mean that users that have
not selected the Unifying driver on their kernel configuration would
lose an otherwise perfectly functioning set of devices which are meant
to work out of the box without any special driver. For this reason I
suggest that we leave the conditionals.

Apart from the, the driver looks good to me. Please find a few rather
minor comments below.

quoted

+config HID_LOGITECH_DJ
+     tristate "Logitech Unifying receivers full support"
+     depends on USB_HID

We already have sub-menu for Logitech devices. I think this driver should
be in that sub-menu, even if it doesn't directly depend on hid-logitech.

That's OK. It is already corrected for the next patch.

quoted

--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile

@@ -44,6 +44,7 @@ obj-$(CONFIG_HID_KEYTOUCH)  += hid-keytouch.o

 obj-$(CONFIG_HID_KYE)                += hid-kye.o
 obj-$(CONFIG_HID_LCPOWER)       += hid-lcpower.o
 obj-$(CONFIG_HID_LOGITECH)   += hid-logitech.o
+obj-$(CONFIG_HID_LOGITECH_DJ)        += hid-logitech-dj.o
 obj-$(CONFIG_HID_MAGICMOUSE)    += hid-magicmouse.o
 obj-$(CONFIG_HID_MICROSOFT)  += hid-microsoft.o
 obj-$(CONFIG_HID_MONTEREY)   += hid-monterey.o

@@ -78,6 +79,7 @@ obj-$(CONFIG_HID_ZYDACRON)  += hid-zydacron.o

 obj-$(CONFIG_HID_WACOM)              += hid-wacom.o
 obj-$(CONFIG_HID_WALTOP)     += hid-waltop.o

+
 obj-$(CONFIG_USB_HID)                += usbhid/
 obj-$(CONFIG_USB_MOUSE)              += usbhid/
 obj-$(CONFIG_USB_KBD)                += usbhid/

Why the added new line?

Did not see it. It is already corrected for the next patch.

quoted

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 408c4be..2855403 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c

@@ -1253,6 +1253,9 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)

      case BUS_BLUETOOTH:
              bus = "BLUETOOTH";
              break;
+     case BUS_DJ:
+             bus = "UNIFYING";
+             break;

Why do we need separate bus? It's always USB, right?

The reason we need a new bus type is because we are creating a set of
new hid devices which are not physically connected to the USB. And
they don't have a USB VID/PID. They only have a Unifying (a.k.a. DJ)
product ID known as Unifying device ID.

I am afraid that mixing things up now (even if it may work for the
moment) can create some undesired couplings in the future. See that
hid-logitech-dj registers a generic hid driver for Unifying devices.
What if at one point we need to have specific DJ drivers por specific
DJ devices that do not fit in the current HID implementation/standard?

On top of that I don't see harm in leaving the extra bus type.

quoted

      default:
              bus = "<UNKNOWN>";
      }

@@ -1485,8 +1488,11 @@ static const struct hid_device_id hid_have_special_driver[] = {

      { HID_USB_DEVICE(USB_VENDOR_ID_ZEROPLUS, 0x0005) },
      { HID_USB_DEVICE(USB_VENDOR_ID_ZEROPLUS, 0x0030) },
      { HID_USB_DEVICE(USB_VENDOR_ID_ZYDACRON, USB_DEVICE_ID_ZYDACRON_REMOTE_CONTROL) },
-
      { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PRESENTER_8K_BT) },
+#if defined(CONFIG_HID_LOGITECH_DJ) || defined(CONFIG_HID_LOGITECH_DJ_MODULE)
+     { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xc52b) },
+     { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xc532) },
+#endif

See above.

quoted

+static void logi_dj_recv_destroy_djhid_device(struct dj_receiver_dev *djrcv_dev,
+                                             struct dj_report *dj_report)
+{
+     /* Called in delayed work context */
+     struct dj_device *dj_dev;
+     unsigned long flags;
+
+     spin_lock_irqsave(&djrcv_dev->lock, flags);
+     dj_dev = djrcv_dev->paired_dj_devices[dj_report->device_index];
+     djrcv_dev->paired_dj_devices[dj_report->device_index] = NULL;
+     spin_unlock_irqrestore(&djrcv_dev->lock, flags);
+
+     if (dj_dev != NULL) {
+             hid_destroy_device(dj_dev->hdev);
+             kfree(dj_dev);
+     } else {
+             dev_err(&djrcv_dev->hdev->dev, "logi_dj_recv_destroy_djhid_device"
+                                     ":cant destroy a NULL device\n");

The error message looks odd (cant is spelled wrong way, and directly
attached to ':').

Did not see it. It is already corrected for the next patch.

quoted

--- /dev/null
+++ b/drivers/hid/hid-logitech-dj.h

@@ -0,0 +1,117 @@
+#ifndef __HID_LOGITECH_DJ_H
+#define __HID_LOGITECH_DJ_H
+
+/*
+ *  HID driver for Logitech Unifying receivers
+ *
+ *  Copyright (c) 2011 Logitech (c)
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.

BTW, are you sure about the "any later version" part?

Thanks,

Very good point. It is already corrected for the next patch.

--
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html