staging: rdma: hfi1: Drop wrapper functions

Drop wrapper functions add_rcvctrl() and clear_rcvctrl() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index a9c6abf..23cacbf 100644
@@ -3176,16 +3176,6 @@ static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear)
 	spin_unlock_irqrestore(&dd->rcvctrl_lock, flags);
 }

drivers/staging/rdma/hfi1/chip.c +10 -10

 /*
  * Called from all interrupt handlers to start handling an SPC freeze.
  */

@@ -3268,7 +3258,7 @@ static void rxe_freeze(struct hfi1_devdata *dd)
 	int i;
 
 	/* disable port */
-	clear_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
+	adjust_rcvctrl(dd, 0, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
 
 	/* disable all receive contexts */
 	for (i = 0; i < dd->num_rcv_contexts; i++)

@@ -3290,7 +3280,7 @@ static void rxe_kernel_unfreeze(struct hfi1_devdata *dd)
 		hfi1_rcvctrl(dd, HFI1_RCVCTRL_CTXT_ENB, i);
 
 	/* enable port */
-	add_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
+	adjust_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK, 0);
 }
 
 /*

@@ -3441,7 +3431,7 @@ void handle_link_down(struct work_struct *work)
 	reset_neighbor_info(ppd);
 
 	/* disable the port */
-	clear_rcvctrl(ppd->dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
+	adjust_rcvctrl(ppd->dd, 0, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
 
 	/* If there is no cable attached, turn the DC off. Otherwise,
 	 * start the link bring up. */

@@ -5771,7 +5761,7 @@ int bringup_serdes(struct hfi1_pportdata *ppd)
 	int ret;
 
 	if (HFI1_CAP_IS_KSET(EXTENDED_PSN))
-		add_rcvctrl(dd, RCV_CTRL_RCV_EXTENDED_PSN_ENABLE_SMASK);
+		adjust_rcvctrl(dd, RCV_CTRL_RCV_EXTENDED_PSN_ENABLE_SMASK, 0);
 
 	guid = ppd->guid;
 	if (!guid) {

@@ -5813,7 +5803,7 @@ void hfi1_quiet_serdes(struct hfi1_pportdata *ppd)
 	set_link_state(ppd, HLS_DN_OFFLINE);
 
 	/* disable the port */
-	clear_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
+	adjust_rcvctrl(dd, 0, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
 }
 
 static inline int init_cpu_counters(struct hfi1_devdata *dd)

@@ -6461,7 +6451,7 @@ int set_link_state(struct hfi1_pportdata *ppd, u32 state)
 					OPA_LINKINIT_REASON_LINKUP;
 
 			/* enable the port */
-			add_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
+			adjust_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK, 0);
 
 			handle_linkup_change(dd, 1);
 		}

@@ -9193,7 +9183,7 @@ static void set_partition_keys(struct hfi1_pportdata *ppd)
 	}
 
 	/* Always enable HW pkeys check when pkeys table is set */
-	add_rcvctrl(dd, RCV_CTRL_RCV_PARTITION_KEY_ENABLE_SMASK);
+	adjust_rcvctrl(dd, RCV_CTRL_RCV_PARTITION_KEY_ENABLE_SMASK, 0);
 }
 
 /*

@@ -9945,8 +9935,8 @@ static void init_qpmap_table(struct hfi1_devdata *dd,
 	if (i % 8)
 		write_csr(dd, regno, reg);
 
-	add_rcvctrl(dd, RCV_CTRL_RCV_QP_MAP_ENABLE_SMASK
-			| RCV_CTRL_RCV_BYPASS_ENABLE_SMASK);
+	adjust_rcvctrl(dd, RCV_CTRL_RCV_QP_MAP_ENABLE_SMASK
+			| RCV_CTRL_RCV_BYPASS_ENABLE_SMASK, 0);
 }
 
 /**

@@ -10041,7 +10031,7 @@ static void init_qos(struct hfi1_devdata *dd, u32 first_ctxt)
 		LRH_SC_MASK << RCV_RSM_MATCH_MASK2_SHIFT |
 		LRH_SC_VALUE << RCV_RSM_MATCH_VALUE2_SHIFT);
 	/* Enable RSM */
-	add_rcvctrl(dd, RCV_CTRL_RCV_RSM_ENABLE_SMASK);
+	adjust_rcvctrl(dd, RCV_CTRL_RCV_RSM_ENABLE_SMASK, 0);
 	kfree(rsmmap);
 	/* map everything else (non-VL15) to context 0 */
 	init_qpmap_table(

Signature · 8 lines

1.9.1

-- 
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/d87be280ac9dc0fe8fc50e25afabf579dcd7b98b.1446366036.git.amitoj1606%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Drop wrapper functions sdma_start_err_halt_wait() and sdma_start_sw_clean_up() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/sdma.c | 40 ++++++++++++++--------------------------
 1 file changed, 14 insertions(+), 26 deletions(-)

drivers/staging/rdma/hfi1/sdma.c +0 -7

index 63ab721..4e44ac7 100644
@@ -230,7 +230,6 @@ static void sdma_hw_clean_up_task(unsigned long);
 static void sdma_put(struct sdma_state *);
 static void sdma_set_state(struct sdma_engine *, enum sdma_states);
 static void sdma_start_hw_clean_up(struct sdma_engine *);
-static void sdma_start_sw_clean_up(struct sdma_engine *);
 static void sdma_sw_clean_up_task(unsigned long);
 static void sdma_sendctrl(struct sdma_engine *, unsigned);
 static void init_sdma_regs(struct sdma_engine *, u32, uint);

@@ -454,12 +453,6 @@ static void sdma_err_halt_wait(struct work_struct *work)
 	sdma_process_event(sde, sdma_event_e15_hw_halt_done);
 }
 
