LinuxLists.cc - [PATCH V2 1/4] staging: vt6556: Cleanup coding style issues

2014-07-13 19:12:04

Subject: [PATCH V2 1/4] staging: vt6556: Cleanup coding style issues

2014-07-13 19:12:29

Subject: [PATCH V2 3/4] staging: vt6556: Cleanup coding style issues

This patch cleanup coding style issues reported by checkpatch.

Additionally this typedef was removed from card.h:

typedef enum _CARD_PHY_TYPE {
PHY_TYPE_AUTO = 0,
PHY_TYPE_11B,
PHY_TYPE_11G,
PHY_TYPE_11A
} CARD_PHY_TYPE, *PCARD_PHY_TYPE;

Tested by compilation only.

Signed-off-by: Peter Senna Tschudin <[email protected]>
---
Cahnges from V1:
- Sent all patches in a series

Made against latest staging-next.

drivers/staging/vt6656/card.c | 19 ++++++++-----------
drivers/staging/vt6656/card.h | 13 +++----------
2 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/vt6656/card.c b/drivers/staging/vt6656/card.c
index ea06b63..f469d68 100644
--- a/drivers/staging/vt6656/card.c
+++ b/drivers/staging/vt6656/card.c
@@ -55,11 +55,12 @@
#include "key.h"
#include "usbpipe.h"

-//const u16 cwRXBCNTSFOff[MAX_RATE] =
-//{17, 34, 96, 192, 34, 23, 17, 11, 8, 5, 4, 3};
+/* const u16 cwRXBCNTSFOff[MAX_RATE] =
+ {17, 34, 96, 192, 34, 23, 17, 11, 8, 5, 4, 3}; */

-static const u16 cwRXBCNTSFOff[MAX_RATE] =
-{192, 96, 34, 17, 34, 23, 17, 11, 8, 5, 4, 3};
+static const u16 cwRXBCNTSFOff[MAX_RATE] = {
+ 192, 96, 34, 17, 34, 23, 17, 11, 8, 5, 4, 3
+};

/*
* Description: Set NIC media channel
@@ -477,7 +478,7 @@ void vnt_update_top_rates(struct vnt_private *priv)
}

priv->byTopCCKBasicRate = top_cck;
- }
+}

int vnt_ofdm_min_rate(struct vnt_private *priv)
{
@@ -673,9 +674,7 @@ void vnt_reset_next_tbtt(struct vnt_private *priv, u16 beacon_interval)
data[7] = (u8)(next_tbtt >> 56);

vnt_control_out(priv, MESSAGE_TYPE_SET_TSFTBTT,
- MESSAGE_REQUEST_TBTT, 0, 8, data);
-
- return;
+ MESSAGE_REQUEST_TBTT, 0, 8, data);
}

/*
@@ -710,11 +709,9 @@ void vnt_update_next_tbtt(struct vnt_private *priv, u64 tsf,
data[7] = (u8)(tsf >> 56);

vnt_control_out(priv, MESSAGE_TYPE_SET_TSFTBTT,
- MESSAGE_REQUEST_TBTT, 0, 8, data);
+ MESSAGE_REQUEST_TBTT, 0, 8, data);

dev_dbg(&priv->usb->dev, "%s TBTT: %8llx\n", __func__, tsf);
-
- return;
}

/*
diff --git a/drivers/staging/vt6656/card.h b/drivers/staging/vt6656/card.h
index 5b7cc5a..03fc167 100644
--- a/drivers/staging/vt6656/card.h
+++ b/drivers/staging/vt6656/card.h
@@ -32,16 +32,9 @@

/* init card type */

-typedef enum _CARD_PHY_TYPE {
- PHY_TYPE_AUTO = 0,
- PHY_TYPE_11B,
- PHY_TYPE_11G,
- PHY_TYPE_11A
-} CARD_PHY_TYPE, *PCARD_PHY_TYPE;
-
-#define CB_MAX_CHANNEL_24G 14
-#define CB_MAX_CHANNEL_5G 42 /* add channel9(5045MHz), 41==>42 */
-#define CB_MAX_CHANNEL (CB_MAX_CHANNEL_24G+CB_MAX_CHANNEL_5G)
+#define CB_MAX_CHANNEL_24G 14
+#define CB_MAX_CHANNEL_5G 42 /* add channel9(5045MHz), 41==>42 */
+#define CB_MAX_CHANNEL (CB_MAX_CHANNEL_24G + CB_MAX_CHANNEL_5G)

