Warning found by checkpatch.pl script.
Signed-off-by: Sandeep C S <[email protected]>
Signed-off-by: Junwan Cho <[email protected]>
Signed-off-by: Jitender Sajwan <[email protected]>
---
drivers/hid/hid-samsung.c | 61 +++++++++++++++++++++++++--------------
1 file changed, 40 insertions(+), 21 deletions(-)
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;
--
2.34.1
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:
---
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 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,
--
Jiri Kosina
SUSE Labs
>> >> - } 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