linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hid: usbhid: usbkbd: fix checkpatch.pl issues
@ 2017-02-21 17:26 Avraham Shukron
  2017-03-01 18:16 ` Dmitry Torokhov
  0 siblings, 1 reply; 8+ messages in thread
From: Avraham Shukron @ 2017-02-21 17:26 UTC (permalink / raw)
  To: jikos, benjamin.tissoires; +Cc: linux-usb, linux-input, linux-kernel

- Broke long lines
- Added spaces where needed
- Removed unnecessary / trailing whitespaces
- Extracted assignments outside of 'if' statements

Signed-off-by: Avraham Shukron <avraham.shukron@gmail.com>
---
 drivers/hid/usbhid/usbkbd.c | 121 ++++++++++++++++++++++++++------------------
 1 file changed, 72 insertions(+), 49 deletions(-)

diff --git a/drivers/hid/usbhid/usbkbd.c b/drivers/hid/usbhid/usbkbd.c
index 7fb2d1e..ae40b0d 100644
--- a/drivers/hid/usbhid/usbkbd.c
+++ b/drivers/hid/usbhid/usbkbd.c
@@ -45,22 +45,24 @@ MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("GPL");

 static const unsigned char usb_kbd_keycode[256] = {
-	  0,  0,  0,  0, 30, 48, 46, 32, 18, 33, 34, 35, 23, 36, 37, 38,
-	 50, 49, 24, 25, 16, 19, 31, 20, 22, 47, 17, 45, 21, 44,  2,  3,
-	  4,  5,  6,  7,  8,  9, 10, 11, 28,  1, 14, 15, 57, 12, 13, 26,
-	 27, 43, 43, 39, 40, 41, 51, 52, 53, 58, 59, 60, 61, 62, 63, 64,
-	 65, 66, 67, 68, 87, 88, 99, 70,119,110,102,104,111,107,109,106,
-	105,108,103, 69, 98, 55, 74, 78, 96, 79, 80, 81, 75, 76, 77, 71,
-	 72, 73, 82, 83, 86,127,116,117,183,184,185,186,187,188,189,190,
-	191,192,193,194,134,138,130,132,128,129,131,137,133,135,136,113,
-	115,114,  0,  0,  0,121,  0, 89, 93,124, 92, 94, 95,  0,  0,  0,
-	122,123, 90, 91, 85,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
-	  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
-	  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
-	  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
-	  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
-	 29, 42, 56,125, 97, 54,100,126,164,166,165,163,161,115,114,113,
-	150,158,159,128,136,177,178,176,142,152,173,140
+	  0,   0,   0,   0,  30,  48,  46,  32,  18,  33,  34,  35,  23,  36,
+	 37,  38,  50,  49,  24,  25,  16,  19,  31,  20,  22,  47,  17,  45,
+	 21,  44,   2,   3,   4,   5,   6,   7,   8,   9,  10,  11,  28,   1,
+	 14,  15,  57,  12,  13,  26,  27,  43,  43,  39,  40,  41,  51,  52,
+	 53,  58,  59,  60,  61,  62,  63,  64,  65,  66,  67,  68,  87,  88,
+	 99,  70, 119, 110, 102, 104, 111, 107, 109, 106, 105, 108, 103,  69,
+	 98,  55,  74,  78,  96,  79,  80,  81,  75,  76,  77,  71,  72,  73,
+	 82,  83,  86, 127, 116, 117, 183, 184, 185, 186, 187, 188, 189, 190,
+	191, 192, 193, 194, 134, 138, 130, 132, 128, 129, 131, 137, 133, 135,
+	136, 113, 115, 114,   0,   0,   0, 121,   0,  89,  93, 124,  92,  94,
+	 95,   0,   0,   0, 122, 123,  90,  91,  85,   0,   0,   0,   0,   0,
+	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
+	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
+	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
+	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
+	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
+	 29,  42,  56, 125,  97,  54, 100, 126, 164, 166, 165, 163, 161, 115,
+	114, 113, 150, 158, 159, 128, 136, 177, 178, 176, 142, 152, 173, 140
 };


@@ -75,7 +77,7 @@ static const unsigned char usb_kbd_keycode[256] = {
  *		new key is pressed or a key that was pressed is released.
  * @led:	URB for sending LEDs (e.g. numlock, ...)
  * @newleds:	data that will be sent with the @led URB representing which LEDs
- 		should be on
+ *		should be on
  * @name:	Name of the keyboard. @dev's name field points to this buffer
  * @phys:	Physical path of the keyboard. @dev's phys field points to this
  *		buffer
@@ -103,7 +105,7 @@ struct usb_kbd {
 	unsigned char *leds;
 	dma_addr_t new_dma;
 	dma_addr_t leds_dma;
-	
+
 	spinlock_t leds_lock;
 	bool led_urb_submitted;

@@ -127,22 +129,30 @@ static void usb_kbd_irq(struct urb *urb)
 	}

 	for (i = 0; i < 8; i++)
-		input_report_key(kbd->dev, usb_kbd_keycode[i + 224], (kbd->new[0] >> i) & 1);
+		input_report_key(kbd->dev,
+			usb_kbd_keycode[i + 224],
+			(kbd->new[0] >> i) & 1);

 	for (i = 2; i < 8; i++) {

-		if (kbd->old[i] > 3 && memscan(kbd->new + 2, kbd->old[i], 6) == kbd->new + 8) {
+		if (kbd->old[i] > 3 &&
+		    memscan(kbd->new + 2, kbd->old[i], 6) == kbd->new + 8) {
 			if (usb_kbd_keycode[kbd->old[i]])
-				input_report_key(kbd->dev, usb_kbd_keycode[kbd->old[i]], 0);
+				input_report_key(
+					kbd->dev,
+					usb_kbd_keycode[kbd->old[i]], 0
+				);
 			else
 				hid_info(urb->dev,
 					 "Unknown key (scancode %#x) released.\n",
 					 kbd->old[i]);
 		}

-		if (kbd->new[i] > 3 && memscan(kbd->old + 2, kbd->new[i], 6) == kbd->old + 8) {
+		if (kbd->new[i] > 3 &&
+		    memscan(kbd->old + 2, kbd->new[i], 6) == kbd->old + 8) {
 			if (usb_kbd_keycode[kbd->new[i]])
-				input_report_key(kbd->dev, usb_kbd_keycode[kbd->new[i]], 1);
+				input_report_key(kbd->dev,
+					usb_kbd_keycode[kbd->new[i]], 1);
 			else
 				hid_info(urb->dev,
 					 "Unknown key (scancode %#x) pressed.\n",
@@ -155,9 +165,10 @@ static void usb_kbd_irq(struct urb *urb)
 	memcpy(kbd->old, kbd->new, 8);

 resubmit:
-	i = usb_submit_urb (urb, GFP_ATOMIC);
+	i = usb_submit_urb(urb, GFP_ATOMIC);
 	if (i)
-		hid_err(urb->dev, "can't resubmit intr, %s-%s/input0, status %d",
+		hid_err(urb->dev,
+			"can't resubmit intr, %s-%s/input0, status %d",
 			kbd->usbdev->bus->bus_name,
 			kbd->usbdev->devpath, i);
 }
@@ -172,30 +183,32 @@ static int usb_kbd_event(struct input_dev *dev, unsigned int type,
 		return -1;

 	spin_lock_irqsave(&kbd->leds_lock, flags);
-	kbd->newleds = (!!test_bit(LED_KANA,    dev->led) << 3) | (!!test_bit(LED_COMPOSE, dev->led) << 3) |
-		       (!!test_bit(LED_SCROLLL, dev->led) << 2) | (!!test_bit(LED_CAPSL,   dev->led) << 1) |
-		       (!!test_bit(LED_NUML,    dev->led));
+	kbd->newleds =	(!!test_bit(LED_KANA,    dev->led) << 3) |
+			(!!test_bit(LED_COMPOSE, dev->led) << 3) |
+			(!!test_bit(LED_SCROLLL, dev->led) << 2) |
+			(!!test_bit(LED_CAPSL,   dev->led) << 1) |
+			(!!test_bit(LED_NUML,    dev->led));

-	if (kbd->led_urb_submitted){
+	if (kbd->led_urb_submitted) {
 		spin_unlock_irqrestore(&kbd->leds_lock, flags);
 		return 0;
 	}

-	if (*(kbd->leds) == kbd->newleds){
+	if (*(kbd->leds) == kbd->newleds) {
 		spin_unlock_irqrestore(&kbd->leds_lock, flags);
 		return 0;
 	}

 	*(kbd->leds) = kbd->newleds;
-	
+
 	kbd->led->dev = kbd->usbdev;
 	if (usb_submit_urb(kbd->led, GFP_ATOMIC))
 		pr_err("usb_submit_urb(leds) failed\n");
 	else
 		kbd->led_urb_submitted = true;
-	
+
 	spin_unlock_irqrestore(&kbd->leds_lock, flags);
-	
+
 	return 0;
 }

@@ -210,21 +223,21 @@ static void usb_kbd_led(struct urb *urb)

 	spin_lock_irqsave(&kbd->leds_lock, flags);

-	if (*(kbd->leds) == kbd->newleds){
+	if (*(kbd->leds) == kbd->newleds) {
 		kbd->led_urb_submitted = false;
 		spin_unlock_irqrestore(&kbd->leds_lock, flags);
 		return;
 	}

 	*(kbd->leds) = kbd->newleds;
-	
+
 	kbd->led->dev = kbd->usbdev;
-	if (usb_submit_urb(kbd->led, GFP_ATOMIC)){
+	if (usb_submit_urb(kbd->led, GFP_ATOMIC)) {
 		hid_err(urb->dev, "usb_submit_urb(leds) failed\n");
 		kbd->led_urb_submitted = false;
 	}
 	spin_unlock_irqrestore(&kbd->leds_lock, flags);
-	
+
 }

 static int usb_kbd_open(struct input_dev *dev)
@@ -247,15 +260,24 @@ static void usb_kbd_close(struct input_dev *dev)

 static int usb_kbd_alloc_mem(struct usb_device *dev, struct usb_kbd *kbd)
 {
-	if (!(kbd->irq = usb_alloc_urb(0, GFP_KERNEL)))
+	kbd->irq = usb_alloc_urb(0, GFP_KERNEL);
+	if (!(kbd->irq))
 		return -1;
-	if (!(kbd->led = usb_alloc_urb(0, GFP_KERNEL)))
+
+	kbd->led = usb_alloc_urb(0, GFP_KERNEL);
+	if (!(kbd->led))
 		return -1;
-	if (!(kbd->new = usb_alloc_coherent(dev, 8, GFP_ATOMIC, &kbd->new_dma)))
+
+	kbd->new = usb_alloc_coherent(dev, 8, GFP_ATOMIC, &kbd->new_dma);
+	if (!(kbd->new))
 		return -1;
-	if (!(kbd->cr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_KERNEL)))
+
+	kbd->cr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_KERNEL);
+	if (!(kbd->cr))
 		return -1;
-	if (!(kbd->leds = usb_alloc_coherent(dev, 1, GFP_ATOMIC, &kbd->leds_dma)))
+
+	kbd->leds = usb_alloc_coherent(dev, 1, GFP_ATOMIC, &kbd->leds_dma);
+	if (!(kbd->leds))
 		return -1;

 	return 0;
@@ -370,9 +392,9 @@ static int usb_kbd_probe(struct usb_interface *iface,
 	device_set_wakeup_enable(&dev->dev, 1);
 	return 0;

-fail2:	
+fail2:
 	usb_kbd_free_mem(dev, kbd);
-fail1:	
+fail1:
 	input_free_device(input_dev);
 	kfree(kbd);
 	return error;
@@ -380,7 +402,7 @@ static int usb_kbd_probe(struct usb_interface *iface,

 static void usb_kbd_disconnect(struct usb_interface *intf)
 {
-	struct usb_kbd *kbd = usb_get_intfdata (intf);
+	struct usb_kbd *kbd = usb_get_intfdata(intf);

 	usb_set_intfdata(intf, NULL);
 	if (kbd) {
@@ -392,13 +414,14 @@ static void usb_kbd_disconnect(struct usb_interface *intf)
 	}
 }

-static struct usb_device_id usb_kbd_id_table [] = {
-	{ USB_INTERFACE_INFO(USB_INTERFACE_CLASS_HID, USB_INTERFACE_SUBCLASS_BOOT,
+static struct usb_device_id usb_kbd_id_table[] = {
+	{ USB_INTERFACE_INFO(USB_INTERFACE_CLASS_HID,
+		USB_INTERFACE_SUBCLASS_BOOT,
 		USB_INTERFACE_PROTOCOL_KEYBOARD) },
-	{ }						/* Terminating entry */
+	{ }		/* Terminating entry */
 };

-MODULE_DEVICE_TABLE (usb, usb_kbd_id_table);
+MODULE_DEVICE_TABLE(usb, usb_kbd_id_table);

 static struct usb_driver usb_kbd_driver = {
 	.name =		"usbkbd",
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] hid: usbhid: usbkbd: fix checkpatch.pl issues
  2017-02-21 17:26 [PATCH] hid: usbhid: usbkbd: fix checkpatch.pl issues Avraham Shukron
@ 2017-03-01 18:16 ` Dmitry Torokhov
  2017-03-01 19:59   ` Avraham Shukron
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Torokhov @ 2017-03-01 18:16 UTC (permalink / raw)
  To: Avraham Shukron
  Cc: jikos, benjamin.tissoires, linux-usb, linux-input, linux-kernel

Hi Avraham,

On Tue, Feb 21, 2017 at 07:26:50PM +0200, Avraham Shukron wrote:
> - Broke long lines
> - Added spaces where needed
> - Removed unnecessary / trailing whitespaces
> - Extracted assignments outside of 'if' statements
> 
> Signed-off-by: Avraham Shukron <avraham.shukron@gmail.com>
> ---
>  drivers/hid/usbhid/usbkbd.c | 121 ++++++++++++++++++++++++++------------------
>  1 file changed, 72 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/usbkbd.c b/drivers/hid/usbhid/usbkbd.c
> index 7fb2d1e..ae40b0d 100644
> --- a/drivers/hid/usbhid/usbkbd.c
> +++ b/drivers/hid/usbhid/usbkbd.c
> @@ -45,22 +45,24 @@ MODULE_DESCRIPTION(DRIVER_DESC);
>  MODULE_LICENSE("GPL");
> 
>  static const unsigned char usb_kbd_keycode[256] = {
> -	  0,  0,  0,  0, 30, 48, 46, 32, 18, 33, 34, 35, 23, 36, 37, 38,
> -	 50, 49, 24, 25, 16, 19, 31, 20, 22, 47, 17, 45, 21, 44,  2,  3,
> -	  4,  5,  6,  7,  8,  9, 10, 11, 28,  1, 14, 15, 57, 12, 13, 26,
> -	 27, 43, 43, 39, 40, 41, 51, 52, 53, 58, 59, 60, 61, 62, 63, 64,
> -	 65, 66, 67, 68, 87, 88, 99, 70,119,110,102,104,111,107,109,106,
> -	105,108,103, 69, 98, 55, 74, 78, 96, 79, 80, 81, 75, 76, 77, 71,
> -	 72, 73, 82, 83, 86,127,116,117,183,184,185,186,187,188,189,190,
> -	191,192,193,194,134,138,130,132,128,129,131,137,133,135,136,113,
> -	115,114,  0,  0,  0,121,  0, 89, 93,124, 92, 94, 95,  0,  0,  0,
> -	122,123, 90, 91, 85,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
> -	  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
> -	  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
> -	  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
> -	  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
> -	 29, 42, 56,125, 97, 54,100,126,164,166,165,163,161,115,114,113,
> -	150,158,159,128,136,177,178,176,142,152,173,140
> +	  0,   0,   0,   0,  30,  48,  46,  32,  18,  33,  34,  35,  23,  36,
> +	 37,  38,  50,  49,  24,  25,  16,  19,  31,  20,  22,  47,  17,  45,
> +	 21,  44,   2,   3,   4,   5,   6,   7,   8,   9,  10,  11,  28,   1,
> +	 14,  15,  57,  12,  13,  26,  27,  43,  43,  39,  40,  41,  51,  52,
> +	 53,  58,  59,  60,  61,  62,  63,  64,  65,  66,  67,  68,  87,  88,
> +	 99,  70, 119, 110, 102, 104, 111, 107, 109, 106, 105, 108, 103,  69,
> +	 98,  55,  74,  78,  96,  79,  80,  81,  75,  76,  77,  71,  72,  73,
> +	 82,  83,  86, 127, 116, 117, 183, 184, 185, 186, 187, 188, 189, 190,
> +	191, 192, 193, 194, 134, 138, 130, 132, 128, 129, 131, 137, 133, 135,
> +	136, 113, 115, 114,   0,   0,   0, 121,   0,  89,  93, 124,  92,  94,
> +	 95,   0,   0,   0, 122, 123,  90,  91,  85,   0,   0,   0,   0,   0,
> +	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
> +	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
> +	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
> +	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
> +	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
> +	 29,  42,  56, 125,  97,  54, 100, 126, 164, 166, 165, 163, 161, 115,
> +	114, 113, 150, 158, 159, 128, 136, 177, 178, 176, 142, 152, 173, 140
>  };

This kind of change is definitely not helpful. The original table was
Nx16, you converted it to Nx14. Why do you think original table used 16
columns?

Regardless, it's a very old driver, just let it be.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hid: usbhid: usbkbd: fix checkpatch.pl issues
  2017-03-01 18:16 ` Dmitry Torokhov