struct vnt_private;

--
1.9.3

2014-07-13 19:12:18

by Peter Senna Tschudin

[permalink] [raw]

Subject: [PATCH V2 2/4] staging: vt6556: Cleanup coding style issues

This patch cleanup coding style issues reported by checkpatch.

Tested by compilation only.

Signed-off-by: Peter Senna Tschudin <[email protected]>
---
Cahnges from V1:
- Sent all patches in a series

Made against latest staging-next.

drivers/staging/vt6656/main_usb.c | 118 +++++++++++++++++---------------------
1 file changed, 54 insertions(+), 64 deletions(-)

diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index 4cdf29e..13be8b2 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -62,7 +62,7 @@
#include "int.h"

/* static int msglevel = MSG_LEVEL_DEBUG; */
-static int msglevel =MSG_LEVEL_INFO;
+static int msglevel = MSG_LEVEL_INFO;

/*
* define module options
@@ -75,18 +75,18 @@ MODULE_AUTHOR(DRIVER_AUTHOR);
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION(DEVICE_FULL_DRV_NAM);

-#define DEVICE_PARAM(N,D) \
- static int N[MAX_UINTS]=OPTION_DEFAULT;\
- module_param_array(N, int, NULL, 0);\
- MODULE_PARM_DESC(N, D);
+#define DEVICE_PARAM(N, D) \
+ static int N[MAX_UINTS] = OPTION_DEFAULT; \
+ module_param_array(N, int, NULL, 0); \
+ MODULE_PARM_DESC(N, D)

-#define RX_DESC_DEF0 64
-DEVICE_PARAM(RxDescriptors0,"Number of receive usb desc buffer");
+#define RX_DESC_DEF0 64
+DEVICE_PARAM(RxDescriptors0, "Number of receive usb desc buffer");

-#define TX_DESC_DEF0 64
-DEVICE_PARAM(TxDescriptors0,"Number of transmit usb desc buffer");
+#define TX_DESC_DEF0 64
+DEVICE_PARAM(TxDescriptors0, "Number of transmit usb desc buffer");

-#define CHANNEL_DEF 6
+#define CHANNEL_DEF 6
DEVICE_PARAM(Channel, "Channel number");

/* PreambleType[] is the preamble length used for transmit.
@@ -177,13 +177,12 @@ static struct usb_device_id vt6656_table[] = {
/* frequency list (map channels to frequencies) */
/*
static const long frequency_list[] = {
- 2412, 2417, 2422, 2427, 2432, 2437, 2442, 2447, 2452, 2457, 2462, 2467, 2472, 2484,
- 4915, 4920, 4925, 4935, 4940, 4945, 4960, 4980,
- 5035, 5040, 5045, 5055, 5060, 5080, 5170, 5180, 5190, 5200, 5210, 5220, 5230, 5240,
- 5260, 5280, 5300, 5320, 5500, 5520, 5540, 5560, 5580, 5600, 5620, 5640, 5660, 5680,
- 5700, 5745, 5765, 5785, 5805, 5825
- };
-
+ 2412, 2417, 2422, 2427, 2432, 2437, 2442, 2447, 2452, 2457, 2462, 2467,
+ 2472, 2484, 4915, 4920, 4925, 4935, 4940, 4945, 4960, 4980, 5035, 5040,
+ 5045, 5055, 5060, 5080, 5170, 5180, 5190, 5200, 5210, 5220, 5230, 5240,
+ 5260, 5280, 5300, 5320, 5500, 5520, 5540, 5560, 5580, 5600, 5620, 5640,
+ 5660, 5680, 5700, 5745, 5765, 5785, 5805, 5825
+};
*/