-static void sdma_start_err_halt_wait(struct sdma_engine *sde)
-{
-	schedule_work(&sde->err_halt_worker);
-}
-
-
 static void sdma_err_progress_check_schedule(struct sdma_engine *sde)
 {
 	if (!is_bx(sde->dd) && HFI1_CAP_IS_KSET(SDMA_AHG)) {

drivers/staging/rdma/hfi1/sdma.c +14 -14

 static void sdma_set_state(struct sdma_engine *sde,
 	enum sdma_states next_state)
 {

@@ -2291,7 +2279,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		case sdma_event_e50_hw_cleaned:
 			break;
 		case sdma_event_e60_hw_halted:
-			sdma_start_err_halt_wait(sde);
+			schedule_work(&sde->err_halt_worker);
 			break;
 		case sdma_event_e70_go_idle:
 			ss->go_s99_running = 0;

@@ -2372,7 +2360,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 			break;
 		case sdma_event_e60_hw_halted:
 			sdma_set_state(sde, sdma_state_s50_hw_halt_wait);
-			sdma_start_err_halt_wait(sde);
+			schedule_work(&sde->err_halt_worker);
 			break;
 		case sdma_event_e70_go_idle:
 			break;

@@ -2435,7 +2423,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		switch (event) {
 		case sdma_event_e00_go_hw_down:
 			sdma_set_state(sde, sdma_state_s00_hw_down);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e10_go_hw_start:
 			break;

@@ -2477,13 +2465,13 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		switch (event) {
 		case sdma_event_e00_go_hw_down:
 			sdma_set_state(sde, sdma_state_s00_hw_down);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e10_go_hw_start:
 			break;
 		case sdma_event_e15_hw_halt_done:
 			sdma_set_state(sde, sdma_state_s30_sw_clean_up_wait);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e25_hw_clean_up_done:
 			break;

@@ -2495,7 +2483,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		case sdma_event_e50_hw_cleaned:
 			break;
 		case sdma_event_e60_hw_halted:
-			sdma_start_err_halt_wait(sde);
+			schedule_work(&sde->err_halt_worker);
 			break;
 		case sdma_event_e70_go_idle:
 			ss->go_s99_running = 0;

@@ -2518,13 +2506,13 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		switch (event) {
 		case sdma_event_e00_go_hw_down:
 			sdma_set_state(sde, sdma_state_s00_hw_down);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e10_go_hw_start:
 			break;
 		case sdma_event_e15_hw_halt_done:
 			sdma_set_state(sde, sdma_state_s30_sw_clean_up_wait);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e25_hw_clean_up_done:
 			break;

@@ -2536,7 +2524,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		case sdma_event_e50_hw_cleaned:
 			break;
 		case sdma_event_e60_hw_halted:
-			sdma_start_err_halt_wait(sde);
+			schedule_work(&sde->err_halt_worker);
 			break;
 		case sdma_event_e70_go_idle:
 			ss->go_s99_running = 0;

@@ -2558,7 +2546,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		switch (event) {
 		case sdma_event_e00_go_hw_down:
 			sdma_set_state(sde, sdma_state_s00_hw_down);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e10_go_hw_start:
 			break;

@@ -2582,7 +2570,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 			break;
 		case sdma_event_e81_hw_frozen:
 			sdma_set_state(sde, sdma_state_s82_freeze_sw_clean);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e82_hw_unfreeze:
 			break;

@@ -2597,7 +2585,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		switch (event) {
 		case sdma_event_e00_go_hw_down:
 			sdma_set_state(sde, sdma_state_s00_hw_down);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e10_go_hw_start:
 			break;

@@ -2641,7 +2629,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		switch (event) {
 		case sdma_event_e00_go_hw_down:
 			sdma_set_state(sde, sdma_state_s00_hw_down);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e10_go_hw_start:
 			break;

@@ -2664,7 +2652,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 			* progress check
 			*/
 			sdma_set_state(sde, sdma_state_s50_hw_halt_wait);
-			sdma_start_err_halt_wait(sde);
+			schedule_work(&sde->err_halt_worker);
 			break;
 		case sdma_event_e70_go_idle:
 			sdma_set_state(sde, sdma_state_s60_idle_halt_wait);

Signature · 8 lines

1.9.1

-- 
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/12c017e4966c52a7f15487e1bd2eb6b907bee4d3.1446366036.git.amitoj1606%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Drop wrapper function remap_receive_available_interrupt() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index beac326d..a9c6abf 100644

drivers/staging/rdma/hfi1/chip.c +1 -1

 static int request_intx_irq(struct hfi1_devdata *dd)
 {
 	int ret;

@@ -8890,7 +8884,7 @@ static int request_msix_irqs(struct hfi1_devdata *dd)
 			snprintf(me->name, sizeof(me->name),
 				DRIVER_NAME"_%d kctxt%d", dd->unit, idx);
 			err_info = "receive context";
-			remap_receive_available_interrupt(dd, idx, i);
+			remap_intr(dd, IS_RCVAVAIL_START + idx, i);
 		} else {
 			/* not in our expected range - complain, then
 			   ignore it */

Signature · 8 lines

1.9.1

-- 
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/23ac38a01d5092053da227aaa0741b270125ceeb.1446366036.git.amitoj1606%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Drop set_sbus_fast_mode() wrapper function that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.h     |  1 -
 drivers/staging/rdma/hfi1/firmware.c | 15 ++++++---------
 drivers/staging/rdma/hfi1/pcie.c     |  3 ++-
 3 files changed, 8 insertions(+), 11 deletions(-)

drivers/staging/rdma/hfi1/chip.h +0 -1

index f89a432..092fc9d 100644
@@ -619,7 +619,6 @@ void sbus_request(struct hfi1_devdata *dd,
 		  u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
 int sbus_request_slow(struct hfi1_devdata *dd,
 		      u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
-void set_sbus_fast_mode(struct hfi1_devdata *dd);
 void clear_sbus_fast_mode(struct hfi1_devdata *dd);
 int hfi1_firmware_init(struct hfi1_devdata *dd);
 int load_pcie_firmware(struct hfi1_devdata *dd);

drivers/staging/rdma/hfi1/firmware.c +2 -1

index 5c2f2ed..0ce9a16 100644
@@ -987,7 +987,8 @@ void fabric_serdes_reset(struct hfi1_devdata *dd)
 	ra = fabric_serdes_broadcast[dd->hfi1_id];
 
 	acquire_hw_mutex(dd);
-	set_sbus_fast_mode(dd);
+	write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
+				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
 	/* place SerDes in reset and disable SPICO */
 	sbus_request(dd, ra, 0x07, WRITE_SBUS_RECEIVER, 0x00000011);
 	/* wait 100 refclk cycles @ 156.25MHz => 640ns */

drivers/staging/rdma/hfi1/firmware.c +4 -2

 {
 	u64 reg, count = 0;

@@ -1244,7 +1239,8 @@ int load_firmware(struct hfi1_devdata *dd)
 		if (ret)
 			return ret;
 
-		set_sbus_fast_mode(dd);
+		write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
+				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
 
 		/*
 		 * The SBus contains part of the fabric firmware and so must

@@ -1583,7 +1579,8 @@ int load_pcie_firmware(struct hfi1_devdata *dd)
 	int ret = 0;
 
 	/* both firmware loads below use the SBus */
-	set_sbus_fast_mode(dd);
+	write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
+				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
 
 	if (fw_sbus_load) {
 		turn_off_spicos(dd, SPICO_SBUS);

drivers/staging/rdma/hfi1/pcie.c +2 -1

index e6773b3..10596b1 100644
@@ -770,7 +770,8 @@ static void pcie_post_steps(struct hfi1_devdata *dd)
 {
 	int i;
 
-	set_sbus_fast_mode(dd);
+	write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
+				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
 	/*
 	 * Write to the PCIe PCSes to set the G3_LOCKED_NEXT bits to 1.
 	 * This avoids a spurious framing error that can otherwise be

Signature · 8 lines

1.9.1

-- 
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/5cd4f403201b3e1c6b0f44429a5203b3447e2d27.1446366036.git.amitoj1606%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Drop wrapper function hfi1_nomsix() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 2 +-
 drivers/staging/rdma/hfi1/hfi.h  | 1 -
 drivers/staging/rdma/hfi1/pcie.c | 8 --------
 3 files changed, 1 insertion(+), 10 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index aa58e59..beac326d 100644

drivers/staging/rdma/hfi1/chip.c +0 -0

 	} else {
 		/* INTx */
 		disable_intx(dd->pcidev);

drivers/staging/rdma/hfi1/hfi.h +0 -1

index 8ca171b..1f97124 100644
@@ -1600,7 +1600,6 @@ void hfi1_pcie_flr(struct hfi1_devdata *);
 int pcie_speeds(struct hfi1_devdata *);
 void request_msix(struct hfi1_devdata *, u32 *, struct hfi1_msix_entry *);
 void hfi1_enable_intx(struct pci_dev *);
-void hfi1_nomsix(struct hfi1_devdata *);
 void restore_pci_variables(struct hfi1_devdata *dd);
 int do_pcie_gen3_transition(struct hfi1_devdata *dd);
 int parse_platform_config(struct hfi1_devdata *dd);

drivers/staging/rdma/hfi1/pcie.c +0 -8

index ac5653c..e6773b3 100644
@@ -426,14 +426,6 @@ void request_msix(struct hfi1_devdata *dd, u32 *nent,
 	tune_pcie_caps(dd);
 }
 
-/*
- * Disable MSI-X.
- */
-void hfi1_nomsix(struct hfi1_devdata *dd)
-{
-	pci_disable_msix(dd->pcidev);
-}
-
 void hfi1_enable_intx(struct pci_dev *pdev)
 {
 	/* first, turn on INTx */

Signature · 8 lines

1.9.1

-- 
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/e9e769aacb2c1db52a7a1eb523fa2d73a8f6d09c.1446366036.git.amitoj1606%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Quoted 13 lines · Amitoj Kaur Chawla

Drop wrapper function remap_receive_available_interrupt() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index beac326d..a9c6abf 100644

drivers/staging/rdma/hfi1/chip.c static void remap_sdma_interrupts(struct hfi1_devdata *dd, · line 8756

Quoted 8 lines

drivers/staging/rdma/hfi1/chip.c +0 -5

@@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
              msix_intr);
 }
 
-static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
-                                           int rx, int msix_intr)
-{
-     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
-}

remap_intr is not a standard kernel function. But perhaps one could argue that the constant name gives the same information as the function name, so the function is not needed.

But this function is the neighbor of another very similar function that remaps three interrupts at once. The two functions are also used in a very parallel way, at the end of successive if branches. It seems strange to inline one function and not the other.

julia

Quoted 23 lines

drivers/staging/rdma/hfi1/chip.c +1 -2

 static int request_intx_irq(struct hfi1_devdata *dd)
 {
      int ret;

@@ -8890,7 +8884,7 @@ static int request_msix_irqs(struct hfi1_devdata *dd)
                      snprintf(me->name, sizeof(me->name),
                              DRIVER_NAME"_%d kctxt%d", dd->unit, idx);
                      err_info = "receive context";
-                     remap_receive_available_interrupt(dd, idx, i);
+                     remap_intr(dd, IS_RCVAVAIL_START + idx, i);
              } else {
                      /* not in our expected range - complain, then
                         ignore it */
--

1.9.1

--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/23ac38a01d5092053da227aaa0741b270125ceeb.1446366036.git.amitoj1606%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Quoted 40 lines · Amitoj Kaur Chawla

Drop set_sbus_fast_mode() wrapper function that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.h     |  1 -
 drivers/staging/rdma/hfi1/firmware.c | 15 ++++++---------
 drivers/staging/rdma/hfi1/pcie.c     |  3 ++-
 3 files changed, 8 insertions(+), 11 deletions(-)

drivers/staging/rdma/hfi1/chip.h +0 -1

index f89a432..092fc9d 100644
@@ -619,7 +619,6 @@ void sbus_request(struct hfi1_devdata *dd,
                u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
 int sbus_request_slow(struct hfi1_devdata *dd,
                    u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
-void set_sbus_fast_mode(struct hfi1_devdata *dd);
 void clear_sbus_fast_mode(struct hfi1_devdata *dd);
 int hfi1_firmware_init(struct hfi1_devdata *dd);
 int load_pcie_firmware(struct hfi1_devdata *dd);

drivers/staging/rdma/hfi1/firmware.c +2 -1

index 5c2f2ed..0ce9a16 100644
@@ -987,7 +987,8 @@ void fabric_serdes_reset(struct hfi1_devdata *dd)
      ra = fabric_serdes_broadcast[dd->hfi1_id];
 
      acquire_hw_mutex(dd);
-     set_sbus_fast_mode(dd);
+     write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
+                             ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
      /* place SerDes in reset and disable SPICO */
      sbus_request(dd, ra, 0x07, WRITE_SBUS_RECEIVER, 0x00000011);
      /* wait 100 refclk cycles @ 156.25MHz => 640ns */

@@ -1214,12 +1215,6 @@ void release_hw_mutex(struct hfi1_devdata *dd)
      write_csr(dd, ASIC_CFG_MUTEX, 0);
 }
 

drivers/staging/rdma/hfi1/firmware.c void release_hw_mutex(struct hfi1_devdata *dd) · line 1218

Quoted 7 lines

drivers/staging/rdma/hfi1/firmware.c +0 -6

-void set_sbus_fast_mode(struct hfi1_devdata *dd)
-{
-     write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
-                             ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
-}
-
 void clear_sbus_fast_mode(struct hfi1_devdata *dd)

set_sbus_fast_mode seems to be the pair of clear_sbus_fast_mode just below it, so it's not clear that it is a good idea to remove one and leave the other.

Quoted 42 lines · Julia Lawall

Quoted 40 lines · Amitoj Kaur Chawla

Drop set_sbus_fast_mode() wrapper function that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.h     |  1 -
 drivers/staging/rdma/hfi1/firmware.c | 15 ++++++---------
 drivers/staging/rdma/hfi1/pcie.c     |  3 ++-
 3 files changed, 8 insertions(+), 11 deletions(-)

drivers/staging/rdma/hfi1/chip.h +0 -1

index f89a432..092fc9d 100644
@@ -619,7 +619,6 @@ void sbus_request(struct hfi1_devdata *dd,
                u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
 int sbus_request_slow(struct hfi1_devdata *dd,
                    u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
-void set_sbus_fast_mode(struct hfi1_devdata *dd);
 void clear_sbus_fast_mode(struct hfi1_devdata *dd);
 int hfi1_firmware_init(struct hfi1_devdata *dd);
 int load_pcie_firmware(struct hfi1_devdata *dd);

drivers/staging/rdma/hfi1/firmware.c +2 -1

index 5c2f2ed..0ce9a16 100644
@@ -987,7 +987,8 @@ void fabric_serdes_reset(struct hfi1_devdata *dd)
      ra = fabric_serdes_broadcast[dd->hfi1_id];
 
      acquire_hw_mutex(dd);
-     set_sbus_fast_mode(dd);
+     write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
+                             ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
      /* place SerDes in reset and disable SPICO */
      sbus_request(dd, ra, 0x07, WRITE_SBUS_RECEIVER, 0x00000011);
      /* wait 100 refclk cycles @ 156.25MHz => 640ns */

@@ -1214,12 +1215,6 @@ void release_hw_mutex(struct hfi1_devdata *dd)
      write_csr(dd, ASIC_CFG_MUTEX, 0);
 }
 

Quoted 12 lines

drivers/staging/rdma/hfi1/firmware.c void release_hw_mutex(struct hfi1_devdata *dd) · line 1218

Quoted 7 lines

drivers/staging/rdma/hfi1/firmware.c +0 -6

-void set_sbus_fast_mode(struct hfi1_devdata *dd)
-{
-     write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
-                             ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
-}
-
 void clear_sbus_fast_mode(struct hfi1_devdata *dd)

set_sbus_fast_mode seems to be the pair of clear_sbus_fast_mode just below it, so it's not clear that it is a good idea to remove one and leave the other.

clear_sbus_fast_mode() is not a wrapper function. So I didn't remove it. But if you think one change without the other is pointless then I can drop this.

Quoted 14 lines · Amitoj Kaur Chawla

Drop wrapper functions add_rcvctrl() and clear_rcvctrl() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index a9c6abf..23cacbf 100644
@@ -3176,16 +3176,6 @@ static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear)
      spin_unlock_irqrestore(&dd->rcvctrl_lock, flags);
 }

drivers/staging/rdma/hfi1/chip.c static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear) · line 3187

Quoted 10 lines

drivers/staging/rdma/hfi1/chip.c +0 -9

 
-static inline void add_rcvctrl(struct hfi1_devdata *dd, u64 add)
-{
-     adjust_rcvctrl(dd, add, 0);
-}
-
-static inline void clear_rcvctrl(struct hfi1_devdata *dd, u64 clear)
-{
-     adjust_rcvctrl(dd, 0, clear);
-}

Is it really an improvement? adjust_rcvctrl is not a standard kernel function. In the original code, one can undestand what add_rcvctrl and clear_rcvctrl do based on their names. In the modified code, you have to know what is the significance of the different argument positions of adjust_rcvctrl, so you have to go look at the definition. So this makes more work for someone who wants to understand the code.

Wrapping standard kernel functions is not good. And wrapping when the arguments don't change and the name is almost the same is probably not good either. But here the wrapping would seem to add some value.

julia

Quoted 56 lines · Amitoj Kaur Chawla

Quoted 54 lines · Julia Lawall

Quoted 37 lines · Amitoj Kaur Chawla

Drop set_sbus_fast_mode() wrapper function that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.h     |  1 -
 drivers/staging/rdma/hfi1/firmware.c | 15 ++++++---------
 drivers/staging/rdma/hfi1/pcie.c     |  3 ++-
 3 files changed, 8 insertions(+), 11 deletions(-)

drivers/staging/rdma/hfi1/chip.h +0 -1

index f89a432..092fc9d 100644
@@ -619,7 +619,6 @@ void sbus_request(struct hfi1_devdata *dd,
                u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
 int sbus_request_slow(struct hfi1_devdata *dd,
                    u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
-void set_sbus_fast_mode(struct hfi1_devdata *dd);
 void clear_sbus_fast_mode(struct hfi1_devdata *dd);
 int hfi1_firmware_init(struct hfi1_devdata *dd);
 int load_pcie_firmware(struct hfi1_devdata *dd);

drivers/staging/rdma/hfi1/firmware.c +2 -1

index 5c2f2ed..0ce9a16 100644
@@ -987,7 +987,8 @@ void fabric_serdes_reset(struct hfi1_devdata *dd)
      ra = fabric_serdes_broadcast[dd->hfi1_id];
 
      acquire_hw_mutex(dd);
-     set_sbus_fast_mode(dd);
+     write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
+                             ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
      /* place SerDes in reset and disable SPICO */
      sbus_request(dd, ra, 0x07, WRITE_SBUS_RECEIVER, 0x00000011);
      /* wait 100 refclk cycles @ 156.25MHz => 640ns */

drivers/staging/rdma/hfi1/firmware.c void release_hw_mutex(struct hfi1_devdata *dd) · line 1218

Quoted 10 lines

drivers/staging/rdma/hfi1/firmware.c +0 -6

@@ -1214,12 +1215,6 @@ void release_hw_mutex(struct hfi1_devdata *dd)
      write_csr(dd, ASIC_CFG_MUTEX, 0);
 }
 
-void set_sbus_fast_mode(struct hfi1_devdata *dd)
-{
-     write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
-                             ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
-}
-
 void clear_sbus_fast_mode(struct hfi1_devdata *dd)

set_sbus_fast_mode seems to be the pair of clear_sbus_fast_mode just below it, so it's not clear that it is a good idea to remove one and leave the other.

Quoted 3 lines

clear_sbus_fast_mode() is not a wrapper function. So I didn't remove it. But if you think one change without the other is pointless then I can drop this.

Pointless is not quite the right word. I think that it hurts readability to drop one without the other. So I think it would be better to drop it.

julia

Quoted 33 lines · Julia Lawall

Quoted 13 lines · Amitoj Kaur Chawla

Drop wrapper function remap_receive_available_interrupt() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index beac326d..a9c6abf 100644

drivers/staging/rdma/hfi1/chip.c static void remap_sdma_interrupts(struct hfi1_devdata *dd, · line 8756

Quoted 8 lines

drivers/staging/rdma/hfi1/chip.c +0 -5

@@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
              msix_intr);
 }
 
-static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
-                                           int rx, int msix_intr)
-{
-     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
-}

remap_intr is not a standard kernel function. But perhaps one could argue that the constant name gives the same information as the function name, so the function is not needed.

But this function is the neighbor of another very similar function that remaps three interrupts at once. The two functions are also used in a very parallel way, at the end of successive if branches. It seems strange to inline one function and not the other.

Quoted 1 line

julia

In this case, the function names are very similar and the wrapper just add the macro IS_RCVAVAIL_START to the second argument, so the change made sense to me.

Quoted 23 lines

Quoted 23 lines

drivers/staging/rdma/hfi1/chip.c +1 -2

 static int request_intx_irq(struct hfi1_devdata *dd)
 {
      int ret;

@@ -8890,7 +8884,7 @@ static int request_msix_irqs(struct hfi1_devdata *dd)
                      snprintf(me->name, sizeof(me->name),
                              DRIVER_NAME"_%d kctxt%d", dd->unit, idx);
                      err_info = "receive context";
-                     remap_receive_available_interrupt(dd, idx, i);
+                     remap_intr(dd, IS_RCVAVAIL_START + idx, i);
              } else {
                      /* not in our expected range - complain, then
                         ignore it */
--

1.9.1

--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/23ac38a01d5092053da227aaa0741b270125ceeb.1446366036.git.amitoj1606%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Signature · 1 line

Amitoj

Quoted 38 lines · Julia Lawall

Quoted 14 lines · Amitoj Kaur Chawla

Drop wrapper functions add_rcvctrl() and clear_rcvctrl() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index a9c6abf..23cacbf 100644
@@ -3176,16 +3176,6 @@ static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear)
      spin_unlock_irqrestore(&dd->rcvctrl_lock, flags);
 }

drivers/staging/rdma/hfi1/chip.c static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear) · line 3187

Quoted 10 lines

drivers/staging/rdma/hfi1/chip.c +0 -9

 
-static inline void add_rcvctrl(struct hfi1_devdata *dd, u64 add)
-{
-     adjust_rcvctrl(dd, add, 0);
-}
-
-static inline void clear_rcvctrl(struct hfi1_devdata *dd, u64 clear)
-{
-     adjust_rcvctrl(dd, 0, clear);
-}

Is it really an improvement? adjust_rcvctrl is not a standard kernel function. In the original code, one can undestand what add_rcvctrl and clear_rcvctrl do based on their names. In the modified code, you have to know what is the significance of the different argument positions of adjust_rcvctrl, so you have to go look at the definition. So this makes more work for someone who wants to understand the code.

Wrapping standard kernel functions is not good. And wrapping when the arguments don't change and the name is almost the same is probably not good either. But here the wrapping would seem to add some value.

Quoted 1 line

julia

Since both were using the same function and one would have to look at adjust_rcvctrl to understand what add_rcvctrl and clear_rcvctrl are doing, I thought this would work by removing that extra 2 functions.

Quoted 36 lines · Amitoj Kaur Chawla

Quoted 34 lines · Julia Lawall

Quoted 13 lines · Amitoj Kaur Chawla

Drop wrapper function remap_receive_available_interrupt() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index beac326d..a9c6abf 100644

drivers/staging/rdma/hfi1/chip.c static void remap_sdma_interrupts(struct hfi1_devdata *dd, · line 8756

Quoted 8 lines

drivers/staging/rdma/hfi1/chip.c +0 -5

@@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
              msix_intr);
 }
 
-static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
-                                           int rx, int msix_intr)
-{
-     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
-}

remap_intr is not a standard kernel function. But perhaps one could argue that the constant name gives the same information as the function name, so the function is not needed.

But this function is the neighbor of another very similar function that remaps three interrupts at once. The two functions are also used in a very parallel way, at the end of successive if branches. It seems strange to inline one function and not the other.

julia

Quoted 3 lines

In this case, the function names are very similar and the wrapper just add the macro IS_RCVAVAIL_START to the second argument, so the change made sense to me.

Personally, I would find the lack of parallelism confusing. But one could have an opinion either way on this one.

julia

Quoted 38 lines · Julia Lawall

Quoted 14 lines · Amitoj Kaur Chawla

Drop wrapper functions add_rcvctrl() and clear_rcvctrl() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index a9c6abf..23cacbf 100644
@@ -3176,16 +3176,6 @@ static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear)
      spin_unlock_irqrestore(&dd->rcvctrl_lock, flags);
 }

drivers/staging/rdma/hfi1/chip.c static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear) · line 3187

Quoted 10 lines

drivers/staging/rdma/hfi1/chip.c +0 -9

 
-static inline void add_rcvctrl(struct hfi1_devdata *dd, u64 add)
-{
-     adjust_rcvctrl(dd, add, 0);
-}
-
-static inline void clear_rcvctrl(struct hfi1_devdata *dd, u64 clear)
-{
-     adjust_rcvctrl(dd, 0, clear);
-}

Is it really an improvement? adjust_rcvctrl is not a standard kernel function. In the original code, one can undestand what add_rcvctrl and clear_rcvctrl do based on their names. In the modified code, you have to know what is the significance of the different argument positions of adjust_rcvctrl, so you have to go look at the definition. So this makes more work for someone who wants to understand the code.

Wrapping standard kernel functions is not good. And wrapping when the arguments don't change and the name is almost the same is probably not good either. But here the wrapping would seem to add some value.

Quoted 1 line

julia

Since both were using the same function and one would have to look at adjust_rcvctrl to understand what add_rcvctrl and clear_rcvctrl are doing, I thought this would work by removing that extra 2 functions.

Quoted 39 lines · Amitoj Kaur Chawla

Quoted 38 lines · Julia Lawall

Quoted 14 lines · Amitoj Kaur Chawla

Drop wrapper functions add_rcvctrl() and clear_rcvctrl() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index a9c6abf..23cacbf 100644
@@ -3176,16 +3176,6 @@ static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear)
      spin_unlock_irqrestore(&dd->rcvctrl_lock, flags);
 }

drivers/staging/rdma/hfi1/chip.c static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear) · line 3187

Quoted 10 lines

drivers/staging/rdma/hfi1/chip.c +0 -9

 
-static inline void add_rcvctrl(struct hfi1_devdata *dd, u64 add)
-{
-     adjust_rcvctrl(dd, add, 0);
-}
-
-static inline void clear_rcvctrl(struct hfi1_devdata *dd, u64 clear)
-{
-     adjust_rcvctrl(dd, 0, clear);
-}

Is it really an improvement? adjust_rcvctrl is not a standard kernel function. In the original code, one can undestand what add_rcvctrl and clear_rcvctrl do based on their names. In the modified code, you have to know what is the significance of the different argument positions of adjust_rcvctrl, so you have to go look at the definition. So this makes more work for someone who wants to understand the code.

Wrapping standard kernel functions is not good. And wrapping when the arguments don't change and the name is almost the same is probably not good either. But here the wrapping would seem to add some value.

Quoted 5 lines

Quoted 1 line

julia

Since both were using the same function and one would have to look at adjust_rcvctrl to understand what add_rcvctrl and clear_rcvctrl are doing, I thought this would work by removing that extra 2 functions.

In any case, all of this discussion shows that the commit message needs to be better. Just saying that something is a one-line function is not enough justification to remove it. There needs to be an explanation about why the code will be improved as a result.

julia

Quoted 42 lines · Julia Lawall

Quoted 39 lines · Amitoj Kaur Chawla

Quoted 28 lines · Julia Lawall

Quoted 13 lines · Amitoj Kaur Chawla

Drop wrapper function remap_receive_available_interrupt() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index beac326d..a9c6abf 100644

drivers/staging/rdma/hfi1/chip.c static void remap_sdma_interrupts(struct hfi1_devdata *dd, · line 8756

Quoted 8 lines

drivers/staging/rdma/hfi1/chip.c +0 -5

@@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
              msix_intr);
 }
 
-static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
-                                           int rx, int msix_intr)
-{
-     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
-}

remap_intr is not a standard kernel function. But perhaps one could argue that the constant name gives the same information as the function name, so the function is not needed.

Quoted 6 lines

But this function is the neighbor of another very similar function that remaps three interrupts at once. The two functions are also used in a very parallel way, at the end of successive if branches. It seems strange to inline one function and not the other.

julia

In this case, the function names are very similar and the wrapper just add the macro IS_RCVAVAIL_START to the second argument, so the change made sense to me.

Quoted 5 lines

Personally, I would find the lack of parallelism confusing. But one could have an opinion either way on this one.

julia

So I should leave this as is or drop it? Confused.

Quoted 43 lines · Amitoj Kaur Chawla

Quoted 42 lines · Julia Lawall

Quoted 39 lines · Amitoj Kaur Chawla

Quoted 28 lines · Julia Lawall

Quoted 13 lines · Amitoj Kaur Chawla

Drop wrapper function remap_receive_available_interrupt() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index beac326d..a9c6abf 100644

drivers/staging/rdma/hfi1/chip.c static void remap_sdma_interrupts(struct hfi1_devdata *dd, · line 8756

Quoted 8 lines

drivers/staging/rdma/hfi1/chip.c +0 -5

@@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
              msix_intr);
 }
 
