Hi Daniel,
thanks for the patch. Please find a few comments below.
On Thu, 12 Jul 2018, Daniel M. Lambea wrote:
> --- /dev/null
> +++ b/drivers/hid/hid-cougar.c
> @@ -0,0 +1,343 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * HID driver for Cougar 500k Gaming Keyboard
> + *
> + * Copyright (c) 2018 Daniel M. Lambea <dmlambea@xxxxxxxxx>
> + *
> + * ChangeLog:
> + * v0.6 (dml) - First submit to kernel.org
> + * v0.7 (dml) - Deep refactor
> + * - No usage of usb.h
> + * - Shared memory now properly managed using krefs.
> + * - Siblings now properly searched for
We usually don't put these kinds of things into headers / comments.
> +/*
> + * From wacom_sys.c
> + */
This doesn't really explain what the function does.
> +static bool compare_device_paths(struct hid_device *hdev_a,
> + struct hid_device *hdev_b, char separator)
> +{
> + int n1 = strrchr(hdev_a->phys, separator) - hdev_a->phys;
> + int n2 = strrchr(hdev_b->phys, separator) - hdev_b->phys;
> +
> + if (n1 != n2 || n1 <= 0 || n2 <= 0)
> + return false;
> +
> + return !strncmp(hdev_a->phys, hdev_b->phys, n1);
> +}
If this is a duplicated code copied from a different driver, perhaps it
should be abstracted to some common place?
> +
> +/*
> + * Derived from wacom_sys.c
> + */
Does this provide any useful information to the person reading the code?
(multiple locations).
Other than that it looks good, could you please submit v2 with
these nits fixed? Thanks.
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
