Sorry, this is still in my postponed messages folder. I meant to send
it earlier.
On Mon, Sep 02, 2013 at 10:23:21PM +0100, Mark Einon wrote:
> Ignoring checkpatch for some lines - now just over 80 chars, but much
> more readable.
>
The person who "fixed" these long lines, did it in the wrong way, yes.
But we always apply those because it's the easiest way and you can't
fight against checkpatch.pl. If we go back to long lines, we'll just
immediately apply another "break the long lines up" patch from some
newbie who doesn't know any better.
We need to fix it in the right way instead of re-introducing checkpatch
warnings.
> Signed-off-by: Mark Einon <mark.einon@xxxxxxxxx>
> ---
> drivers/staging/et131x/et131x.c | 120 +++++++++++++--------------------------
> 1 file changed, 40 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> index a8f7cd7..55a6d59 100644
> --- a/drivers/staging/et131x/et131x.c
> +++ b/drivers/staging/et131x/et131x.c
> @@ -872,8 +872,8 @@ static void et131x_tx_dma_enable(struct et131x_adapter *adapter)
> /* Setup the transmit dma configuration register for normal
> * operation
> */
> - writel(ET_TXDMA_SNGL_EPKT|(PARM_DMA_CACHE_DEF << ET_TXDMA_CACHE_SHIFT),
> - &adapter->regs->txdma.csr);
> + writel(ET_TXDMA_SNGL_EPKT | (PARM_DMA_CACHE_DEF << ET_TXDMA_CACHE_SHIFT),
PARM_DMA_CACHE_DEF is a poorly chosen name. It is too generic, it's too
long and it's misleading. Also it is zero so this line is partly a
no-op.
> + &adapter->regs->txdma.csr);
> }
>
> static inline void add_10bit(u32 *v, int n)
> @@ -978,12 +978,10 @@ static void et1310_config_mac_regs2(struct et131x_adapter *adapter)
> }
>
> /* We need to enable Rx/Tx */
> - cfg1 |= ET_MAC_CFG1_RX_ENABLE | ET_MAC_CFG1_TX_ENABLE |
> - ET_MAC_CFG1_TX_FLOW;
> + cfg1 |= ET_MAC_CFG1_RX_ENABLE | ET_MAC_CFG1_TX_ENABLE | ET_MAC_CFG1_TX_FLOW;
Is it possible to choose shorter names? If the names were each 2
characters shorter than this would fit. I haven't really understood why
there are two configurations. Why would you choose one configuration
as opposed to the other? What I'm saying is that the naming is unclear.
> /* Initialize loop back to off */
> cfg1 &= ~(ET_MAC_CFG1_LOOPBACK | ET_MAC_CFG1_RX_FLOW);
> - if (adapter->flowcontrol == FLOW_RXONLY ||
> - adapter->flowcontrol == FLOW_BOTH)
> + if (adapter->flowcontrol == FLOW_RXONLY || adapter->flowcontrol == FLOW_BOTH)
> cfg1 |= ET_MAC_CFG1_RX_FLOW;
This should be:
if (adapter->flowcontrol == FLOW_RXONLY ||
adapter->flowcontrol == FLOW_BOTH)
cfg1 |= ET_MAC_CFG1_RX_FLOW;
writel(cfg1, &mac->cfg1);
When it's aligned like that it's fine and perfectly readable.
> writel(cfg1, &mac->cfg1);
>
> @@ -1807,8 +1805,7 @@ static void et131x_config_rx_dma_regs(struct et131x_adapter *adapter)
> writel(0, &rx_dma->psr_full_offset);
>
> psr_num_des = readl(&rx_dma->psr_num_des) & ET_RXDMA_PSR_NUM_DES_MASK;
> - writel((psr_num_des * LO_MARK_PERCENT_FOR_PSR) / 100,
> - &rx_dma->psr_min_des);
> + writel((psr_num_des * LO_MARK_PERCENT_FOR_PSR) / 100, &rx_dma->psr_min_des);
The original is fine and it's already aligned properly.
>
> spin_lock_irqsave(&adapter->rcv_lock, flags);
>
> @@ -1837,10 +1834,8 @@ static void et131x_config_rx_dma_regs(struct et131x_adapter *adapter)
> }
>
> /* Now's the best time to initialize FBR contents */
Do this:
struct fbr_lookup *fbr;
fbr = rx_local->fbr[id];
Then replace all the instances of "rx_local->fbr[id]" and it solves the
problems in this function.
> - fbr_entry =
> - (struct fbr_desc *) rx_local->fbr[id]->ring_virtaddr;
> - for (entry = 0;
> - entry < rx_local->fbr[id]->num_entries; entry++) {
> + fbr_entry = (struct fbr_desc *) rx_local->fbr[id]->ring_virtaddr;
> + for (entry = 0; entry < rx_local->fbr[id]->num_entries; entry++) {
> fbr_entry->addr_hi = rx_local->fbr[id]->bus_high[entry];
> fbr_entry->addr_lo = rx_local->fbr[id]->bus_low[entry];
> fbr_entry->word2 = entry;
> @@ -1850,10 +1845,8 @@ static void et131x_config_rx_dma_regs(struct et131x_adapter *adapter)
> /* Set the address and parameters of Free buffer ring 1 and 0
> * into the 1310's registers
> */
> - writel(upper_32_bits(rx_local->fbr[id]->ring_physaddr),
> - base_hi);
> - writel(lower_32_bits(rx_local->fbr[id]->ring_physaddr),
> - base_lo);
> + writel(upper_32_bits(rx_local->fbr[id]->ring_physaddr), base_hi);
> + writel(lower_32_bits(rx_local->fbr[id]->ring_physaddr), base_lo);
> writel(rx_local->fbr[id]->num_entries - 1, num_des);
> writel(ET_DMA10_WRAP, full_offset);
>
> @@ -1862,8 +1855,7 @@ static void et131x_config_rx_dma_regs(struct et131x_adapter *adapter)
> */
> rx_local->fbr[id]->local_full = ET_DMA10_WRAP;
> writel(((rx_local->fbr[id]->num_entries *
> - LO_MARK_PERCENT_FOR_RX) / 100) - 1,
> - min_des);
> + LO_MARK_PERCENT_FOR_RX) / 100) - 1, min_des);
> }
>
> /* Program the number of packets we will receive before generating an
> @@ -1894,19 +1886,15 @@ static void et131x_config_tx_dma_regs(struct et131x_adapter *adapter)
> struct txdma_regs __iomem *txdma = &adapter->regs->txdma;
>
Do this like:
struct tx_ring *tx_ring = &adapter->tx_ring;
writel(upper_32_bits(tx_ring->tx_desc_ring_pa), &txdma->pr_base_hi);
writel(lower_32_bits(tx_ring->tx_desc_ring_pa), &txdma->pr_base_lo);
> /* Load the hardware with the start of the transmit descriptor ring. */
> - writel(upper_32_bits(adapter->tx_ring.tx_desc_ring_pa),
> - &txdma->pr_base_hi);
> - writel(lower_32_bits(adapter->tx_ring.tx_desc_ring_pa),
> - &txdma->pr_base_lo);
> + writel(upper_32_bits(adapter->tx_ring.tx_desc_ring_pa), &txdma->pr_base_hi);
> + writel(lower_32_bits(adapter->tx_ring.tx_desc_ring_pa), &txdma->pr_base_lo);
>
> /* Initialise the transmit DMA engine */
> writel(NUM_DESC_PER_RING_TX - 1, &txdma->pr_num_des);
>
> /* Load the completion writeback physical address */
> - writel(upper_32_bits(adapter->tx_ring.tx_status_pa),
> - &txdma->dma_wb_base_hi);
> - writel(lower_32_bits(adapter->tx_ring.tx_status_pa),
> - &txdma->dma_wb_base_lo);
> + writel(upper_32_bits(adapter->tx_ring.tx_status_pa), &txdma->dma_wb_base_hi);
> + writel(lower_32_bits(adapter->tx_ring.tx_status_pa), &txdma->dma_wb_base_lo);
>
> *adapter->tx_ring.tx_status = 0;
>
> @@ -1975,8 +1963,7 @@ static void et131x_enable_interrupts(struct et131x_adapter *adapter)
> u32 mask;
>
> /* Enable all global interrupts */
> - if (adapter->flowcontrol == FLOW_TXONLY ||
> - adapter->flowcontrol == FLOW_BOTH)
> + if (adapter->flowcontrol == FLOW_TXONLY || adapter->flowcontrol == FLOW_BOTH)
> mask = INT_MASK_ENABLE;
> else
> mask = INT_MASK_ENABLE_NO_FLOW;
> @@ -2001,8 +1988,7 @@ static void et131x_disable_interrupts(struct et131x_adapter *adapter)
> static void et131x_tx_dma_disable(struct et131x_adapter *adapter)
> {
> /* Setup the tramsmit dma configuration register */
> - writel(ET_TXDMA_CSR_HALT | ET_TXDMA_SNGL_EPKT,
> - &adapter->regs->txdma.csr);
> + writel(ET_TXDMA_CSR_HALT | ET_TXDMA_SNGL_EPKT, &adapter->regs->txdma.csr);
The original was aligned badly. It should be:
writel(ET_TXDMA_CSR_HALT | ET_TXDMA_SNGL_EPKT,
&adapter->regs->txdma.csr);
Etc. The rest of the patch is similar.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