static int vt6656_probe(struct usb_interface *intf,
@@ -206,16 +205,16 @@ static void usb_device_reset(struct vnt_private *pDevice);

static void
device_set_options(struct vnt_private *pDevice) {
- pDevice->cbTD = TX_DESC_DEF0;
- pDevice->cbRD = RX_DESC_DEF0;
- pDevice->byShortRetryLimit = SHORT_RETRY_DEF;
- pDevice->byLongRetryLimit = LONG_RETRY_DEF;
- pDevice->op_mode = NL80211_IFTYPE_UNSPECIFIED;
- pDevice->byBBType = BBP_TYPE_DEF;
- pDevice->byPacketType = pDevice->byBBType;
- pDevice->byAutoFBCtrl = AUTO_FB_0;
- pDevice->byPreambleType = 0;
- pDevice->bExistSWNetAddr = false;
+ pDevice->cbTD = TX_DESC_DEF0;
+ pDevice->cbRD = RX_DESC_DEF0;
+ pDevice->byShortRetryLimit = SHORT_RETRY_DEF;
+ pDevice->byLongRetryLimit = LONG_RETRY_DEF;
+ pDevice->op_mode = NL80211_IFTYPE_UNSPECIFIED;
+ pDevice->byBBType = BBP_TYPE_DEF;
+ pDevice->byPacketType = pDevice->byBBType;
+ pDevice->byAutoFBCtrl = AUTO_FB_0;
+ pDevice->byPreambleType = 0;
+ pDevice->bExistSWNetAddr = false;
}

/*
@@ -383,38 +382,40 @@ static int device_init_registers(struct vnt_private *pDevice)
/* load vt3266 calibration parameters in EEPROM */
if (pDevice->byRFType == RF_VT3226D0) {
if ((pDevice->abyEEPROM[EEP_OFS_MAJOR_VER] == 0x1) &&
- (pDevice->abyEEPROM[EEP_OFS_MINOR_VER] >= 0x4)) {
+ (pDevice->abyEEPROM[EEP_OFS_MINOR_VER] >= 0x4)) {

byCalibTXIQ = pDevice->abyEEPROM[EEP_OFS_CALIB_TX_IQ];
byCalibTXDC = pDevice->abyEEPROM[EEP_OFS_CALIB_TX_DC];
byCalibRXIQ = pDevice->abyEEPROM[EEP_OFS_CALIB_RX_IQ];
if (byCalibTXIQ || byCalibTXDC || byCalibRXIQ) {
- /* CR255, enable TX/RX IQ and DC compensation mode */
+ /* CR255, enable TX/RX IQ and
+ DC compensation mode */
vnt_control_out_u8(pDevice,
- MESSAGE_REQUEST_BBREG,
- 0xff,
- 0x03);
- /* CR251, TX I/Q Imbalance Calibration */
+ MESSAGE_REQUEST_BBREG,
+ 0xff,
+ 0x03);
+ /* CR251, TX I/Q Imbalance Calibration */
vnt_control_out_u8(pDevice,
- MESSAGE_REQUEST_BBREG,
- 0xfb,
- byCalibTXIQ);
- /* CR252, TX DC-Offset Calibration */
+ MESSAGE_REQUEST_BBREG,
+ 0xfb,
+ byCalibTXIQ);
+ /* CR252, TX DC-Offset Calibration */
vnt_control_out_u8(pDevice,
- MESSAGE_REQUEST_BBREG,
- 0xfC,
- byCalibTXDC);
- /* CR253, RX I/Q Imbalance Calibration */
+ MESSAGE_REQUEST_BBREG,
+ 0xfC,
+ byCalibTXDC);
+ /* CR253, RX I/Q Imbalance Calibration */
vnt_control_out_u8(pDevice,
- MESSAGE_REQUEST_BBREG,
- 0xfd,
- byCalibRXIQ);
+ MESSAGE_REQUEST_BBREG,
+ 0xfd,
+ byCalibRXIQ);
} else {
- /* CR255, turn off BB Calibration compensation */
+ /* CR255, turn off
+ BB Calibration compensation */
vnt_control_out_u8(pDevice,
- MESSAGE_REQUEST_BBREG,
- 0xff,
- 0x0);
+ MESSAGE_REQUEST_BBREG,
+ 0xff,
+ 0x0);
}
}
}
@@ -494,8 +495,6 @@ static void device_free_tx_bufs(struct vnt_private *priv)

kfree(tx_context);
}
-
- return;
}

static void device_free_rx_bufs(struct vnt_private *priv)
@@ -520,24 +519,20 @@ static void device_free_rx_bufs(struct vnt_private *priv)

kfree(rcb);
}
-
- return;
}

static void usb_device_reset(struct vnt_private *pDevice)
{
- int status;
- status = usb_reset_device(pDevice->usb);
+ int status;
+
+ status = usb_reset_device(pDevice->usb);
if (status)
- printk("usb_device_reset fail status=%d\n",status);
- return ;
+ pr_warn("usb_device_reset fail status=%d\n", status);
}

static void device_free_int_bufs(struct vnt_private *priv)
{
kfree(priv->int_buf.data_buf);
-
- return;
}