@ 2017-03-01 19:59   ` Avraham Shukron
  2017-03-01 20:43     ` Dmitry Torokhov
                       ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Avraham Shukron @ 2017-03-01 19:59 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: jikos, benjamin.tissoires, linux-usb, linux-input, linux-kernel

> 
> This kind of change is definitely not helpful. The original table was
> Nx16, you converted it to Nx14. Why do you think original table used 16
> columns?
> 
> Regardless, it's a very old driver, just let it be.
> 
> Thanks.
> 

I can make it Nx8 :)

Seriously now - I don't understand what is so wrong with checkpatch fixes?
I'm a new to kernel development, and the natural place to start is to do some
coding style fixes.
I thought fixing a driver that I actually use daily will be more satisfying.
Why driver being old is a good reason to ignore the coding style conventions?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hid: usbhid: usbkbd: fix checkpatch.pl issues
  2017-03-01 19:59   ` Avraham Shukron
@ 2017-03-01 20:43     ` Dmitry Torokhov
  2017-03-01 20:46     ` Joe Perches
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Dmitry Torokhov @ 2017-03-01 20:43 UTC (permalink / raw)
  To: Avraham Shukron
  Cc: jikos, benjamin.tissoires, linux-usb, linux-input, linux-kernel

On Wed, Mar 01, 2017 at 09:59:31PM +0200, Avraham Shukron wrote:
> > 
> > This kind of change is definitely not helpful. The original table was
> > Nx16, you converted it to Nx14. Why do you think original table used 16
> > columns?
> > 
> > Regardless, it's a very old driver, just let it be.
> > 
> > Thanks.
> > 
> 
> I can make it Nx8 :)

Or you can leave it as is.

> 
> Seriously now - I don't understand what is so wrong with checkpatch fixes?

Checkpatch is a tool to make sure new code follows standard conversions,
not reshuffling old working code.

> I'm a new to kernel development, and the natural place to start is to do some
> coding style fixes.
> I thought fixing a driver that I actually use daily will be more satisfying.

You are not using this driver daily, pretty much nobody does. What you
are using is usbhid + hid-input + probably some hardware-specific hid
driver that twiddles the behavior of your keyboard.

> Why driver being old is a good reason to ignore the coding style conventions?

Since there is no active development nor use it is easy to introduce
bugs that won't be caught until much later. Checkpatch fixes are usually
welcome when there are additional fixes to the same driver.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hid: usbhid: usbkbd: fix checkpatch.pl issues
  2017-03-01 19:59   ` Avraham Shukron
  2017-03-01 20:43     ` Dmitry Torokhov
@ 2017-03-01 20:46     ` Joe Perches
  2017-03-01 20:55       ` Avraham Shukron
  2017-03-01 20:56     ` Greg KH
  2017-03-01 22:05     ` Alan Stern
  3 siblings, 1 reply; 8+ messages in thread
From: Joe Perches @ 2017-03-01 20:46 UTC (permalink / raw)
  To: Avraham Shukron, Dmitry Torokhov
  Cc: jikos, benjamin.tissoires, linux-usb, linux-input, linux-kernel

On Wed, 2017-03-01 at 21:59 +0200, Avraham Shukron wrote:
> > 
> > This kind of change is definitely not helpful. The original table was
> > Nx16, you converted it to Nx14. Why do you think original table used 16
> > columns?
> > 
> > Regardless, it's a very old driver, just let it be.
> > 
> > Thanks.
> > 
> 
> I can make it Nx8 :)
> 
> Seriously now - I don't understand what is so wrong with checkpatch fixes?

Some say is makes following logical changes more difficult.
git blame, etc...  I'm not a big adherent of that though.

> I'm a new to kernel development, and the natural place to start is to do some
> coding style fixes.
> I thought fixing a driver that I actually use daily will be more satisfying.
> Why driver being old is a good reason to ignore the coding style conventions?

The space after comma rule _should_ be broken when
alignment is more obvious.  The existing code is just fine.

Conventions are just that.  It's OK to be unconventional.

It's better to know when to follow and when not to follow
those conventions.

checkpatch messages should be considered guides and not
dicta that must be followed to the last letter.

To me, the rest of the patch was OK though.

If you want to learn the kernel patch process, modifying files
in drivers/staging is a good place to start.

Otherwise, it's probably best to find/fix some actual defect
and suggest actual logical changes that reduce code size,
improve performance, expand test coverage, better document
the code, etc..

cheers, Joe

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hid: usbhid: usbkbd: fix checkpatch.pl issues
  2017-03-01 20:46     ` Joe Perches
