| Message ID | 20240108091917.1552013-3-sandeep.cs@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | HID Support for Samsung driver | expand |
>On Mon, 2024-01-08 at 14:49 +0530, Sandeep C S wrote: >> Warning found by checkpatch.pl script. >[] >> diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c >[] >> @@ -67,20 +67,17 @@ static __u8 *samsung_irda_report_fixup(struct >hid_device *hdev, __u8 *rdesc, >> rdesc[178] = 0x08; >> rdesc[180] = 0x06; >> rdesc[182] = 0x42; >> - } else >> - if (*rsize == 203 && rdesc[192] == 0x15 && rdesc[193] == 0x0 && >> + } else if (*rsize == 203 && rdesc[192] == 0x15 && rdesc[193] == 0x0 >> +&& >> rdesc[194] == 0x25 && rdesc[195] == 0x12) { >> samsung_irda_dev_trace(hdev, 203); >> rdesc[193] = 0x1; >> rdesc[195] = 0xf; >> - } else >> - if (*rsize == 135 && rdesc[124] == 0x15 && rdesc[125] == 0x0 && >> + } else if (*rsize == 135 && rdesc[124] == 0x15 && rdesc[125] == 0x0 >> +&& >> rdesc[126] == 0x25 && rdesc[127] == 0x11) { >> samsung_irda_dev_trace(hdev, 135); >> rdesc[125] = 0x1; >> rdesc[127] = 0xe; >> - } else >> - if (*rsize == 171 && rdesc[160] == 0x15 && rdesc[161] == 0x0 && >> + } else if (*rsize == 171 && rdesc[160] == 0x15 && rdesc[161] == 0x0 >> +&& >> rdesc[162] == 0x25 && rdesc[163] == 0x01) { >> samsung_irda_dev_trace(hdev, 171); >> rdesc[161] = 0x1; > >For this block, I think a rewrite using memcmp would be clearer. >Something like: Okay . Thanks for your valuable feedback. We will promptly address your suggestions and enhance our code accordingly. And also please review other patch set as well. Thank you >--- > drivers/hid/hid-samsung.c | 34 +++++++++++++++------------------- > 1 file changed, 15 insertions(+), 19 deletions(-) > >diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c index >cf5992e970940..cd84fb5e68f69 100644 >--- a/drivers/hid/hid-samsung.c >+++ b/drivers/hid/hid-samsung.c >@@ -58,33 +58,29 @@ static inline void samsung_irda_dev_trace(struct >hid_device *hdev, static __u8 *samsung_irda_report_fixup(struct hid_device >*hdev, __u8 *rdesc, > unsigned int *rsize) > { >- if (*rsize == 184 && rdesc[175] == 0x25 && rdesc[176] == 0x40 && >- rdesc[177] == 0x75 && rdesc[178] == 0x30 && >- rdesc[179] == 0x95 && rdesc[180] == 0x01 && >- rdesc[182] == 0x40) { >+ if (*rsize == 184 && >+ !memcmp(&rdesc[175], "\x25\x40\x75\x30\x95\x01", 6) && >+ rdesc[182] == 0x40) { > samsung_irda_dev_trace(hdev, 184); > rdesc[176] = 0xff; > rdesc[178] = 0x08; > rdesc[180] = 0x06; > rdesc[182] = 0x42; >- } else >- if (*rsize == 203 && rdesc[192] == 0x15 && rdesc[193] == 0x0 && >- rdesc[194] == 0x25 && rdesc[195] == 0x12) { >+ } else if (*rsize == 203 && >+ !memcmp(&rdesc[192], "\x15\x00\x25\x12", 4)) { > samsung_irda_dev_trace(hdev, 203); >- rdesc[193] = 0x1; >- rdesc[195] = 0xf; >- } else >- if (*rsize == 135 && rdesc[124] == 0x15 && rdesc[125] == 0x0 && >- rdesc[126] == 0x25 && rdesc[127] == 0x11) { >+ rdesc[193] = 0x01; >+ rdesc[195] = 0x0f; >+ } else if (*rsize == 135 && >+ !memcmp(&rdesc[124], "\x15\x00\x25\x11", 4)) { > samsung_irda_dev_trace(hdev, 135); >- rdesc[125] = 0x1; >- rdesc[127] = 0xe; >- } else >- if (*rsize == 171 && rdesc[160] == 0x15 && rdesc[161] == 0x0 && >- rdesc[162] == 0x25 && rdesc[163] == 0x01) { >+ rdesc[125] = 0x01; >+ rdesc[127] = 0x0e; >+ } else if (*rsize == 171 && >+ !memcmp(&rdesc[160], "\x15\x00\x25\x01", 4)) { > samsung_irda_dev_trace(hdev, 171); >- rdesc[161] = 0x1; >- rdesc[163] = 0x3; >+ rdesc[161] = 0x01; >+ rdesc[163] = 0x03; > } > return rdesc; > }
On Mon, 2024-01-08 at 16:14 +0530, sandeep.cs wrote: > > On Mon, 2024-01-08 at 14:49 +0530, Sandeep C S wrote: Generally, it's better to refactor code that checkpatch bleats about than merely shutting up the warning. > > For this block, I think a rewrite using memcmp would be clearer. > > Something like: > Okay . Thanks for your valuable feedback. We will promptly address your > suggestions and enhance our code accordingly. > And also please review other patch set as well. Another way to write the input_mapping function is using a static const struct and a for loop like: static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev, struct hid_input *hi, struct hid_field *field, struct hid_usage *usage, unsigned long **bit, int *max) { struct usb_interface *intf = to_usb_interface(hdev->dev.parent); unsigned short ifnum = intf->cur_altsetting->desc.bInterfaceNumber; static const struct { unsigned hid; __u16 map; } samsung_hid_key_map[] = { {0x183, KEY_MEDIA}, {0x195, KEY_EMAIL}, {0x196, KEY_CALC}, {0x197, KEY_COMPUTER}, {0x22b, KEY_SEARCH}, {0x22c, KEY_WWW}, {0x22d, KEY_BACK}, {0x22e, KEY_FORWARD}, {0x22f, KEY_FAVORITES}, {0x230, KEY_REFRESH}, {0x231, KEY_STOP}, }; int i; unsigned hid; if (1 != ifnum || HID_UP_CONSUMER != (usage->hid & HID_USAGE_PAGE)) return 0; hid = usage->hid & HID_USAGE; dbg_hid("samsung wireless keyboard/mouse input mapping event [0x%x]\n", hid); for (i = 0; i < ARRAY_SIZE(samsung_hid_key_map); i++) { if (hid == samsung_hid_key_map[i].hid) { hid_map_usage_clear(hi, usage, bit, max, EV_KEY, samsung_hid_key_map[i].map); return 1; } } return 0; }
On Mon, 8 Jan 2024, sandeep.cs wrote: > >> - } else > >> - if (*rsize == 203 && rdesc[192] == 0x15 && rdesc[193] == 0x0 && > >> + } else if (*rsize == 203 && rdesc[192] == 0x15 && rdesc[193] == 0x0 > >> +&& > >> rdesc[194] == 0x25 && rdesc[195] == 0x12) { > >> samsung_irda_dev_trace(hdev, 203); > >> rdesc[193] = 0x1; > >> rdesc[195] = 0xf; > >> - } else > >> - if (*rsize == 135 && rdesc[124] == 0x15 && rdesc[125] == 0x0 && > >> + } else if (*rsize == 135 && rdesc[124] == 0x15 && rdesc[125] == 0x0 > >> +&& > >> rdesc[126] == 0x25 && rdesc[127] == 0x11) { > >> samsung_irda_dev_trace(hdev, 135); > >> rdesc[125] = 0x1; > >> rdesc[127] = 0xe; > >> - } else > >> - if (*rsize == 171 && rdesc[160] == 0x15 && rdesc[161] == 0x0 && > >> + } else if (*rsize == 171 && rdesc[160] == 0x15 && rdesc[161] == 0x0 > >> +&& > >> rdesc[162] == 0x25 && rdesc[163] == 0x01) { > >> samsung_irda_dev_trace(hdev, 171); > >> rdesc[161] = 0x1; > > > >For this block, I think a rewrite using memcmp would be clearer. > >Something like: > Okay . Thanks for your valuable feedback. We will promptly address your > suggestions and enhance our code accordingly. I agree with Joe's suggestion here; are you planning to send v2 of the series? The rest of the set looks good to me. Thanks,
>> >> - } else >> >> - if (*rsize == 203 && rdesc[192] == 0x15 && rdesc[193] == 0x0 && >> >> + } else if (*rsize == 203 && rdesc[192] == 0x15 && rdesc[193] == >> >> +0x0 && >> >> rdesc[194] == 0x25 && rdesc[195] == 0x12) { >> >> samsung_irda_dev_trace(hdev, 203); >> >> rdesc[193] = 0x1; >> >> rdesc[195] = 0xf; >> >> - } else >> >> - if (*rsize == 135 && rdesc[124] == 0x15 && rdesc[125] == 0x0 && >> >> + } else if (*rsize == 135 && rdesc[124] == 0x15 && rdesc[125] == >> >> +0x0 && >> >> rdesc[126] == 0x25 && rdesc[127] == 0x11) { >> >> samsung_irda_dev_trace(hdev, 135); >> >> rdesc[125] = 0x1; >> >> rdesc[127] = 0xe; >> >> - } else >> >> - if (*rsize == 171 && rdesc[160] == 0x15 && rdesc[161] == 0x0 && >> >> + } else if (*rsize == 171 && rdesc[160] == 0x15 && rdesc[161] == >> >> +0x0 && >> >> rdesc[162] == 0x25 && rdesc[163] == 0x01) { >> >> samsung_irda_dev_trace(hdev, 171); >> >> rdesc[161] = 0x1; >> > >> >For this block, I think a rewrite using memcmp would be clearer. >> >Something like: >> Okay . Thanks for your valuable feedback. We will promptly address >> your suggestions and enhance our code accordingly. > >I agree with Joe's suggestion here; are you planning to send v2 of the series? > >The rest of the set looks good to me. > >Thanks, > >-- >Jiri Kosina >SUSE Labs Hello Jiri , Yes, I am planning to send the v2 of the series. I will include review comments and share it with you soon. Thank & Regards Sandeep C S
diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c index 885657531607..33cc92337394 100644 --- a/drivers/hid/hid-samsung.c +++ b/drivers/hid/hid-samsung.c @@ -67,20 +67,17 @@ static __u8 *samsung_irda_report_fixup(struct hid_device *hdev, __u8 *rdesc, rdesc[178] = 0x08; rdesc[180] = 0x06; rdesc[182] = 0x42; - } else - if (*rsize == 203 && rdesc[192] == 0x15 && rdesc[193] == 0x0 && + } else if (*rsize == 203 && rdesc[192] == 0x15 && rdesc[193] == 0x0 && rdesc[194] == 0x25 && rdesc[195] == 0x12) { samsung_irda_dev_trace(hdev, 203); rdesc[193] = 0x1; rdesc[195] = 0xf; - } else - if (*rsize == 135 && rdesc[124] == 0x15 && rdesc[125] == 0x0 && + } else if (*rsize == 135 && rdesc[124] == 0x15 && rdesc[125] == 0x0 && rdesc[126] == 0x25 && rdesc[127] == 0x11) { samsung_irda_dev_trace(hdev, 135); rdesc[125] = 0x1; rdesc[127] = 0xe; - } else - if (*rsize == 171 && rdesc[160] == 0x15 && rdesc[161] == 0x0 && + } else if (*rsize == 171 && rdesc[160] == 0x15 && rdesc[161] == 0x0 && rdesc[162] == 0x25 && rdesc[163] == 0x01) { samsung_irda_dev_trace(hdev, 171); rdesc[161] = 0x1; @@ -99,7 +96,7 @@ static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev, struct usb_interface *intf = to_usb_interface(hdev->dev.parent); unsigned short ifnum = intf->cur_altsetting->desc.bInterfaceNumber; - if (1 != ifnum || HID_UP_CONSUMER != (usage->hid & HID_USAGE_PAGE)) + if (ifnum != 1 || HID_UP_CONSUMER != (usage->hid & HID_USAGE_PAGE)) return 0; dbg_hid("samsung wireless keyboard/mouse input mapping event [0x%x]\n", @@ -107,17 +104,39 @@ static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev, switch (usage->hid & HID_USAGE) { /* report 2 */ - case 0x183: samsung_kbd_mouse_map_key_clear(KEY_MEDIA); break; - case 0x195: samsung_kbd_mouse_map_key_clear(KEY_EMAIL); break; - case 0x196: samsung_kbd_mouse_map_key_clear(KEY_CALC); break; - case 0x197: samsung_kbd_mouse_map_key_clear(KEY_COMPUTER); break; - case 0x22b: samsung_kbd_mouse_map_key_clear(KEY_SEARCH); break; - case 0x22c: samsung_kbd_mouse_map_key_clear(KEY_WWW); break; - case 0x22d: samsung_kbd_mouse_map_key_clear(KEY_BACK); break; - case 0x22e: samsung_kbd_mouse_map_key_clear(KEY_FORWARD); break; - case 0x22f: samsung_kbd_mouse_map_key_clear(KEY_FAVORITES); break; - case 0x230: samsung_kbd_mouse_map_key_clear(KEY_REFRESH); break; - case 0x231: samsung_kbd_mouse_map_key_clear(KEY_STOP); break; + case 0x183: + samsung_kbd_mouse_map_key_clear(KEY_MEDIA); + break; + case 0x195: + samsung_kbd_mouse_map_key_clear(KEY_EMAIL); + break; + case 0x196: + samsung_kbd_mouse_map_key_clear(KEY_CALC); + break; + case 0x197: + samsung_kbd_mouse_map_key_clear(KEY_COMPUTER); + break; + case 0x22b: + samsung_kbd_mouse_map_key_clear(KEY_SEARCH); + break; + case 0x22c: + samsung_kbd_mouse_map_key_clear(KEY_WWW); + break; + case 0x22d: + samsung_kbd_mouse_map_key_clear(KEY_BACK); + break; + case 0x22e: + samsung_kbd_mouse_map_key_clear(KEY_FORWARD); + break; + case 0x22f: + samsung_kbd_mouse_map_key_clear(KEY_FAVORITES); + break; + case 0x230: + samsung_kbd_mouse_map_key_clear(KEY_REFRESH); + break; + case 0x231: + samsung_kbd_mouse_map_key_clear(KEY_STOP); + break; default: return 0; } @@ -128,7 +147,7 @@ static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev, static __u8 *samsung_report_fixup(struct hid_device *hdev, __u8 *rdesc, unsigned int *rsize) { - if (USB_DEVICE_ID_SAMSUNG_IR_REMOTE == hdev->product && hid_is_usb(hdev)) + if (hdev->product == USB_DEVICE_ID_SAMSUNG_IR_REMOTE && hid_is_usb(hdev)) rdesc = samsung_irda_report_fixup(hdev, rdesc, rsize); return rdesc; } @@ -139,7 +158,7 @@ static int samsung_input_mapping(struct hid_device *hdev, struct hid_input *hi, { int ret = 0; - if (USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD_MOUSE == hdev->product && hid_is_usb(hdev)) + if (hdev->product == USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD_MOUSE && hid_is_usb(hdev)) ret = samsung_kbd_mouse_input_mapping(hdev, hi, field, usage, bit, max); @@ -158,7 +177,7 @@ static int samsung_probe(struct hid_device *hdev, goto err_free; } - if (USB_DEVICE_ID_SAMSUNG_IR_REMOTE == hdev->product) { + if (hdev->product == USB_DEVICE_ID_SAMSUNG_IR_REMOTE) { if (!hid_is_usb(hdev)) { ret = -EINVAL; goto err_free;