static bool device_alloc_bufs(struct vnt_private *priv)
@@ -718,8 +713,6 @@ static void vnt_stop(struct ieee80211_hw *hw)

usb_kill_urb(priv->pInterruptURB);
usb_free_urb(priv->pInterruptURB);
-
- return;
}

static int vnt_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
@@ -785,8 +778,6 @@ static void vnt_remove_interface(struct ieee80211_hw *hw,

/* LED slow blink */
vnt_mac_set_led(priv, LEDSTS_STS, LEDSTS_SLOW);
-
- return;
}

static int vnt_config(struct ieee80211_hw *hw, u32 changed)
@@ -836,6 +827,7 @@ static void vnt_bss_info_changed(struct ieee80211_hw *hw,
u32 changed)
{
struct vnt_private *priv = hw->priv;
+
priv->current_aid = conf->aid;

if (changed & BSS_CHANGED_BSSID)
@@ -965,8 +957,6 @@ static void vnt_configure(struct ieee80211_hw *hw,
vnt_control_out_u8(priv, MESSAGE_REQUEST_MACREG, MAC_REG_RCR, rx_mode);

dev_dbg(&priv->usb->dev, "rx mode out= %x\n", rx_mode);
-
- return;
}

static int vnt_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
--
1.9.3

2014-07-13 19:12:40

by Peter Senna Tschudin

[permalink] [raw]

Subject: [PATCH V2 4/4] staging: vt6556: Cleanup coding style issues

This patch cleanup coding style issues reported by checkpatch.

This patch change the following enums:
- typedef enum __device_msg_level
- typedef enum __DEVICE_NDIS_STATUS

Tested by compilation only.

Signed-off-by: Peter Senna Tschudin <[email protected]>
---
Cahnges from V1:
- Sent all patches in a series

Made against latest staging-next.

drivers/staging/vt6656/channel.c | 5 +++--
drivers/staging/vt6656/device.h | 27 +++++++++++++--------------
2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/vt6656/channel.c b/drivers/staging/vt6656/channel.c
index fb1838e..4a53f1a 100644
--- a/drivers/staging/vt6656/channel.c
+++ b/drivers/staging/vt6656/channel.c
@@ -28,8 +28,9 @@
*
*
* Revision History:
- * 01-18-2005 RobertYu: remove the for loop searching in ChannelValid,
- * change ChannelRuleTab to lookup-type, reorder table items.
+ * 01-18-2005 RobertYu: remove the for loop searching in
+ * ChannelValid, change ChannelRuleTab
+ * to lookup-type, reorder table items.
*
*
*/
diff --git a/drivers/staging/vt6656/device.h b/drivers/staging/vt6656/device.h
index 789c55d..e3acf2f 100644
--- a/drivers/staging/vt6656/device.h
+++ b/drivers/staging/vt6656/device.h
@@ -187,13 +187,13 @@

#define DBG_PRT(l, p, args...) { if (l <= msglevel) printk(p, ##args); }

-typedef enum __device_msg_level {
+enum {
MSG_LEVEL_ERR = 0, /* Errors causing abnormal operation */
MSG_LEVEL_NOTICE = 1, /* Errors needing user notification */
MSG_LEVEL_INFO = 2, /* Normal message. */
MSG_LEVEL_VERBOSE = 3, /* Will report all trival errors. */
MSG_LEVEL_DEBUG = 4 /* Only for debug purpose. */
-} DEVICE_MSG_LEVEL, *PDEVICE_MSG_LEVEL;
+};

#define DEVICE_INIT_COLD 0x0 /* cold init */
#define DEVICE_INIT_RESET 0x1 /* reset init or Dx to D0 power remain */
@@ -268,13 +268,12 @@ struct vnt_interrupt_buffer {

/*++ NDIS related */

-typedef enum __DEVICE_NDIS_STATUS {
- STATUS_SUCCESS = 0,
- STATUS_FAILURE,
- STATUS_RESOURCES,
- STATUS_PENDING,
-} DEVICE_NDIS_STATUS, *PDEVICE_NDIS_STATUS;
-
+enum {
+ STATUS_SUCCESS = 0,
+ STATUS_FAILURE,
+ STATUS_RESOURCES,
+ STATUS_PENDING,
+};

/* flags for options */
#define DEVICE_FLAGS_UNPLUG 0x00000001UL
@@ -443,11 +442,11 @@ struct vnt_private {
struct ieee80211_low_level_stats low_stats;
};

-#define ADD_ONE_WITH_WRAP_AROUND(uVar, uModulo) { \
- if ((uVar) >= ((uModulo) - 1)) \
- (uVar) = 0; \
- else \
- (uVar)++; \
+#define ADD_ONE_WITH_WRAP_AROUND(uVar, uModulo) { \
+ if ((uVar) >= ((uModulo) - 1)) \
+ (uVar) = 0; \
+ else \
+ (uVar)++; \
}

#define fMP_DISCONNECTED 0x00000002
--
1.9.3

2014-07-13 19:32:22

by Greg KH

[permalink] [raw]

Subject: Re: [PATCH V2 1/4] staging: vt6556: Cleanup coding style issues

On Sun, Jul 13, 2014 at 09:11:18PM +0200, Peter Senna Tschudin wrote:
> This patch cleanup coding style issues reported by checkpatch.
>
> Tested by compilation only.
>
> Signed-off-by: Peter Senna Tschudin <[email protected]>
> ---
> Cahnges from V1:
> - Sent all patches in a series

Why did you forget the other thing that I asked you to change?

I can't take these as-is, sorry, please go back and re-read what I
wrote...

greg k-h

2014-07-14 08:59:58

by Peter Senna Tschudin

[permalink] [raw]

Subject: Re: [PATCH V2 1/4] staging: vt6556: Cleanup coding style issues

On Sun, Jul 13, 2014 at 12:36:47PM -0700, Greg KH wrote:
> On Sun, Jul 13, 2014 at 09:11:18PM +0200, Peter Senna Tschudin wrote:
> > This patch cleanup coding style issues reported by checkpatch.
> >
> > Tested by compilation only.
> >
> > Signed-off-by: Peter Senna Tschudin <[email protected]>
> > ---
> > Cahnges from V1:
> > - Sent all patches in a series
>
> Why did you forget the other thing that I asked you to change?
>
> I can't take these as-is, sorry, please go back and re-read what I
> wrote...

Actually I was trying to do what you asked me on this E-mail:

-- // --
You just sent me 4 patches, all with the same subject (but at least 2 of
them had the order in which to apply them in, which is nice.)

Please redo these such that they have a unique subject, and I can tell
which order to apply them in, as I really have no idea about the second
two.
-- // --

I'm sending V3 as a single patch.

>
> greg k-h

Attachments:

(No filename) (935.00 B)
0001-staging-vt6556-Cleanup-coding-style-issues.patch (15.19 kB)
Download all attachments

2014-07-14 12:59:05

by Greg KH

[permalink] [raw]

Subject: Re: [PATCH V2 1/4] staging: vt6556: Cleanup coding style issues

On Mon, Jul 14, 2014 at 10:59:32AM +0200, Peter Senna Tschudin wrote:
> On Sun, Jul 13, 2014 at 12:36:47PM -0700, Greg KH wrote:
> > On Sun, Jul 13, 2014 at 09:11:18PM +0200, Peter Senna Tschudin wrote:
> > > This patch cleanup coding style issues reported by checkpatch.
> > >
> > > Tested by compilation only.
> > >
> > > Signed-off-by: Peter Senna Tschudin <[email protected]>
> > > ---
> > > Cahnges from V1:
> > > - Sent all patches in a series
> >
> > Why did you forget the other thing that I asked you to change?
> >
> > I can't take these as-is, sorry, please go back and re-read what I
> > wrote...
>
> Actually I was trying to do what you asked me on this E-mail:
>
> -- // --
> You just sent me 4 patches, all with the same subject (but at least 2 of
> them had the order in which to apply them in, which is nice.)
>
> Please redo these such that they have a unique subject, and I can tell
> which order to apply them in, as I really have no idea about the second
> two.
> -- // --
>
> I'm sending V3 as a single patch.

That's going to get rejected as well :(

You did put them in a nice order, but you had the same subject text for
each patch, which I told you not to do, right?

{sigh}

greg k-h

2014-07-14 14:03:17

by Peter Senna Tschudin

[permalink] [raw]

Subject: Re: [PATCH V2 1/4] staging: vt6556: Cleanup coding style issues

On Mon, Jul 14, 2014 at 2:58 PM, Greg KH <[email protected]> wrote:
> On Mon, Jul 14, 2014 at 10:59:32AM +0200, Peter Senna Tschudin wrote:
>> On Sun, Jul 13, 2014 at 12:36:47PM -0700, Greg KH wrote:
>> > On Sun, Jul 13, 2014 at 09:11:18PM +0200, Peter Senna Tschudin wrote:
>> > > This patch cleanup coding style issues reported by checkpatch.
>> > >
>> > > Tested by compilation only.
>> > >
>> > > Signed-off-by: Peter Senna Tschudin <[email protected]>
>> > > ---
>> > > Cahnges from V1:
>> > > - Sent all patches in a series
>> >
>> > Why did you forget the other thing that I asked you to change?
>> >
>> > I can't take these as-is, sorry, please go back and re-read what I
>> > wrote...
>>
>> Actually I was trying to do what you asked me on this E-mail:
>>
>> -- // --
>> You just sent me 4 patches, all with the same subject (but at least 2 of
>> them had the order in which to apply them in, which is nice.)
>>
>> Please redo these such that they have a unique subject, and I can tell
>> which order to apply them in, as I really have no idea about the second
>> two.
>> -- // --
>>
>> I'm sending V3 as a single patch.
>
> That's going to get rejected as well :(
>
> You did put them in a nice order, but you had the same subject text for
> each patch, which I told you not to do, right?
>
> {sigh}
The noise was caused by my poor English language skills. When you wrote:

"Please redo these such that they have a unique subject, and I can tell
which order to apply them in, as I really have no idea about the second
two."

I understood:
1 - redo these patches
2 - such as they have a unique subject
3 - in a way Greg can tell in which order to apply them

I just understood what you wanted today. Sorry for that.

But, there is a plain text attachment on my previous E-mail with the
V3 of the patch set. The V3 is a single patch instead of 4 patches, is
that ok? Should I resend in a new thread?

>
> greg k-h

--
Peter

2014-07-14 14:12:48

by Dan Carpenter

[permalink] [raw]

Subject: Re: [PATCH V2 1/4] staging: vt6556: Cleanup coding style issues

Since you're going to redo this patch anyway, I may as well give you
the normal feedback for these kinds of patches.

> >From 69cd87aca39730c0578592d1296b738f7f223f29 Mon Sep 17 00:00:00 2001
> From: Peter Senna Tschudin <[email protected]>
> Date: Mon, 14 Jul 2014 10:28:42 +0200
> Subject: [PATCH V3] staging: vt6556: Cleanup coding style issues
>
> This patch cleanup coding style issues reported by checkpatch.
>
> This typedef, reported by checkpatch, was removed from card.h:
> typedef enum _CARD_PHY_TYPE {
> PHY_TYPE_AUTO = 0,
> PHY_TYPE_11B,
> PHY_TYPE_11G,
> PHY_TYPE_11A
> } CARD_PHY_TYPE, *PCARD_PHY_TYPE;
>
> The following typedefs were removed, but enums were kept at device.h:
> - typedef enum __device_msg_level
> - typedef enum __DEVICE_NDIS_STATUS
>

Break this kind of patch into patches which fix one type of mistake per
patch:
patch 1: fix whitespace stuff
patch 2: remove useless returns
patch 3: remove typedefs
etc.

> -//{{RobertYu:20060515, new BB setting for VT3226D0
> +/* {{RobertYu:20060515, new BB setting for VT3226D0 */

Just delete these, because we have version control.

regards,
dan carpenter

2014-07-14 17:01:47

by Peter Senna Tschudin

[permalink] [raw]

Subject: Re: [PATCH V2 1/4] staging: vt6556: Cleanup coding style issues

<note>
I'm not trying to push my changes over the rules. I'm trying to
understand the problem, to avoid creating similar noise in the future.
</note>

Now I understand that the problem with the series of 4 patches is that
the subject is the same on the 4 patches. Having the same subject in 4
patches is not good. I got this one.

But I have no clue why joining 4 cleanup patches into 1 is bad. The
patches are all for the same driver, are all silencing checkpatch
warnings, and even the typedef stuff was reported by checkpatch. The
commit message of the single patch describes it all. If the subject of
the series is the problem, why not make a single patch instead of a
series of similar patches? It made sense from my perspective. So what
is the problem in re-submit 4 similar patches as a single patch?

On Mon, Jul 14, 2014 at 4:12 PM, Dan Carpenter <[email protected]> wrote:
> Since you're going to redo this patch anyway, I may as well give you
> the normal feedback for these kinds of patches.
>
>> >From 69cd87aca39730c0578592d1296b738f7f223f29 Mon Sep 17 00:00:00 2001
>> From: Peter Senna Tschudin <[email protected]>
>> Date: Mon, 14 Jul 2014 10:28:42 +0200
>> Subject: [PATCH V3] staging: vt6556: Cleanup coding style issues
>>
>> This patch cleanup coding style issues reported by checkpatch.
>>
>> This typedef, reported by checkpatch, was removed from card.h:
>> typedef enum _CARD_PHY_TYPE {
>> PHY_TYPE_AUTO = 0,
>> PHY_TYPE_11B,
>> PHY_TYPE_11G,
>> PHY_TYPE_11A
>> } CARD_PHY_TYPE, *PCARD_PHY_TYPE;
>>
>> The following typedefs were removed, but enums were kept at device.h:
>> - typedef enum __device_msg_level
>> - typedef enum __DEVICE_NDIS_STATUS
>>
>
> Break this kind of patch into patches which fix one type of mistake per
> patch:
> patch 1: fix whitespace stuff
> patch 2: remove useless returns
> patch 3: remove typedefs
> etc.
>
>> -//{{RobertYu:20060515, new BB setting for VT3226D0
>> +/* {{RobertYu:20060515, new BB setting for VT3226D0 */
>
> Just delete these, because we have version control.
>
> regards,
> dan carpenter
>

--
Peter

2014-07-14 17:09:49

by Dan Carpenter

[permalink] [raw]

Subject: Re: [PATCH V2 1/4] staging: vt6556: Cleanup coding style issues

On Mon, Jul 14, 2014 at 07:01:37PM +0200, Peter Senna Tschudin wrote:
> <note>
> I'm not trying to push my changes over the rules. I'm trying to
> understand the problem, to avoid creating similar noise in the future.
> </note>
>
> Now I understand that the problem with the series of 4 patches is that
> the subject is the same on the 4 patches. Having the same subject in 4
> patches is not good. I got this one.
>
> But I have no clue why joining 4 cleanup patches into 1 is bad. The
> patches are all for the same driver, are all silencing checkpatch
> warnings, and even the typedef stuff was reported by checkpatch. The
> commit message of the single patch describes it all. If the subject of
> the series is the problem, why not make a single patch instead of a
> series of similar patches? It made sense from my perspective. So what
> is the problem in re-submit 4 similar patches as a single patch?

The one thing per patch rule is a bit ambiguous, but normally we auto
reject patches which "fix every checkpatch warning in somefile_foo.c"
and sugest that they instead be broken into one type of fix per patch.

Breaking it up like this is maybe not always beautiful but it's simple
to explain to newbies and generally easier to review.

If there are very few warnings in the file then "fix everything" is ok.

regards,
dan carpenter

2014-07-14 17:21:38

by Greg KH

[permalink] [raw]

Subject: Re: [PATCH V2 1/4] staging: vt6556: Cleanup coding style issues

On Mon, Jul 14, 2014 at 07:01:37PM +0200, Peter Senna Tschudin wrote:
> <note>
> I'm not trying to push my changes over the rules. I'm trying to
> understand the problem, to avoid creating similar noise in the future.
> </note>
>
> Now I understand that the problem with the series of 4 patches is that
> the subject is the same on the 4 patches. Having the same subject in 4
> patches is not good. I got this one.
>
> But I have no clue why joining 4 cleanup patches into 1 is bad. The
> patches are all for the same driver, are all silencing checkpatch
> warnings, and even the typedef stuff was reported by checkpatch. The
> commit message of the single patch describes it all. If the subject of
> the series is the problem, why not make a single patch instead of a
> series of similar patches? It made sense from my perspective. So what
> is the problem in re-submit 4 similar patches as a single patch?

Because it is _much_ harder to review a patch that way. I get a few
hundred patches a week to review. If you only do one thing per patch,
it is trivial to review, and you don't have to pick through a patch to
determine if all of it is correct based on a larger patch, that does
multiple things.

Also, the rule for a kernel patch is "do only one thing". If you do:
- remove typedef
- fix space layout
for a single file, that really is 2 different things, yet you could
claim they are both "coding style cleanups". Reviewing both of these at
the same time, together, makes it much harder to do.

Remember, for the kernel, we waste individual developer's time, at the
expense of reviewer's time, as we have far more developers than
reviewers.

Hope this helps explain things more.

greg k-h