Skip to contentSkip to conversation

b43: reduce checkpatch.pl errors

PATCH In review John Daiker 2 participants 6 messages Oct 17, 2008 Networking drivers (wireless)

lore
Share & tools
lore

John Daiker

author
#0 lore raw

PATCH 1/1 b43: reduce checkpatch.pl errors

A few changes to reduce checkpatch.pl errors in the b43 driver. For the most part, I only fixed cosmetic things, and left the actual 'code flow' untouched (hopefully)!

Diff is against wireless-testing HEAD.

Signed-off-by John Daiker <[email protected]>

---

drivers/net/wireless/b43/debugfs.c +2 -2
index 06a01da..af5eec7 100644
@@ -51,8 +51,8 @@ struct b43_debugfs_fops {
 };
 
 static inline
-struct b43_dfs_file * fops_to_dfs_file(struct b43_wldev *dev,
-				       const struct b43_debugfs_fops *dfops)
+struct b43_dfs_file *fops_to_dfs_file(struct b43_wldev *dev,
+				      const struct b43_debugfs_fops *dfops)
 {
 	void *p;
drivers/net/wireless/b43/dma.c +2 -2
index 098f886..e399fe5 100644
@@ -1254,8 +1254,8 @@ static inline int should_inject_overflow(struct b43_dmaring *ring)
 }
 
 /* Static mapping of mac80211's queues (priorities) to b43 DMA rings. */
