On Tue, Dec 16, 2014 at 10:55 AM, Peter Wu <peter@xxxxxxxxxxxxx> wrote:
> Previously wtp_raw_event would be called through
> hidpp_raw_hidpp_event (for the touchpad report) and hidpp_raw_event
> (for the mouse report).
>
> This patch removes one calling surface, making a clearer distinction
> between "generic HID++ processing" (matching internal reports) and
> device-specific event processing.
>
> Suggested-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> Signed-off-by: Peter Wu <peter@xxxxxxxxxxxxx>
> ---
> v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
> v2: splitted original report length check patch. Restructured code.
> ---
The patch is good per-se (and I tested the whole series BTW).
I just have a minor cosmetic comment.
If you feel like sending a v3, go ahead, otherwise, this patch is
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> drivers/hid/hid-logitech-hidpp.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 2f1b0ac..3dcd59c 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -942,7 +942,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
>
> /*
> * If the mutex is locked then we have a pending answer from a
> - * previoulsly sent command
> + * previously sent command.
> */
> if (unlikely(mutex_is_locked(&hidpp->send_mutex))) {
> /*
> @@ -973,9 +973,6 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
> return 1;
> }
>
> - if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> - return wtp_raw_event(hidpp->hid_dev, data, size);
> -
> return 0;
> }
>
> @@ -983,7 +980,9 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
> u8 *data, int size)
> {
> struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> + int r = 0;
I'd rather have this variable named "ret".
2 reasons: that's how it is across the file, and I dislike variable
names with only one letter (except i, j, k, n, where this is obvious).
Cheers,
Benjamin
>
> + /* Generic HID++ processing. */
> switch (data[0]) {
> case REPORT_ID_HIDPP_LONG:
> if (size != HIDPP_REPORT_LONG_LENGTH) {
> @@ -991,16 +990,23 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
> size);
> return 1;
> }
> - return hidpp_raw_hidpp_event(hidpp, data, size);
> + r = hidpp_raw_hidpp_event(hidpp, data, size);
> + break;
> case REPORT_ID_HIDPP_SHORT:
> if (size != HIDPP_REPORT_SHORT_LENGTH) {
> hid_err(hdev, "received hid++ report of bad size (%d)",
> size);
> return 1;
> }
> - return hidpp_raw_hidpp_event(hidpp, data, size);
> + r = hidpp_raw_hidpp_event(hidpp, data, size);
> + break;
> }
>
> + /* If no report is available for further processing, skip calling
> + * raw_event of subclasses. */
> + if (r != 0)
> + return r;
> +
> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> return wtp_raw_event(hdev, data, size);
>
> --
> 2.1.3
>
> --
> 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
--
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
