On 24/08/16 13:02, Rafał Miłecki wrote:
On 24 August 2016 at 12:49, Matthias Brugger <mbrugger@xxxxxxxx> wrote:
On 24/08/16 00:03, Rafał Miłecki wrote:
[...]
+static int usbport_trig_notify(struct notifier_block *nb, unsigned long
action,
+ void *data)
+{
+ struct usbport_trig_data *usbport_data =
+ container_of(nb, struct usbport_trig_data, nb);
+ struct led_classdev *led_cdev = usbport_data->led_cdev;
+
+ switch (action) {
+ case USB_DEVICE_ADD:
+ if (usbport_trig_usb_dev_observed(usbport_data, data)) {
Maybe we should switch this and fist see if the usbport is observed before
evaluating the action. Also cast data to "struct usb_device *" to make that
clear.
I'm aware there is one duplicated line of code, I did to first
evaluate very quick test (checking unsigned long value), then iterate
over ports & keep only 1 switch block. I could move
usbport_trig_usb_dev_observed call up, but it would be executed for
other actions as well (currently just USB_BUS_ADD and USB_BUS_REMOVE).
Ok. I'm a USB noop but from my understanding the notifier is only called
when a device or a hub gets added/removed. So this shouldn't happen that
much. Therefor it has no impact if we check if the usb device is in the
observer list for all actions.
+ if (usbport_data->count++ == 0)
I'm a bit puzzled. I think:
if (++usbport_data->count > 0)
makes this more consistent with the remove case.
Your condition would be always true (as we don't use negative
numbers). The point is to enable LED only if it was disabled before.
So I need to increase counter unconditionally but enable LED only if
initial value (before increasing it) was 0.
Got it. My personal opinion is, that adding one line for
incrementing/decrementing the counter would help to make this
crystal-clear to everyone (at least to me :)
Cheers,
Matthias
+module_init(usbport_trig_init);
+module_exit(usbport_trig_exit);
+
+MODULE_AUTHOR("Rafał Miłecki <rafal@milecki@pl>");
Nit: rafal@xxxxxxxxxx
Oops, thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