-static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
-                                           int rx, int msix_intr)
-{
-     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
-}

remap_intr is not a standard kernel function. But perhaps one could argue that the constant name gives the same information as the function name, so the function is not needed.

Quoted 6 lines

But this function is the neighbor of another very similar function that remaps three interrupts at once. The two functions are also used in a very parallel way, at the end of successive if branches. It seems strange to inline one function and not the other.

julia

In this case, the function names are very similar and the wrapper just add the macro IS_RCVAVAIL_START to the second argument, so the change made sense to me.

Quoted 7 lines

Quoted 5 lines

Personally, I would find the lack of parallelism confusing. But one could have an opinion either way on this one.

julia

So I should leave this as is or drop it? Confused.

Up to you in this case. I can see both points of view. In one point of view, it is nice to have parallel function names. In another point of view, in the case with three operations, a helper function is being introduced for a complicated case. This case is different than set/clear because the two functions are not eg opposites of each other. There are just four registers, three that fit together and one that is different.

julia

Quoted 62 lines · Julia Lawall

Quoted 43 lines · Amitoj Kaur Chawla

Quoted 42 lines · Julia Lawall

Quoted 39 lines · Amitoj Kaur Chawla

Quoted 28 lines · Julia Lawall

Quoted 13 lines · Amitoj Kaur Chawla

