On Wed, 2019-08-28 at 16:03 +0200, Thomas Bogendoerfer wrote:
> Before massaging the driver further fix oddities found by checkpatch like
> - wrong indention
> - comment formatting
> - use of printk instead or netdev_xxx/pr_xxx
trivial notes:
Please try to make the code better rather than merely
shutting up checkpatch.
> diff --git a/drivers/net/ethernet/sgi/ioc3-eth.c
> b/drivers/net/ethernet/sgi/ioc3-eth.c
[]
> @@ -209,8 +201,7 @@ static inline void nic_write_bit(u32 __iomem *mcr, int
> bit)
> nic_wait(mcr);
> }
>
> -/*
> - * Read a byte from an iButton device
> +/* Read a byte from an iButton device
> */
These comment styles would be simpler on a single line
/* Read a byte from an iButton device */
> static u32 nic_read_byte(u32 __iomem *mcr)
> {
> @@ -223,8 +214,7 @@ static u32 nic_read_byte(u32 __iomem *mcr)
> return result;
> }
>
> -/*
> - * Write a byte to an iButton device
> +/* Write a byte to an iButton device
> */
/* Write a byte to an iButton device */
etc...
[]
> @@ -323,16 +315,15 @@ static int nic_init(u32 __iomem *mcr)
> break;
> }
>
> - printk("Found %s NIC", type);
> + pr_info("Found %s NIC", type);
> if (type != unknown)
> - printk (" registration number %pM, CRC %02x", serial, crc);
> - printk(".\n");
> + pr_cont(" registration number %pM, CRC %02x", serial, crc);
> + pr_cont(".\n");
This code would be more sensible as
if (type != unknown)
pr_info("Found %s NIC registration number %pM, CRC %02x\n",
type, serial, crc);
else
pr_info("Found %s NIC\n", type);
Though I don't know if registration number is actually a MAC
address or something else. If it's just a 6 byte identifier
that uses colon separation it should probably use "%6phC"
instead of "%pM"
[]
> @@ -645,22 +636,21 @@ static inline void ioc3_tx(struct net_device *dev)
> static void ioc3_error(struct net_device *dev, u32 eisr)
> {
> struct ioc3_private *ip = netdev_priv(dev);
> - unsigned char *iface = dev->name;
>
> spin_lock(&ip->ioc3_lock);
>
> if (eisr & EISR_RXOFLO)
> - printk(KERN_ERR "%s: RX overflow.\n", iface);
> + netdev_err(dev, "RX overflow.\n");
> if (eisr & EISR_RXBUFOFLO)
> - printk(KERN_ERR "%s: RX buffer overflow.\n", iface);
> + netdev_err(dev, "RX buffer overflow.\n");
> if (eisr & EISR_RXMEMERR)
> - printk(KERN_ERR "%s: RX PCI error.\n", iface);
> + netdev_err(dev, "RX PCI error.\n");
> if (eisr & EISR_RXPARERR)
> - printk(KERN_ERR "%s: RX SSRAM parity error.\n", iface);
> + netdev_err(dev, "RX SSRAM parity error.\n");
> if (eisr & EISR_TXBUFUFLO)
> - printk(KERN_ERR "%s: TX buffer underflow.\n", iface);
> + netdev_err(dev, "TX buffer underflow.\n");
> if (eisr & EISR_TXMEMERR)
> - printk(KERN_ERR "%s: TX PCI error.\n", iface);
> + netdev_err(dev, "TX PCI error.\n");
All of these should probably be ratelimited() output.