@ 2017-03-01 20:55       ` Avraham Shukron
  0 siblings, 0 replies; 8+ messages in thread
From: Avraham Shukron @ 2017-03-01 20:55 UTC (permalink / raw)
  To: Joe Perches, Dmitry Torokhov
  Cc: jikos, benjamin.tissoires, linux-usb, linux-input, linux-kernel


On 01/03/17 22:46, Joe Perches wrote:
> On Wed, 2017-03-01 at 21:59 +0200, Avraham Shukron wrote:
>>>
>>> This kind of change is definitely not helpful. The original table was
>>> Nx16, you converted it to Nx14. Why do you think original table used 16
>>> columns?
>>>
>>> Regardless, it's a very old driver, just let it be.
>>>
>>> Thanks.
>>>
>>
>> I can make it Nx8 :)
>>
>> Seriously now - I don't understand what is so wrong with checkpatch fixes?
> 
> Some say is makes following logical changes more difficult.
> git blame, etc...  I'm not a big adherent of that though.
> 
>> I'm a new to kernel development, and the natural place to start is to do some
>> coding style fixes.
>> I thought fixing a driver that I actually use daily will be more satisfying.
>> Why driver being old is a good reason to ignore the coding style conventions?
> 
> The space after comma rule _should_ be broken when
> alignment is more obvious.  The existing code is just fine.
> 
> Conventions are just that.  It's OK to be unconventional.
> 
> It's better to know when to follow and when not to follow
> those conventions.
> 
> checkpatch messages should be considered guides and not
> dicta that must be followed to the last letter.
> 
> To me, the rest of the patch was OK though.
> 
> If you want to learn the kernel patch process, modifying files
> in drivers/staging is a good place to start.
> 
> Otherwise, it's probably best to find/fix some actual defect
> and suggest actual logical changes that reduce code size,
> improve performance, expand test coverage, better document
> the code, etc..
> 
> cheers, Joe
> 

Thanks you very much for the the answer.
That explains the matter.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hid: usbhid: usbkbd: fix checkpatch.pl issues
  2017-03-01 19:59   ` Avraham Shukron
  2017-03-01 20:43     ` Dmitry Torokhov
  2017-03-01 20:46     ` Joe Perches