Drop wrapper function remap_receive_available_interrupt() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index beac326d..a9c6abf 100644

drivers/staging/rdma/hfi1/chip.c static void remap_sdma_interrupts(struct hfi1_devdata *dd, · line 8756

Quoted 8 lines

drivers/staging/rdma/hfi1/chip.c +0 -5

@@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
              msix_intr);
 }
 
-static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
-                                           int rx, int msix_intr)
-{
-     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
-}

remap_intr is not a standard kernel function. But perhaps one could argue that the constant name gives the same information as the function name, so the function is not needed.

Quoted 6 lines

But this function is the neighbor of another very similar function that remaps three interrupts at once. The two functions are also used in a very parallel way, at the end of successive if branches. It seems strange to inline one function and not the other.

julia

In this case, the function names are very similar and the wrapper just add the macro IS_RCVAVAIL_START to the second argument, so the change made sense to me.

Quoted 7 lines

Quoted 5 lines

Personally, I would find the lack of parallelism confusing. But one could have an opinion either way on this one.

julia

So I should leave this as is or drop it? Confused.

Up to you in this case. I can see both points of view. In one point of view, it is nice to have parallel function names. In another point of view, in the case with three operations, a helper function is being introduced for a complicated case. This case is different than set/clear because the two functions are not eg opposites of each other. There are just four registers, three that fit together and one that is different.

