Steve.Glendinning@smsc.com wrote:

Hi Ben,

Thanks for the feedback.  I've implemented most of your suggestions, but 
have a few questions:

quoted

[...]

quoted

+/* Set a mac register, phy_lock must be acquired before calling */
+static void smsc911x_mac_write(struct smsc911x_data *pdata,
+                unsigned int offset, u32 val)
+{
+   unsigned int temp;
+
+#ifdef CONFIG_DEBUG_SPINLOCK
+   if (!spin_is_locked(&pdata->phy_lock))
+      SMSC_WARNING("phy_lock not held");
+#endif            /* CONFIG_DEBUG_SPINLOCK */
+
+   temp = smsc911x_reg_read(pdata, MAC_CSR_CMD);
+   if (unlikely(temp & MAC_CSR_CMD_CSR_BUSY_)) {
+      SMSC_WARNING("smsc911x_mac_write failed, MAC busy at entry");
+      return;
+   }

Shouldn't this return an error code?

[...]

quoted

+/* Sets a phy register, phy_lock must be acquired before calling */
+static void smsc911x_phy_write(struct smsc911x_data *pdata,
+                unsigned int index, u16 val)
+{
+   unsigned int addr;
+   int i;
+
+#ifdef CONFIG_DEBUG_SPINLOCK
+   if (!spin_is_locked(&pdata->phy_lock))
+      SMSC_WARNING("phy_lock not held");
+#endif            /* CONFIG_DEBUG_SPINLOCK */
+
+   /* Confirm MII not busy */
+   if (unlikely(smsc911x_mac_read(pdata, MII_ACC) & 

MII_ACC_MII_BUSY_)) {

quoted

quoted

+      SMSC_WARNING("MII is busy in smsc911x_write_phy???");
+      return;
+   }

Similarly for this function.

I could return an error code from these write functions, but the 
equivalent read functions currently return their register value.  Would 
you change the read functions to take a result pointer?

If you can't reserve some range of values for errors (e.g. 32-bit reads
which may yield any 32-bit value) then I suppose so.  For 16-bit reads you
can return an int with negative values reserved for errors.

I should mention this "MAC busy at entry" check is an assert to catch 
driver locking problems during development (as is the spinlock check 
above).  If the driver is "correct" it should only be seen in the case of 
hardware failure.

Hardware failure does happen though, and once you've detected it it seems
like a bad idea to hide it.

quoted

[...]

quoted

+static int smsc911x_soft_reset(struct smsc911x_data *pdata)
+{
+   unsigned int timeout;
+   unsigned int temp;
+
+   /* Reset the LAN911x */
+   smsc911x_reg_write(HW_CFG_SRST_, pdata, HW_CFG);
+   timeout = 10;
+   do {
+      udelay(10);
+      temp = smsc911x_reg_read(pdata, HW_CFG);
+   } while ((--timeout) && (temp & HW_CFG_SRST_));
+
+   if (unlikely(temp & HW_CFG_SRST_)) {
+      SMSC_WARNING("Failed to complete reset");
+      return -ENODEV;

I think this should be -EIO unless this is only called during probe.

This reset function is called from both probe and open, although its 
return code is only checked for nonzero by both.  Should probe return 
-ENODEV, and open return -EIO if this device reset fails?

Personally I think -EIO is a perfectly reasonable error code for I/O
errors after the device has been positively identified, even during
probe.  You could have the probe function squash other errors into
-ENODEV if you want.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.