-static struct b43_dmaring * select_ring_by_priority(struct b43_wldev *dev,
-						    u8 queue_prio)
+static struct b43_dmaring *select_ring_by_priority(struct b43_wldev *dev,
+						   u8 queue_prio)
 {
 	struct b43_dmaring *ring;
drivers/net/wireless/b43/lo.c +19 -19
index 6a18a14..7c7cff4 100644
@@ -36,9 +36,9 @@
 #include <linux/sched.h>
 
 
-static struct b43_lo_calib * b43_find_lo_calib(struct b43_txpower_lo_control *lo,
-					       const struct b43_bbatt *bbatt,
-					       const struct b43_rfatt *rfatt)
+static struct b43_lo_calib *b43_find_lo_calib(struct b43_txpower_lo_control *lo,
+					      const struct b43_bbatt *bbatt,
+					      const struct b43_rfatt *rfatt)
 {
 	struct b43_lo_calib *c;
 

@@ -138,7 +138,7 @@ static u16 lo_measure_feedthrough(struct b43_wldev *dev,
  * "pad_mix_gain" is the PAD Mixer Gain.
  */
 static u16 lo_txctl_register_table(struct b43_wldev *dev,
-				   u16 * value, u16 * pad_mix_gain)
+				   u16 *value, u16 *pad_mix_gain)
 {
 	struct b43_phy *phy = &dev->phy;
 	u16 reg, v, padmix;

@@ -615,14 +615,14 @@ static int lo_probe_possible_loctls(struct b43_wldev *dev,
 	u16 feedth;
 
 	static const struct b43_loctl modifiers[] = {
-		{.i = 1,.q = 1,},
-		{.i = 1,.q = 0,},
-		{.i = 1,.q = -1,},
-		{.i = 0,.q = -1,},
-		{.i = -1,.q = -1,},
-		{.i = -1,.q = 0,},
-		{.i = -1,.q = 1,},
-		{.i = 0,.q = 1,},
+		{.i = 1, .q = 1,},
+		{.i = 1, .q = 0,},
+		{.i = 1, .q = -1,},
+		{.i = 0, .q = -1,},
+		{.i = -1, .q = -1,},
+		{.i = -1, .q = 0,},
+		{.i = -1, .q = 1,},
+		{.i = 0, .q = 1,},
 	};
 
 	if (d->current_state == 0) {

@@ -754,9 +754,9 @@ static void lo_probe_loctls_statemachine(struct b43_wldev *dev,
 }
 
 static
-struct b43_lo_calib * b43_calibrate_lo_setting(struct b43_wldev *dev,
-					       const struct b43_bbatt *bbatt,
-					       const struct b43_rfatt *rfatt)
+struct b43_lo_calib *b43_calibrate_lo_setting(struct b43_wldev *dev,
+					      const struct b43_bbatt *bbatt,
+					      const struct b43_rfatt *rfatt)
 {
 	struct b43_phy *phy = &dev->phy;
 	struct b43_phy_g *gphy = phy->g;

@@ -824,9 +824,9 @@ struct b43_lo_calib * b43_calibrate_lo_setting(struct b43_wldev *dev,
 /* Get a calibrated LO setting for the given attenuation values.
  * Might return a NULL pointer under OOM! */
 static
-struct b43_lo_calib * b43_get_calib_lo_settings(struct b43_wldev *dev,
-						const struct b43_bbatt *bbatt,
-						const struct b43_rfatt *rfatt)
+struct b43_lo_calib *b43_get_calib_lo_settings(struct b43_wldev *dev,
+					       const struct b43_bbatt *bbatt,
+					       const struct b43_rfatt *rfatt)
 {
 	struct b43_txpower_lo_control *lo = dev->phy.g->lo_control;
 	struct b43_lo_calib *c;

@@ -981,7 +981,7 @@ void b43_lo_g_maintanance_work(struct b43_wldev *dev)
 			lo_read_power_vector(dev);
 			b43_gphy_dc_lt_init(dev, 0);
 		}
-		//FIXME Recalc the whole DC table from time to time?
+		/* FIXME Recalc the whole DC table from time to time? */
 	}
 
 	if (hwpctl)
drivers/net/wireless/b43/main.c +33 -25

168 lines not on this page · raw message

drivers/net/wireless/b43/phy_a.c +19 -14
index 0f1a84c..0d0d4a4 100644
@@ -71,7 +71,7 @@ static s8 b43_aphy_estimate_power_out(struct b43_wldev *dev, s8 tssi)
 	tmp += 0x80;
 	tmp = clamp_val(tmp, 0x00, 0xFF);
 	dbm = aphy->tssi2dbm[tmp];
-	//TODO: There's a FIXME on the specs
+	/* TODO: There's a FIXME on the specs */
 
 	return dbm;
 }

@@ -105,10 +105,10 @@ static void aphy_channel_switch(struct b43_wldev *dev, unsigned int channel)
 	b43_write16(dev, 0x03F0, freq);
 	b43_radio_write16(dev, 0x0008, r8);
 
-	//TODO: write max channel TX power? to Radio 0x2D
+	/* TODO: write max channel TX power? to Radio 0x2D */
 	tmp = b43_radio_read16(dev, 0x002E);
 	tmp &= 0x0080;
-	//TODO: OR tmp with the Power out estimation for this channel?
+	/* TODO: OR tmp with the Power out estimation for this channel? */
 	b43_radio_write16(dev, 0x002E, tmp);
 
 	if (freq >= 4920 && freq <= 5500) {

@@ -143,8 +143,8 @@ static void aphy_channel_switch(struct b43_wldev *dev, unsigned int channel)
 	b43_radio_write16(dev, 0x0035, (b43_radio_read16(dev, 0x0035)
 					& 0xFFEF) | 0x0010);
 	b43_radio_set_tx_iq(dev);
-	//TODO: TSSI2dbm workaround
-//FIXME	b43_phy_xmitpower(dev);
+	/* TODO: TSSI2dbm workaround */
+	/* FIXME b43_phy_xmitpower(dev); */
 }
 
 void b43_radio_init2060(struct b43_wldev *dev)

@@ -282,7 +282,7 @@ static void b43_phy_ww(struct b43_wldev *dev)
 
 static void hardware_pctl_init_aphy(struct b43_wldev *dev)
 {
-	//TODO
+	/* TODO */
 }
 
 void b43_phy_inita(struct b43_wldev *dev)

@@ -326,7 +326,7 @@ void b43_phy_inita(struct b43_wldev *dev)
 		if ((bus->boardinfo.vendor == SSB_BOARDVENDOR_BCM) &&
 		    ((bus->boardinfo.type == SSB_BOARD_BU4306) ||
 		     (bus->boardinfo.type == SSB_BOARD_BU4309))) {
-			; //TODO: A PHY LO
+			; /* TODO: A PHY LO */
 		}
 
 		if (phy->rev >= 3)

@@ -334,7 +334,7 @@ void b43_phy_inita(struct b43_wldev *dev)
 
 		hardware_pctl_init_aphy(dev);
 
-		//TODO: radar detection
+		/* TODO: radar detection */
 	}
 
 	if ((phy->type == B43_PHYTYPE_G) &&

@@ -422,7 +422,7 @@ static void b43_aphy_op_prepare_structs(struct b43_wldev *dev)
 	aphy->tssi2dbm = tssi2dbm;
 	aphy->tgt_idle_tssi = tgt_idle_tssi;
 
-	//TODO init struct b43_phy_a
+	/* TODO init struct b43_phy_a */
 
 }
 

@@ -547,7 +547,8 @@ static unsigned int b43_aphy_op_get_default_chan(struct b43_wldev *dev)
 }
 
 static void b43_aphy_op_set_rx_antenna(struct b43_wldev *dev, int antenna)
-{//TODO
+{
+	/* TODO */
 	struct b43_phy *phy = &dev->phy;
 	u64 hf;
 	u16 tmp;

@@ -604,21 +605,25 @@ static void b43_aphy_op_set_rx_antenna(struct b43_wldev *dev, int antenna)
 }
 
 static void b43_aphy_op_adjust_txpower(struct b43_wldev *dev)
-{//TODO
+{
+	/* TODO */
 }
 
 static enum b43_txpwr_result b43_aphy_op_recalc_txpower(struct b43_wldev *dev,
 							bool ignore_tssi)
-{//TODO
+{
+	/* TODO */
 	return B43_TXPWR_RES_DONE;
 }
 
 static void b43_aphy_op_pwork_15sec(struct b43_wldev *dev)
-{//TODO
+{
+	/* TODO */
 }
 
 static void b43_aphy_op_pwork_60sec(struct b43_wldev *dev)
-{//TODO
+{
+	/* TODO */
 }
 
 const struct b43_phy_operations b43_phyops_a = {
drivers/net/wireless/b43/phy_common.c +1 -1
index af37abc..decddcf 100644
@@ -249,7 +249,7 @@ int b43_switch_channel(struct b43_wldev *dev, unsigned int new_channel)
 	channelcookie = new_channel;
 	if (b43_current_band(dev->wl) == IEEE80211_BAND_5GHZ)
 		channelcookie |= 0x100;
-	//FIXME set 40Mhz flag if required
+	/* FIXME set 40Mhz flag if required */
 	savedcookie = b43_shm_read16(dev, B43_SHM_SHARED, B43_SHM_SH_CHAN);
 	b43_shm_write16(dev, B43_SHM_SHARED, B43_SHM_SH_CHAN, channelcookie);
drivers/net/wireless/b43/phy_g.c +39 -37

126 lines not on this page · raw message

drivers/net/wireless/b43/phy_lp.c +7 -7
index c5d9dc3..ffcc39a 100644
@@ -46,7 +46,7 @@ static void b43_lpphy_op_prepare_structs(struct b43_wldev *dev)
 
 	memset(lpphy, 0, sizeof(*lpphy));
 
-	//TODO
+	/* TODO */
 }
 
 static void b43_lpphy_op_free(struct b43_wldev *dev)

@@ -59,7 +59,7 @@ static void b43_lpphy_op_free(struct b43_wldev *dev)
 
 static int b43_lpphy_op_init(struct b43_wldev *dev)
 {
-	//TODO
+	/* TODO */
 
 	return 0;
 }

@@ -103,13 +103,13 @@ static void b43_lpphy_op_radio_write(struct b43_wldev *dev, u16 reg, u16 value)
 static void b43_lpphy_op_software_rfkill(struct b43_wldev *dev,
 					 enum rfkill_state state)
 {
-	//TODO
+	/* TODO */
 }
 
 static int b43_lpphy_op_switch_channel(struct b43_wldev *dev,
 				       unsigned int new_channel)
 {
-	//TODO
+	/* TODO */
 	return 0;
 }
 

@@ -120,18 +120,18 @@ static unsigned int b43_lpphy_op_get_default_chan(struct b43_wldev *dev)
 
 static void b43_lpphy_op_set_rx_antenna(struct b43_wldev *dev, int antenna)
 {
-	//TODO
+	/* TODO */
 }
 
 static void b43_lpphy_op_adjust_txpower(struct b43_wldev *dev)
 {
-	//TODO
+	/* TODO */
 }
 
 static enum b43_txpwr_result b43_lpphy_op_recalc_txpower(struct b43_wldev *dev,
 							 bool ignore_tssi)
 {
-	//TODO
+	/* TODO */
 	return B43_TXPWR_RES_DONE;
 }
drivers/net/wireless/b43/phy_n.c +35 -28

142 lines not on this page · raw message

drivers/net/wireless/b43/pio.c +10 -10
index 4015912..578823c 100644
@@ -55,9 +55,9 @@ static u16 generate_cookie(struct b43_pio_txqueue *q,
 }
 
 static
-struct b43_pio_txqueue * parse_cookie(struct b43_wldev *dev,
-				      u16 cookie,
-				      struct b43_pio_txpacket **pack)
+struct b43_pio_txqueue *parse_cookie(struct b43_wldev *dev,
+				     u16 cookie,
+				     struct b43_pio_txpacket **pack)
 {
 	struct b43_pio *pio = &dev->pio;
 	struct b43_pio_txqueue *q = NULL;

@@ -134,8 +134,8 @@ static u16 pio_rxqueue_offset(struct b43_wldev *dev)
 	return 8;
 }
 
-static struct b43_pio_txqueue * b43_setup_pioqueue_tx(struct b43_wldev *dev,
-						      unsigned int index)
+static struct b43_pio_txqueue *b43_setup_pioqueue_tx(struct b43_wldev *dev,
+						     unsigned int index)
 {
 	struct b43_pio_txqueue *q;
 	struct b43_pio_txpacket *p;

@@ -153,7 +153,7 @@ static struct b43_pio_txqueue * b43_setup_pioqueue_tx(struct b43_wldev *dev,
 
 	q->free_packet_slots = B43_PIO_MAX_NR_TXPACKETS;
 	if (q->rev >= 8) {
-		q->buffer_size = 1920; //FIXME this constant is wrong.
+		q->buffer_size = 1920; /* FIXME this constant is wrong. */
 	} else {
 		q->buffer_size = b43_piotx_read16(q, B43_PIO_TXQBUFSIZE);
 		q->buffer_size -= 80;

@@ -171,8 +171,8 @@ static struct b43_pio_txqueue * b43_setup_pioqueue_tx(struct b43_wldev *dev,
 	return q;
 }
 
-static struct b43_pio_rxqueue * b43_setup_pioqueue_rx(struct b43_wldev *dev,
-						      unsigned int index)
+static struct b43_pio_rxqueue *b43_setup_pioqueue_rx(struct b43_wldev *dev,
+						     unsigned int index)
 {
 	struct b43_pio_rxqueue *q;
 

@@ -308,8 +308,8 @@ err_destroy_bk:
 }
 
 /* Static mapping of mac80211's queues (priorities) to b43 PIO queues. */
-static struct b43_pio_txqueue * select_queue_by_priority(struct b43_wldev *dev,
-							 u8 queue_prio)
+static struct b43_pio_txqueue *select_queue_by_priority(struct b43_wldev *dev,
+							u8 queue_prio)
 {
 	struct b43_pio_txqueue *q;
drivers/net/wireless/b43/rfkill.c +1 -1
index 7137537..afad423 100644
@@ -113,7 +113,7 @@ out_unlock:
 	return err;
 }
 
-char * b43_rfkill_led_name(struct b43_wldev *dev)
+char *b43_rfkill_led_name(struct b43_wldev *dev)
 {
 	struct b43_rfkill *rfk = &(dev->wl->rfkill);
drivers/net/wireless/b43/tables_nphy.c +2 -2
index 4e23363..6d8f25b 100644
@@ -38,10 +38,10 @@ struct b2055_inittab_entry {
 #define B2055_INITTAB_ENTRY_OK	0x01
 #define B2055_INITTAB_UPLOAD	0x02
 };
-#define UPLOAD		.flags = B2055_INITTAB_ENTRY_OK | B2055_INITTAB_UPLOAD
+#define UPLOAD		.flags = (B2055_INITTAB_ENTRY_OK | B2055_INITTAB_UPLOAD)
 #define NOUPLOAD	.flags = B2055_INITTAB_ENTRY_OK
 
-static const struct b2055_inittab_entry b2055_inittab [] = {
+static const struct b2055_inittab_entry b2055_inittab[] = {
   [B2055_SP_PINPD]		= { .ghz5 = 0x0080, .ghz2 = 0x0080, NOUPLOAD, },
   [B2055_C1_SP_RSSI]		= { .ghz5 = 0x0000, .ghz2 = 0x0000, NOUPLOAD, },
   [B2055_C1_SP_PDMISC]		= { .ghz5 = 0x0027, .ghz2 = 0x0027, NOUPLOAD, },
drivers/net/wireless/b43/wa.c +1 -1
index 0c0fb15..be15a29 100644
@@ -634,7 +634,7 @@ void b43_wa_all(struct b43_wldev *dev)
 		b43_wa_boards_a(dev);
 	} else if (phy->type == B43_PHYTYPE_G) {
 		switch (phy->rev) {
-		case 1://XXX review rev1
+		case 1:	/* XXX review rev1 */
 			b43_wa_crs_ed(dev);
 			b43_wa_crs_thr(dev);
 			b43_wa_crs_blank(dev);
drivers/net/wireless/b43/xmit.c +9 -7
index 2fabcf8..c8b5145 100644
@@ -488,7 +488,7 @@ static s8 b43_rssi_postprocess(struct b43_wldev *dev,
 	return (s8) tmp;
 }
 
-//TODO
+/* TODO */
 #if 0
 static s8 b43_rssinoise_postprocess(struct b43_wldev *dev, u8 in_rssi)
 {

@@ -496,7 +496,7 @@ static s8 b43_rssinoise_postprocess(struct b43_wldev *dev, u8 in_rssi)
 	s8 ret;
 
 	if (phy->type == B43_PHYTYPE_A) {
-		//TODO: Incomplete specs.
+		/* TODO: Incomplete specs. */
 		ret = 0;
 	} else
 		ret = b43_rssi_postprocess(dev, in_rssi, 0, 1, 1);

@@ -579,10 +579,12 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
 	/* Link quality statistics */
 	status.noise = dev->stats.link_noise;
 	if ((chanstat & B43_RX_CHAN_PHYTYPE) == B43_PHYTYPE_N) {
-//		s8 rssi = max(rxhdr->power0, rxhdr->power1);
-		//TODO: Find out what the rssi value is (dBm or percentage?)
-		//      and also find out what the maximum possible value is.
-		//      Fill status.ssi and status.signal fields.
+/*		s8 rssi = max(rxhdr->power0, rxhdr->power1); */
+		/*
+		 * TODO: Find out what the rssi value is (dBm or percentage?)
+		 * 	and also find out what the maximum possible value is.
+		 * 	Fill status.ssi and status.signal fields.
+		 */
 	} else {
 		status.signal = b43_rssi_postprocess(dev, rxhdr->jssi,
 						  (phystat0 & B43_RX_PHYST0_OFDM),

@@ -670,7 +672,7 @@ void b43_handle_txstatus(struct b43_wldev *dev,
 	if (!status->acked)
 		dev->wl->ieee_stats.dot11ACKFailureCount++;
 	if (status->rts_count) {
-		if (status->rts_count == 0xF)	//FIXME
+		if (status->rts_count == 0xF)	/* FIXME */
 			dev->wl->ieee_stats.dot11RTSFailureCount++;
 		else
 			dev->wl->ieee_stats.dot11RTSSuccessCount++;
#1 lore raw
Quoted 4 lines · On Friday 17 October 2008 21:16:09 John Daiker wrote:

A few changes to reduce checkpatch.pl errors in the b43 driver. For the most part, I only fixed cosmetic things, and left the actual 'code flow' untouched (hopefully)!

Quoted 8 lines

Diff is against wireless-testing HEAD.

Signed-off-by John Daiker <[email protected]>

---

Looks good, except these:

drivers/net/wireless/b43/main.c generate_new: · line 1256

Quoted 3 lines
drivers/net/wireless/b43/main.c +2 -1
-		if (1 /*FIXME: the last PSpoll frame was sent successfully */ )
+		if (1) {
+			/*FIXME: the last PSpoll frame was sent successfully */

Makes it a lot less obvious what the comment is talking about. Please leave this untouched.

drivers/net/wireless/b43/phy_g.c static void b43_gphy_op_pwork_15sec(struct b43_wldev *dev) · line 3224

Quoted 6 lines
drivers/net/wireless/b43/phy_g.c +4 -2
-		if (!gphy->aci_enable && 1 /*TODO: not scanning? */ ) {
-			if (0 /*TODO: bunch of conditions */ ) {
+		if (!gphy->aci_enable && 1) {
+			/* TODO: not scanning? */
+			if (0) {
+				/*TODO: bunch of conditions */

Same here.

There are probably more of these. I didn't check the whole patch.

Review

Quoted 2 lines
(quoted diff) +1 -1
-	//TODO
+	/* TODO */

Well, that's only generating noise in git and results in merge conflicts and stuff. I do only use // style comments for temporary comments. IMO this is OK.

drivers/net/wireless/b43/rfkill.c out_unlock: · line 116

Quoted 2 lines
drivers/net/wireless/b43/rfkill.c +1 -1
-char * b43_rfkill_led_name(struct b43_wldev *dev)
+char *b43_rfkill_led_name(struct b43_wldev *dev)

Well, in my opinion it looks bad to glue the * to the function name. The pointer * is not related to the function name, but to the return value.

(Note that for variable definitions this is different and I agree that we should put the * in front of the variable name without spaces)

Well, but I'm not going to create a flamewar for this. If this is kernel coding style, feel free to change the code.

One additional thing I'd like you to do.
Do a b43 compile before and after applying the patch. Keep the b43.ko files for both runs and do an md5sum on them. Add the results to the commit log. The sums _must_ match. If they don't, please send the changes that change the actual binary code in seperate patches.

Signature · 1 line
Greetings Michael.
author
#2 lore raw

Michael,

Thanks for the feedback. Bob Copeland had mentioned on a different patch that I compare the binaries before and after a patch. I will be sure to do this on the patch rework. Further comments inline.

Quoted 17 lines · Michael Buesch
Quoted 11 lines · On Friday 17 October 2008 21:16:09 John Daiker wrote:

A few changes to reduce checkpatch.pl errors in the b43 driver. For the most part, I only fixed cosmetic things, and left the actual 'code flow' untouched (hopefully)!

Diff is against wireless-testing HEAD.

Signed-off-by John Daiker <[email protected]>

---

Looks good, except these:

Quoted 8 lines

drivers/net/wireless/b43/main.c generate_new: · line 1256

Quoted 4 lines
drivers/net/wireless/b43/main.c +2 -1
-		if (1 /*FIXME: the last PSpoll frame was sent successfully */ )
+		if (1) {
+			/*FIXME: the last PSpoll frame was sent successfully */

Makes it a lot less obvious what the comment is talking about. Please leave this untouched.

How about this:

- if (1 /*FIXME: the last PSpoll frame was sent successfully */ )
+ if (1) { /*FIXME: the last PSpoll frame was sent successfully */

or this?

- if (1 /*FIXME: the last PSpoll frame was sent successfully */ )
+ if (1) /*FIXME: the last PSpoll frame was sent successfully */ {

I'm just not a fan of putting the comment inside the if conditional.
Your argument about losing the precise meaning of the comment is understood, but I think there is a middleground here. I would hope that any 'if (1)' line would immediately raise the 'look for a comment' flag in someone's brain. :P

Quoted 13 lines

drivers/net/wireless/b43/phy_g.c static void b43_gphy_op_pwork_15sec(struct b43_wldev *dev) · line 3224

Quoted 7 lines
drivers/net/wireless/b43/phy_g.c +4 -2
-		if (!gphy->aci_enable && 1 /*TODO: not scanning? */ ) {
-			if (0 /*TODO: bunch of conditions */ ) {
+		if (!gphy->aci_enable && 1) {
+			/* TODO: not scanning? */
+			if (0) {
+				/*TODO: bunch of conditions */

Same here.

There are probably more of these. I didn't check the whole patch.

Quoted 8 lines

Review

Quoted 3 lines
(quoted diff) +1 -1
-	//TODO
+	/* TODO */

Well, that's only generating noise in git and results in merge conflicts and stuff. I do only use // style comments for temporary comments. IMO this is OK.

I'll strip these out of the next patch. I agree this is a weird rule, but checkpatch.pl did all the work. I'll put more brainpower behind the next patch, haha.

Quoted 8 lines

drivers/net/wireless/b43/rfkill.c out_unlock: · line 116

Quoted 4 lines
drivers/net/wireless/b43/rfkill.c +1 -1
-char * b43_rfkill_led_name(struct b43_wldev *dev)
+char *b43_rfkill_led_name(struct b43_wldev *dev)

Well, in my opinion it looks bad to glue the * to the function name. The pointer * is not related to the function name, but to the return value.

Quoted 6 lines

(Note that for variable definitions this is different and I agree that we should put the * in front of the variable name without spaces)

Well, but I'm not going to create a flamewar for this. If this is kernel coding style, feel free to change the code.

I'm certainly not trying to start a flamewar, either, so thanks for keeping the torches at bay! :) AFAIK, this is accepted (and preferred?) kernel style, so it will be included in the updated patch. At least until I see more vigorous pushback.

Quoted 8 lines

One additional thing I'd like you to do.
Do a b43 compile before and after applying the patch. Keep the b43.ko files for both runs and do an md5sum on them. Add the results to the commit log. The sums _must_ match. If they don't, please send the changes that change the actual binary code in seperate patches.

Will do this next time with updated patch, among other things.

Thanks,
JD

#3 lore raw

Review

Quoted 2 lines · On Friday 17 October 2008 23:36:36 John Daiker wrote:
(quoted diff) +1 -1
-		if (1 /*FIXME: the last PSpoll frame was sent successfully */ )
+		if (1) /*FIXME: the last PSpoll frame was sent successfully */ {

Well, go for this, if you're really forced to fix this. ;)

Quoted 9 lines
Quoted 8 lines

One additional thing I'd like you to do.
Do a b43 compile before and after applying the patch. Keep the b43.ko files for both runs and do an md5sum on them. Add the results to the commit log. The sums _must_ match. If they don't, please send the changes that change the actual binary code in seperate patches.

Will do this next time with updated patch, among other things.

Thanks. Also make sure to enable all kconfig options. (also remove the BROKEN dependencies that some options have, if you are changing files related to these options.).

Signature · 1 line
Greetings Michael.
author
#4 lore raw
Quoted 17 lines · Michael Buesch

Review

Quoted 3 lines · On Friday 17 October 2008 23:36:36 John Daiker wrote:
(quoted diff) +1 -1
-		if (1 /*FIXME: the last PSpoll frame was sent successfully */ )
+		if (1) /*FIXME: the last PSpoll frame was sent successfully */ {

Well, go for this, if you're really forced to fix this. ;)

Quoted 8 lines
Quoted 8 lines

One additional thing I'd like you to do.
Do a b43 compile before and after applying the patch. Keep the b43.ko files for both runs and do an md5sum on them. Add the results to the commit log. The sums _must_ match. If they don't, please send the changes that change the actual binary code in seperate patches.

Quoted 6 lines
Quoted 2 lines

Will do this next time with updated patch, among other things.

Thanks. Also make sure to enable all kconfig options. (also remove the BROKEN dependencies that some options have, if you are changing files related to these options.)

I'm assuming you're debugging and forcing PIO over DMA?
For the BROKEN things, can I just remove 'BROKEN' from the Kconfig depends line?

#5 lore raw
Quoted 21 lines · On Friday 17 October 2008 23:58:28 John Daiker wrote:
Quoted 20 lines · Michael Buesch

Review

Quoted 3 lines · On Friday 17 October 2008 23:36:36 John Daiker wrote:
(quoted diff) +1 -1
-		if (1 /*FIXME: the last PSpoll frame was sent successfully */ )
+		if (1) /*FIXME: the last PSpoll frame was sent successfully */ {

Well, go for this, if you're really forced to fix this. ;)

Quoted 10 lines
Quoted 8 lines

One additional thing I'd like you to do.
Do a b43 compile before and after applying the patch. Keep the b43.ko files for both runs and do an md5sum on them. Add the results to the commit log. The sums _must_ match. If they don't, please send the changes that change the actual binary code in seperate patches.

Will do this next time with updated patch, among other things.

Quoted 6 lines
Quoted 3 lines

Thanks. Also make sure to enable all kconfig options. (also remove the BROKEN dependencies that some options have, if you are changing files related to these options.)

I'm assuming you're debugging and forcing PIO over DMA? For the BROKEN things, can I just remove 'BROKEN' from the Kconfig depends line?

Yeah, just enable all that stuff to make sure the binary code isn't changed. You can just temporarly remove the "depends on BROKEN" and enable the option. It will compile fine. It's just runtime broken.

Signature · 1 line
Greetings Michael.