Quoted 1 line

julia

I'll send this one as a separate patch , since Greg would then decide whether the change is worth being made and send the changed patch series separately.

Quoted 65 lines · Amitoj Kaur Chawla

Quoted 63 lines · Julia Lawall

Quoted 43 lines · Amitoj Kaur Chawla

Quoted 42 lines · Julia Lawall

Quoted 39 lines · Amitoj Kaur Chawla

Quoted 28 lines · Julia Lawall

Quoted 13 lines · Amitoj Kaur Chawla

Drop wrapper function remap_receive_available_interrupt() that can be replaced by a single line of code.

Signed-off-by Amitoj Kaur Chawla <[email protected]>

---
 drivers/staging/rdma/hfi1/chip.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/staging/rdma/hfi1/chip.c +0 -0

index beac326d..a9c6abf 100644

drivers/staging/rdma/hfi1/chip.c static void remap_sdma_interrupts(struct hfi1_devdata *dd, · line 8756

Quoted 8 lines

drivers/staging/rdma/hfi1/chip.c +0 -5

@@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
              msix_intr);
 }
 
-static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
-                                           int rx, int msix_intr)
-{
-     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
-}

remap_intr is not a standard kernel function. But perhaps one could argue that the constant name gives the same information as the function name, so the function is not needed.

Quoted 6 lines

But this function is the neighbor of another very similar function that remaps three interrupts at once. The two functions are also used in a very parallel way, at the end of successive if branches. It seems strange to inline one function and not the other.

julia

In this case, the function names are very similar and the wrapper just add the macro IS_RCVAVAIL_START to the second argument, so the change made sense to me.

Quoted 7 lines

Quoted 5 lines

Personally, I would find the lack of parallelism confusing. But one could have an opinion either way on this one.

julia

So I should leave this as is or drop it? Confused.

Up to you in this case. I can see both points of view. In one point of view, it is nice to have parallel function names. In another point of view, in the case with three operations, a helper function is being introduced for a complicated case. This case is different than set/clear because the two functions are not eg opposites of each other. There are just four registers, three that fit together and one that is different.

julia

Quoted 3 lines

I'll send this one as a separate patch , since Greg would then decide whether the change is worth being made and send the changed patch series separately.

If it's a different file than the others, then it's fine.

julia