@ 2017-03-01 20:56     ` Greg KH
  2017-03-01 22:05     ` Alan Stern
  3 siblings, 0 replies; 8+ messages in thread
From: Greg KH @ 2017-03-01 20:56 UTC (permalink / raw)
  To: Avraham Shukron
  Cc: Dmitry Torokhov, jikos, benjamin.tissoires, linux-usb,
	linux-input, linux-kernel

On Wed, Mar 01, 2017 at 09:59:31PM +0200, Avraham Shukron wrote:
> > 
> > This kind of change is definitely not helpful. The original table was
> > Nx16, you converted it to Nx14. Why do you think original table used 16
> > columns?
> > 
> > Regardless, it's a very old driver, just let it be.
> > 
> > Thanks.
> > 
> 
> I can make it Nx8 :)
> 
> Seriously now - I don't understand what is so wrong with checkpatch fixes?
> I'm a new to kernel development, and the natural place to start is to do some
> coding style fixes.

Start doing this in the drivers/staging/ portion of the kernel.  Other
places usually do not want these types of changes for the reasons people
have given you already.

That is what drivers/staging/ is for, to learn the process.  Please
stick to it there.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hid: usbhid: usbkbd: fix checkpatch.pl issues
  2017-03-01 19:59   ` Avraham Shukron
                       ` (2 preceding siblings ...)
  2017-03-01 20:56     ` Greg KH
@ 2017-03-01 22:05     ` Alan Stern
  3 siblings, 0 replies; 8+ messages in thread
From: Alan Stern @ 2017-03-01 22:05 UTC (permalink / raw)
  To: Avraham Shukron
  Cc: Dmitry Torokhov, jikos, benjamin.tissoires, linux-usb,
	linux-input, linux-kernel

On Wed, 1 Mar 2017, Avraham Shukron wrote:

> I thought fixing a driver that I actually use daily will be more satisfying.

Unless there are special reasons, you should not be using the usbkbd
driver.  It is legacy code; everyone should now use usbhid instead.

Alan Stern

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-03-01 22:09 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-21 17:26 [PATCH] hid: usbhid: usbkbd: fix checkpatch.pl issues Avraham Shukron
2017-03-01 18:16 ` Dmitry Torokhov
2017-03-01 19:59   ` Avraham Shukron
2017-03-01 20:43     ` Dmitry Torokhov
2017-03-01 20:46     ` Joe Perches
2017-03-01 20:55       ` Avraham Shukron
2017-03-01 20:56     ` Greg KH
2017-03-01 22:05     ` Alan Stern

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).