All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] phylib: cleanups
@ 2014-01-04  1:13 Sergei Shtylyov
  2014-01-04  1:14 ` [PATCH 1/6] phy: coding style fixes Sergei Shtylyov
                   ` (8 more replies)
  0 siblings, 9 replies; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04  1:13 UTC (permalink / raw)
  To: netdev

Hello.

   This patchset mainly cleans up the phylib's coding style but then also
removes some historically unused stuff.

[1/6] phy: coding style fixes
[2/6] mdio_bus: coding style fixes
[3/6] phy: kill useless local variables
[4/6] phy: kill excess code
[5/6] phylib: remove unused adjust_state callback
[6/6] phylib: make phy_scan_fixups() static

WBR, Sergei

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/6] phy: coding style fixes
  2014-01-04  1:13 [PATCH 0/6] phylib: cleanups Sergei Shtylyov
@ 2014-01-04  1:14 ` Sergei Shtylyov
  2014-01-04  1:16 ` [PATCH 2/6] mdio_bus: " Sergei Shtylyov
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04  1:14 UTC (permalink / raw)
  To: netdev

The recent patch from Florian Fainelli fixed all 'checkpatch.pl' errors but left
the numerous warnings like:

- including <asm/io.h> instead of <linux/io.h>;

- including <asm/uaccess.h> instead of <linux/uaccess.h>;

- block comments using empty /* line;

- block comments not starting with * on the middle lines; 

- block comments not having trailing */ on a separate line;

- EXPORT_SYMBOL() not immediately following its function;

- unnecessary {} for signle statement block;

- spaces before tabs.

While fixing these, also fix the following style issues (some of which were
found running 'checkpatch.pl --strict'):

- alignment not matching open paren;

- missing {} on one of the *if* arms where another has them;

- use of sizeof(struct structure) instead of sizeof(*variable);

- multiple assignments on one line;

- empty line before };

- file names in the heading comments;

- missing spaces around operators;

- no {} around multi-line *if* operator's arm;

- unneeded () around subexpressions;

- incomplete kernel-doc comment style;

- comment line exceeding 80 characters;

- missing empty line after declarations.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/net/phy/phy.c        |   82 ++++++++++-------------
 drivers/net/phy/phy_device.c |  147 ++++++++++++++++++++++---------------------
 2 files changed, 114 insertions(+), 115 deletions(-)

Index: net-next/drivers/net/phy/phy.c
===================================================================
--- net-next.orig/drivers/net/phy/phy.c
+++ net-next/drivers/net/phy/phy.c
@@ -1,7 +1,4 @@
-/*
- * drivers/net/phy/phy.c
- *
- * Framework for configuring and reading PHY devices
+/* Framework for configuring and reading PHY devices
  * Based on code in sungem_phy.c and gianfar_phy.c
  *
  * Author: Andy Fleming
@@ -36,11 +33,11 @@
 #include <linux/timer.h>
 #include <linux/workqueue.h>
 #include <linux/mdio.h>
-
+#include <linux/io.h>
+#include <linux/uaccess.h>
 #include <linux/atomic.h>
-#include <asm/io.h>
+
 #include <asm/irq.h>
-#include <asm/uaccess.h>
 
 /**
  * phy_print_status - Convenience function to print out the current phy status
@@ -48,13 +45,14 @@
  */
 void phy_print_status(struct phy_device *phydev)
 {
-	if (phydev->link)
+	if (phydev->link) {
 		pr_info("%s - Link is Up - %d/%s\n",
 			dev_name(&phydev->dev),
 			phydev->speed,
 			DUPLEX_FULL == phydev->duplex ? "Full" : "Half");
-	else
+	} else	{
 		pr_info("%s - Link is Down\n", dev_name(&phydev->dev));
+	}
 }
 EXPORT_SYMBOL(phy_print_status);
 
@@ -114,7 +112,8 @@ static inline int phy_aneg_done(struct p
 }
 
 /* A structure for mapping a particular speed and duplex
- * combination to a particular SUPPORTED and ADVERTISED value */
+ * combination to a particular SUPPORTED and ADVERTISED value
+ */
 struct phy_setting {
 	int speed;
 	int duplex;
@@ -177,8 +176,7 @@ static inline int phy_find_setting(int s
 	int idx = 0;
 
 	while (idx < ARRAY_SIZE(settings) &&
-			(settings[idx].speed != speed ||
-			settings[idx].duplex != duplex))
+	       (settings[idx].speed != speed || settings[idx].duplex != duplex))
 		idx++;
 
 	return idx < MAX_NUM_SETTINGS ? idx : MAX_NUM_SETTINGS - 1;
@@ -245,8 +243,7 @@ int phy_ethtool_sset(struct phy_device *
 	if (cmd->phy_address != phydev->addr)
 		return -EINVAL;
 
-	/* We make sure that we don't pass unsupported
-	 * values in to the PHY */
+	/* We make sure that we don't pass unsupported values in to the PHY */
 	cmd->advertising &= phydev->supported;
 
 	/* Verify the settings we care about. */
@@ -313,8 +310,7 @@ EXPORT_SYMBOL(phy_ethtool_gset);
  * PHYCONTROL layer.  It changes registers without regard to
  * current state.  Use at own risk.
  */
-int phy_mii_ioctl(struct phy_device *phydev,
-		struct ifreq *ifr, int cmd)
+int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 {
 	struct mii_ioctl_data *mii_data = if_mii(ifr);
 	u16 val = mii_data->val_in;
@@ -334,19 +330,18 @@ int phy_mii_ioctl(struct phy_device *phy
 		if (mii_data->phy_id == phydev->addr) {
 			switch (mii_data->reg_num) {
 			case MII_BMCR:
-				if ((val & (BMCR_RESET|BMCR_ANENABLE)) == 0)
+				if ((val & (BMCR_RESET | BMCR_ANENABLE)) == 0)
 					phydev->autoneg = AUTONEG_DISABLE;
 				else
 					phydev->autoneg = AUTONEG_ENABLE;
-				if ((!phydev->autoneg) && (val & BMCR_FULLDPLX))
+				if (!phydev->autoneg && (val & BMCR_FULLDPLX))
 					phydev->duplex = DUPLEX_FULL;
 				else
 					phydev->duplex = DUPLEX_HALF;
-				if ((!phydev->autoneg) &&
-						(val & BMCR_SPEED1000))
+				if (!phydev->autoneg && (val & BMCR_SPEED1000))
 					phydev->speed = SPEED_1000;
-				else if ((!phydev->autoneg) &&
-						(val & BMCR_SPEED100))
+				else if (!phydev->autoneg &&
+					 (val & BMCR_SPEED100))
 					phydev->speed = SPEED_100;
 				break;
 			case MII_ADVERTISE:
@@ -433,7 +428,7 @@ EXPORT_SYMBOL(phy_start_aneg);
  *   function.
  */
 void phy_start_machine(struct phy_device *phydev,
-		void (*handler)(struct net_device *))
+		       void (*handler)(struct net_device *))
 {
 	phydev->adjust_state = handler;
 
@@ -494,7 +489,8 @@ static irqreturn_t phy_interrupt(int irq
 	/* The MDIO bus is not allowed to be written in interrupt
 	 * context, so we need to disable the irq here.  A work
 	 * queue will write the PHY to disable and clear the
-	 * interrupt, and then reenable the irq line. */
+	 * interrupt, and then reenable the irq line.
+	 */
 	disable_irq_nosync(irq);
 	atomic_inc(&phydev->irq_disable);
 
@@ -595,15 +591,13 @@ int phy_stop_interrupts(struct phy_devic
 
 	free_irq(phydev->irq, phydev);
 
-	/*
-	 * Cannot call flush_scheduled_work() here as desired because
+	/* Cannot call flush_scheduled_work() here as desired because
 	 * of rtnl_lock(), but we do not really care about what would
 	 * be done, except from enable_irq(), so cancel any work
 	 * possibly pending and take care of the matter below.
 	 */
 	cancel_work_sync(&phydev->phy_queue);
-	/*
-	 * If work indeed has been cancelled, disable_irq() will have
+	/* If work indeed has been cancelled, disable_irq() will have
 	 * been left unbalanced from phy_interrupt() and enable_irq()
 	 * has to be called so that other devices on the line work.
 	 */
@@ -691,12 +685,12 @@ void phy_stop(struct phy_device *phydev)
 out_unlock:
 	mutex_unlock(&phydev->lock);
 
-	/*
-	 * Cannot call flush_scheduled_work() here as desired because
+	/* Cannot call flush_scheduled_work() here as desired because
 	 * of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
 	 * will not reenable interrupts.
 	 */
 }
+EXPORT_SYMBOL(phy_stop);
 
 
 /**
@@ -727,7 +721,6 @@ void phy_start(struct phy_device *phydev
 	}
 	mutex_unlock(&phydev->lock);
 }
-EXPORT_SYMBOL(phy_stop);
 EXPORT_SYMBOL(phy_start);
 
 /**
@@ -765,8 +758,7 @@ void phy_state_machine(struct work_struc
 		if (err < 0)
 			break;
 
-		/* If the link is down, give up on
-		 * negotiation for now */
+		/* If the link is down, give up on negotiation for now */
 		if (!phydev->link) {
 			phydev->state = PHY_NOLINK;
 			netif_carrier_off(phydev->attached_dev);
@@ -774,8 +766,7 @@ void phy_state_machine(struct work_struc
 			break;
 		}
 
-		/* Check if negotiation is done.  Break
-		 * if there's an error */
+		/* Check if negotiation is done.  Break if there's an error */
 		err = phy_aneg_done(phydev);
 		if (err < 0)
 			break;
@@ -788,8 +779,7 @@ void phy_state_machine(struct work_struc
 
 		} else if (0 == phydev->link_timeout--) {
 			needs_aneg = 1;
-			/* If we have the magic_aneg bit,
-			 * we try again */
+			/* If we have the magic_aneg bit, we try again */
 			if (phydev->drv->flags & PHY_HAS_MAGICANEG)
 				break;
 		}
@@ -847,7 +837,7 @@ void phy_state_machine(struct work_struc
 
 		if (phy_interrupt_is_valid(phydev))
 			err = phy_config_interrupt(phydev,
-					PHY_INTERRUPT_ENABLED);
+						   PHY_INTERRUPT_ENABLED);
 		break;
 	case PHY_HALTED:
 		if (phydev->link) {
@@ -864,8 +854,7 @@ void phy_state_machine(struct work_struc
 		if (err)
 			break;
 
-		err = phy_config_interrupt(phydev,
-				PHY_INTERRUPT_ENABLED);
+		err = phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
 
 		if (err)
 			break;
@@ -876,8 +865,8 @@ void phy_state_machine(struct work_struc
 				break;
 
 			/* err > 0 if AN is done.
-			 * Otherwise, it's 0, and we're
-			 * still waiting for AN */
+			 * Otherwise, it's 0, and we're  still waiting for AN
+			 */
 			if (err > 0) {
 				err = phy_read_status(phydev);
 				if (err)
@@ -886,8 +875,9 @@ void phy_state_machine(struct work_struc
 				if (phydev->link) {
 					phydev->state = PHY_RUNNING;
 					netif_carrier_on(phydev->attached_dev);
-				} else
+				} else	{
 					phydev->state = PHY_NOLINK;
+				}
 				phydev->adjust_link(phydev->attached_dev);
 			} else {
 				phydev->state = PHY_AN;
@@ -901,8 +891,9 @@ void phy_state_machine(struct work_struc
 			if (phydev->link) {
 				phydev->state = PHY_RUNNING;
 				netif_carrier_on(phydev->attached_dev);
-			} else
+			} else	{
 				phydev->state = PHY_NOLINK;
+			}
 			phydev->adjust_link(phydev->attached_dev);
 		}
 		break;
@@ -920,7 +911,7 @@ void phy_state_machine(struct work_struc
 		phy_error(phydev);
 
 	queue_delayed_work(system_power_efficient_wq, &phydev->state_queue,
-			PHY_STATE_TIME * HZ);
+			   PHY_STATE_TIME * HZ);
 }
 
 void phy_mac_interrupt(struct phy_device *phydev, int new_link)
@@ -1091,7 +1082,6 @@ int phy_get_eee_err(struct phy_device *p
 {
 	return phy_read_mmd_indirect(phydev->bus, MDIO_PCS_EEE_WK_ERR,
 				     MDIO_MMD_PCS, phydev->addr);
-
 }
 EXPORT_SYMBOL(phy_get_eee_err);
 
Index: net-next/drivers/net/phy/phy_device.c
===================================================================
--- net-next.orig/drivers/net/phy/phy_device.c
+++ net-next/drivers/net/phy/phy_device.c
@@ -1,7 +1,4 @@
-/*
- * drivers/net/phy/phy_device.c
- *
- * Framework for finding and configuring PHYs.
+/* Framework for finding and configuring PHYs.
  * Also contains generic PHY driver
  *
  * Author: Andy Fleming
@@ -33,10 +30,10 @@
 #include <linux/mii.h>
 #include <linux/ethtool.h>
 #include <linux/phy.h>
+#include <linux/io.h>
+#include <linux/uaccess.h>
 
-#include <asm/io.h>
 #include <asm/irq.h>
-#include <asm/uaccess.h>
 
 MODULE_DESCRIPTION("PHY library");
 MODULE_AUTHOR("Andy Fleming");
@@ -63,21 +60,21 @@ static DEFINE_MUTEX(phy_fixup_lock);
 static int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 			     u32 flags, phy_interface_t interface);
 
-/*
- * Creates a new phy_fixup and adds it to the list
+/**
+ * phy_register_fixup - creates a new phy_fixup and adds it to the list
  * @bus_id: A string which matches phydev->dev.bus_id (or PHY_ANY_ID)
  * @phy_uid: Used to match against phydev->phy_id (the UID of the PHY)
- * 	It can also be PHY_ANY_UID
+ *	It can also be PHY_ANY_UID
  * @phy_uid_mask: Applied to phydev->phy_id and fixup->phy_uid before
- * 	comparison
+ *	comparison
  * @run: The actual code to be run when a matching PHY is found
  */
 int phy_register_fixup(const char *bus_id, u32 phy_uid, u32 phy_uid_mask,
-		int (*run)(struct phy_device *))
+		       int (*run)(struct phy_device *))
 {
 	struct phy_fixup *fixup;
 
-	fixup = kzalloc(sizeof(struct phy_fixup), GFP_KERNEL);
+	fixup = kzalloc(sizeof(*fixup), GFP_KERNEL);
 	if (!fixup)
 		return -ENOMEM;
 
@@ -96,7 +93,7 @@ EXPORT_SYMBOL(phy_register_fixup);
 
 /* Registers a fixup to be run on any PHY with the UID in phy_uid */
 int phy_register_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask,
-		int (*run)(struct phy_device *))
+			       int (*run)(struct phy_device *))
 {
 	return phy_register_fixup(PHY_ANY_ID, phy_uid, phy_uid_mask, run);
 }
@@ -104,14 +101,13 @@ EXPORT_SYMBOL(phy_register_fixup_for_uid
 
 /* Registers a fixup to be run on the PHY with id string bus_id */
 int phy_register_fixup_for_id(const char *bus_id,
-		int (*run)(struct phy_device *))
+			      int (*run)(struct phy_device *))
 {
 	return phy_register_fixup(bus_id, PHY_ANY_UID, 0xffffffff, run);
 }
 EXPORT_SYMBOL(phy_register_fixup_for_id);
 
-/*
- * Returns 1 if fixup matches phydev in bus_id and phy_uid.
+/* Returns 1 if fixup matches phydev in bus_id and phy_uid.
  * Fixups can be set to match any in one or more fields.
  */
 static int phy_needs_fixup(struct phy_device *phydev, struct phy_fixup *fixup)
@@ -121,7 +117,7 @@ static int phy_needs_fixup(struct phy_de
 			return 0;
 
 	if ((fixup->phy_uid & fixup->phy_uid_mask) !=
-			(phydev->phy_id & fixup->phy_uid_mask))
+	    (phydev->phy_id & fixup->phy_uid_mask))
 		if (fixup->phy_uid != PHY_ANY_UID)
 			return 0;
 
@@ -153,12 +149,12 @@ int phy_scan_fixups(struct phy_device *p
 EXPORT_SYMBOL(phy_scan_fixups);
 
 struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
-			bool is_c45, struct phy_c45_device_ids *c45_ids)
+				     bool is_c45,
+				     struct phy_c45_device_ids *c45_ids)
 {
 	struct phy_device *dev;
 
-	/* We allocate the device, and initialize the
-	 * default values */
+	/* We allocate the device, and initialize the default values */
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 
 	if (NULL == dev)
@@ -168,7 +164,8 @@ struct phy_device *phy_device_create(str
 
 	dev->speed = 0;
 	dev->duplex = -1;
-	dev->pause = dev->asym_pause = 0;
+	dev->pause = 0;
+	dev->asym_pause = 0;
 	dev->link = 1;
 	dev->interface = PHY_INTERFACE_MODE_GMII;
 
@@ -192,14 +189,15 @@ struct phy_device *phy_device_create(str
 	INIT_WORK(&dev->phy_queue, phy_change);
 
 	/* Request the appropriate module unconditionally; don't
-	   bother trying to do so only if it isn't already loaded,
-	   because that gets complicated. A hotplug event would have
-	   done an unconditional modprobe anyway.
-	   We don't do normal hotplug because it won't work for MDIO
-	   -- because it relies on the device staying around for long
-	   enough for the driver to get loaded. With MDIO, the NIC
-	   driver will get bored and give up as soon as it finds that
-	   there's no driver _already_ loaded. */
+	 * bother trying to do so only if it isn't already loaded,
+	 * because that gets complicated. A hotplug event would have
+	 * done an unconditional modprobe anyway.
+	 * We don't do normal hotplug because it won't work for MDIO
+	 * -- because it relies on the device staying around for long
+	 * enough for the driver to get loaded. With MDIO, the NIC
+	 * driver will get bored and give up as soon as it finds that
+	 * there's no driver _already_ loaded.
+	 */
 	request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT, MDIO_ID_ARGS(phy_id));
 
 	device_initialize(&dev->dev);
@@ -299,8 +297,7 @@ static int get_phy_id(struct mii_bus *bu
 	if (is_c45)
 		return get_phy_c45_ids(bus, addr, phy_id, c45_ids);
 
-	/* Grab the bits from PHYIR1, and put them
-	 * in the upper half */
+	/* Grab the bits from PHYIR1, and put them in the upper half */
 	phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
 
 	if (phy_reg < 0)
@@ -320,7 +317,8 @@ static int get_phy_id(struct mii_bus *bu
 }
 
 /**
- * get_phy_device - reads the specified PHY device and returns its @phy_device struct
+ * get_phy_device - reads the specified PHY device and returns its @phy_device
+ *		    struct
  * @bus: the target MII bus
  * @addr: PHY address on the MII bus
  * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
@@ -357,8 +355,7 @@ int phy_device_register(struct phy_devic
 {
 	int err;
 
-	/* Don't register a phy if one is already registered at this
-	 * address */
+	/* Don't register a phy if one is already registered at this address */
 	if (phydev->bus->phy_map[phydev->addr])
 		return -EINVAL;
 	phydev->bus->phy_map[phydev->addr] = phydev;
@@ -413,7 +410,7 @@ EXPORT_SYMBOL(phy_find_first);
  *   this function.
  */
 static void phy_prepare_link(struct phy_device *phydev,
-		void (*handler)(struct net_device *))
+			     void (*handler)(struct net_device *))
 {
 	phydev->adjust_link = handler;
 }
@@ -460,15 +457,16 @@ EXPORT_SYMBOL(phy_connect_direct);
  *   the desired functionality.
  */
 struct phy_device *phy_connect(struct net_device *dev, const char *bus_id,
-		void (*handler)(struct net_device *),
-		phy_interface_t interface)
+			       void (*handler)(struct net_device *),
+			       phy_interface_t interface)
 {
 	struct phy_device *phydev;
 	struct device *d;
 	int rc;
 
 	/* Search the list of PHY devices on the mdio bus for the
-	 * PHY with the requested name */
+	 * PHY with the requested name
+	 */
 	d = bus_find_device_by_name(&mdio_bus_type, NULL, bus_id);
 	if (!d) {
 		pr_err("PHY %s not found\n", bus_id);
@@ -485,7 +483,8 @@ struct phy_device *phy_connect(struct ne
 EXPORT_SYMBOL(phy_connect);
 
 /**
- * phy_disconnect - disable interrupts, stop state machine, and detach a PHY device
+ * phy_disconnect - disable interrupts, stop state machine, and detach a PHY
+ *		    device
  * @phydev: target phy_device struct
  */
 void phy_disconnect(struct phy_device *phydev)
@@ -534,8 +533,7 @@ static int phy_poll_reset(struct phy_dev
 	if (ret & BMCR_RESET)
 		return -ETIMEDOUT;
 
-	/*
-	 * Some chips (smsc911x) may still need up to another 1ms after the
+	/* Some chips (smsc911x) may still need up to another 1ms after the
 	 * BMCR_RESET bit is cleared before they are usable.
 	 */
 	msleep(1);
@@ -586,7 +584,8 @@ static int phy_attach_direct(struct net_
 	int err;
 
 	/* Assume that if there is no driver, that it doesn't
-	 * exist, and we should use the genphy driver. */
+	 * exist, and we should use the genphy driver.
+	 */
 	if (NULL == d->driver) {
 		if (phydev->is_c45) {
 			pr_err("No driver for phy %x\n", phydev->phy_id);
@@ -619,7 +618,8 @@ static int phy_attach_direct(struct net_
 
 	/* Do initial configuration here, now that
 	 * we have certain key parameters
-	 * (dev_flags and interface) */
+	 * (dev_flags and interface)
+	 */
 	err = phy_init_hw(phydev);
 	if (err)
 		phy_detach(phydev);
@@ -638,8 +638,8 @@ static int phy_attach_direct(struct net_
  * Description: Same as phy_attach_direct() except that a PHY bus_id
  *     string is passed instead of a pointer to a struct phy_device.
  */
-struct phy_device *phy_attach(struct net_device *dev,
-		const char *bus_id, phy_interface_t interface)
+struct phy_device *phy_attach(struct net_device *dev, const char *bus_id,
+			      phy_interface_t interface)
 {
 	struct bus_type *bus = &mdio_bus_type;
 	struct phy_device *phydev;
@@ -647,7 +647,8 @@ struct phy_device *phy_attach(struct net
 	int rc;
 
 	/* Search the list of PHY devices on the mdio bus for the
-	 * PHY with the requested name */
+	 * PHY with the requested name
+	 */
 	d = bus_find_device_by_name(bus, NULL, bus_id);
 	if (!d) {
 		pr_err("PHY %s not found\n", bus_id);
@@ -676,7 +677,8 @@ void phy_detach(struct phy_device *phyde
 	/* If the device had no specific driver before (i.e. - it
 	 * was using the generic driver), we unbind the device
 	 * from the generic driver so that there's a chance a
-	 * real driver could be loaded */
+	 * real driver could be loaded
+	 */
 	if (phydev->dev.driver == &genphy_driver.driver)
 		device_release_driver(&phydev->dev);
 }
@@ -724,17 +726,17 @@ static int genphy_config_advert(struct p
 	int oldadv, adv;
 	int err, changed = 0;
 
-	/* Only allow advertising what
-	 * this PHY supports */
+	/* Only allow advertising what this PHY supports */
 	phydev->advertising &= phydev->supported;
 	advertise = phydev->advertising;
 
 	/* Setup standard advertisement */
-	oldadv = adv = phy_read(phydev, MII_ADVERTISE);
+	adv = phy_read(phydev, MII_ADVERTISE);
 
 	if (adv < 0)
 		return adv;
 
+	oldadv = adv;
 	adv &= ~(ADVERTISE_ALL | ADVERTISE_100BASE4 | ADVERTISE_PAUSE_CAP |
 		 ADVERTISE_PAUSE_ASYM);
 	adv |= ethtool_adv_to_mii_adv_t(advertise);
@@ -749,12 +751,13 @@ static int genphy_config_advert(struct p
 
 	/* Configure gigabit if it's supported */
 	if (phydev->supported & (SUPPORTED_1000baseT_Half |
-				SUPPORTED_1000baseT_Full)) {
-		oldadv = adv = phy_read(phydev, MII_CTRL1000);
+				 SUPPORTED_1000baseT_Full)) {
+		adv = phy_read(phydev, MII_CTRL1000);
 
 		if (adv < 0)
 			return adv;
 
+		oldadv = adv;
 		adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
 		adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
 
@@ -783,7 +786,8 @@ int genphy_setup_forced(struct phy_devic
 	int err;
 	int ctl = 0;
 
-	phydev->pause = phydev->asym_pause = 0;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
 
 	if (SPEED_1000 == phydev->speed)
 		ctl |= BMCR_SPEED1000;
@@ -812,10 +816,10 @@ int genphy_restart_aneg(struct phy_devic
 	if (ctl < 0)
 		return ctl;
 
-	ctl |= (BMCR_ANENABLE | BMCR_ANRESTART);
+	ctl |= BMCR_ANENABLE | BMCR_ANRESTART;
 
 	/* Don't isolate the PHY if we're negotiating */
-	ctl &= ~(BMCR_ISOLATE);
+	ctl &= ~BMCR_ISOLATE;
 
 	ctl = phy_write(phydev, MII_BMCR, ctl);
 
@@ -846,7 +850,8 @@ int genphy_config_aneg(struct phy_device
 
 	if (result == 0) {
 		/* Advertisement hasn't changed, but maybe aneg was never on to
-		 * begin with?  Or maybe phy was isolated? */
+		 * begin with?  Or maybe phy was isolated?
+		 */
 		int ctl = phy_read(phydev, MII_BMCR);
 
 		if (ctl < 0)
@@ -857,7 +862,8 @@ int genphy_config_aneg(struct phy_device
 	}
 
 	/* Only restart aneg if we are advertising something different
-	 * than we were before.	 */
+	 * than we were before.
+	 */
 	if (result > 0)
 		result = genphy_restart_aneg(phydev);
 
@@ -914,8 +920,7 @@ int genphy_read_status(struct phy_device
 	int lpa;
 	int lpagb = 0;
 
-	/* Update the link, but return if there
-	 * was an error */
+	/* Update the link, but return if there was an error */
 	err = genphy_update_link(phydev);
 	if (err)
 		return err;
@@ -956,7 +961,8 @@ int genphy_read_status(struct phy_device
 
 		phydev->speed = SPEED_10;
 		phydev->duplex = DUPLEX_HALF;
-		phydev->pause = phydev->asym_pause = 0;
+		phydev->pause = 0;
+		phydev->asym_pause = 0;
 
 		if (lpagb & (LPA_1000FULL | LPA_1000HALF)) {
 			phydev->speed = SPEED_1000;
@@ -978,6 +984,7 @@ int genphy_read_status(struct phy_device
 		}
 	} else {
 		int bmcr = phy_read(phydev, MII_BMCR);
+
 		if (bmcr < 0)
 			return bmcr;
 
@@ -993,7 +1000,8 @@ int genphy_read_status(struct phy_device
 		else
 			phydev->speed = SPEED_10;
 
-		phydev->pause = phydev->asym_pause = 0;
+		phydev->pause = 0;
+		phydev->asym_pause = 0;
 	}
 
 	return 0;
@@ -1006,7 +1014,8 @@ static int genphy_config_init(struct phy
 	u32 features;
 
 	/* For now, I'll claim that the generic driver supports
-	 * all possible port types */
+	 * all possible port types
+	 */
 	features = (SUPPORTED_TP | SUPPORTED_MII
 			| SUPPORTED_AUI | SUPPORTED_FIBRE |
 			SUPPORTED_BNC);
@@ -1053,7 +1062,7 @@ int genphy_suspend(struct phy_device *ph
 	mutex_lock(&phydev->lock);
 
 	value = phy_read(phydev, MII_BMCR);
-	phy_write(phydev, MII_BMCR, (value | BMCR_PDOWN));
+	phy_write(phydev, MII_BMCR, value | BMCR_PDOWN);
 
 	mutex_unlock(&phydev->lock);
 
@@ -1068,7 +1077,7 @@ int genphy_resume(struct phy_device *phy
 	mutex_lock(&phydev->lock);
 
 	value = phy_read(phydev, MII_BMCR);
-	phy_write(phydev, MII_BMCR, (value & ~BMCR_PDOWN));
+	phy_write(phydev, MII_BMCR, value & ~BMCR_PDOWN);
 
 	mutex_unlock(&phydev->lock);
 
@@ -1101,7 +1110,7 @@ static int phy_probe(struct device *dev)
 	 * but the interrupt is still a valid one
 	 */
 	if (!(phydrv->flags & PHY_HAS_INTERRUPT) &&
-			phy_interrupt_is_valid(phydev))
+	    phy_interrupt_is_valid(phydev))
 		phydev->irq = PHY_POLL;
 
 	if (phydrv->flags & PHY_IS_INTERNAL)
@@ -1111,7 +1120,8 @@ static int phy_probe(struct device *dev)
 
 	/* Start out supporting everything. Eventually,
 	 * a controller will attach, and may modify one
-	 * or both of these values */
+	 * or both of these values
+	 */
 	phydev->supported = phydrv->features;
 	phydev->advertising = phydrv->features;
 
@@ -1124,7 +1134,6 @@ static int phy_probe(struct device *dev)
 	mutex_unlock(&phydev->lock);
 
 	return err;
-
 }
 
 static int phy_remove(struct device *dev)
@@ -1197,9 +1206,9 @@ EXPORT_SYMBOL(phy_driver_unregister);
 void phy_drivers_unregister(struct phy_driver *drv, int n)
 {
 	int i;
-	for (i = 0; i < n; i++) {
+
+	for (i = 0; i < n; i++)
 		phy_driver_unregister(drv + i);
-	}
 }
 EXPORT_SYMBOL(phy_drivers_unregister);
 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 2/6] mdio_bus: coding style fixes
  2014-01-04  1:13 [PATCH 0/6] phylib: cleanups Sergei Shtylyov
  2014-01-04  1:14 ` [PATCH 1/6] phy: coding style fixes Sergei Shtylyov
@ 2014-01-04  1:16 ` Sergei Shtylyov
  2014-01-04  1:17 ` [PATCH 3/6] phy: kill useless local variables Sergei Shtylyov
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04  1:16 UTC (permalink / raw)
  To: netdev

The recent patch from Florian Fainelli fixed all 'checkpatch.pl' errors but left
some warnings like:

- including <asm/io.h> instead of <linux/io.h>;

- including <asm/uaccess.h> instead of <linux/uaccess.h>;

- block comments using empty /* line;

- 'struct dev_pm_ops' variable not being *const*.

While fixing these, also fix the following style issues (some of which were
found running 'checkpatch.pl --strict'):

- alignment not matching open paren;

- file name in the heading comment.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/net/phy/mdio_bus.c |   27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

Index: net-next/drivers/net/phy/mdio_bus.c
===================================================================
--- net-next.orig/drivers/net/phy/mdio_bus.c
+++ net-next/drivers/net/phy/mdio_bus.c
@@ -1,7 +1,4 @@
-/*
- * drivers/net/phy/mdio_bus.c
- *
- * MDIO Bus interface
+/* MDIO Bus interface
  *
  * Author: Andy Fleming
  *
@@ -36,10 +33,10 @@
 #include <linux/mii.h>
 #include <linux/ethtool.h>
 #include <linux/phy.h>
+#include <linux/io.h>
+#include <linux/uaccess.h>
 
-#include <asm/io.h>
 #include <asm/irq.h>
-#include <asm/uaccess.h>
 
 /**
  * mdiobus_alloc_size - allocate a mii_bus structure
@@ -139,8 +136,7 @@ int mdiobus_register(struct mii_bus *bus
 	int i, err;
 
 	if (NULL == bus || NULL == bus->name ||
-			NULL == bus->read ||
-			NULL == bus->write)
+	    NULL == bus->read || NULL == bus->write)
 		return -EINVAL;
 
 	BUG_ON(bus->state != MDIOBUS_ALLOCATED &&
@@ -214,9 +210,7 @@ EXPORT_SYMBOL(mdiobus_unregister);
  */
 void mdiobus_free(struct mii_bus *bus)
 {
-	/*
-	 * For compatibility with error handling in drivers.
-	 */
+	/* For compatibility with error handling in drivers. */
 	if (bus->state == MDIOBUS_ALLOCATED) {
 		kfree(bus);
 		return;
@@ -335,15 +329,13 @@ static bool mdio_bus_phy_may_suspend(str
 	if (!netdev)
 		return true;
 
-	/*
-	 * Don't suspend PHY if the attched netdev parent may wakeup.
+	/* Don't suspend PHY if the attched netdev parent may wakeup.
 	 * The parent may point to a PCI device, as in tg3 driver.
 	 */
 	if (netdev->dev.parent && device_may_wakeup(netdev->dev.parent))
 		return false;
 
-	/*
-	 * Also don't suspend PHY if the netdev itself may wakeup. This
+	/* Also don't suspend PHY if the netdev itself may wakeup. This
 	 * is the case for devices w/o underlaying pwr. mgmt. aware bus,
 	 * e.g. SoC devices.
 	 */
@@ -358,8 +350,7 @@ static int mdio_bus_suspend(struct devic
 	struct phy_driver *phydrv = to_phy_driver(dev->driver);
 	struct phy_device *phydev = to_phy_device(dev);
 
-	/*
-	 * We must stop the state machine manually, otherwise it stops out of
+	/* We must stop the state machine manually, otherwise it stops out of
 	 * control, possibly with the phydev->lock held. Upon resume, netdev
 	 * may call phy routines that try to grab the same lock, and that may
 	 * lead to a deadlock.
@@ -415,7 +406,7 @@ static int mdio_bus_restore(struct devic
 	return 0;
 }
 
-static struct dev_pm_ops mdio_bus_pm_ops = {
+static const struct dev_pm_ops mdio_bus_pm_ops = {
 	.suspend = mdio_bus_suspend,
 	.resume = mdio_bus_resume,
 	.freeze = mdio_bus_suspend,

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 3/6] phy: kill useless local variables
  2014-01-04  1:13 [PATCH 0/6] phylib: cleanups Sergei Shtylyov
  2014-01-04  1:14 ` [PATCH 1/6] phy: coding style fixes Sergei Shtylyov
  2014-01-04  1:16 ` [PATCH 2/6] mdio_bus: " Sergei Shtylyov
@ 2014-01-04  1:17 ` Sergei Shtylyov
  2014-01-04  7:34   ` Richard Cochran
  2014-01-04  1:19 ` [PATCH 4/6] phy: kill excess code Sergei Shtylyov
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04  1:17 UTC (permalink / raw)
  To: netdev

A number of functions (especially in phy.c) has local variables that were hardly
needed in the first place -- remove them.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/net/phy/phy.c        |   55 ++++++++++++-------------------------------
 drivers/net/phy/phy_device.c |   10 +------
 2 files changed, 18 insertions(+), 47 deletions(-)

Index: net-next/drivers/net/phy/phy.c
===================================================================
--- net-next.orig/drivers/net/phy/phy.c
+++ net-next/drivers/net/phy/phy.c
@@ -67,12 +67,10 @@ EXPORT_SYMBOL(phy_print_status);
  */
 static int phy_clear_interrupt(struct phy_device *phydev)
 {
-	int err = 0;
-
 	if (phydev->drv->ack_interrupt)
-		err = phydev->drv->ack_interrupt(phydev);
+		return phydev->drv->ack_interrupt(phydev);
 
-	return err;
+	return 0;
 }
 
 /**
@@ -84,13 +82,11 @@ static int phy_clear_interrupt(struct ph
  */
 static int phy_config_interrupt(struct phy_device *phydev, u32 interrupts)
 {
-	int err = 0;
-
 	phydev->interrupts = interrupts;
 	if (phydev->drv->config_intr)
-		err = phydev->drv->config_intr(phydev);
+		return phydev->drv->config_intr(phydev);
 
-	return err;
+	return 0;
 }
 
 
@@ -314,7 +310,6 @@ int phy_mii_ioctl(struct phy_device *phy
 {
 	struct mii_ioctl_data *mii_data = if_mii(ifr);
 	u16 val = mii_data->val_in;
-	int ret = 0;
 
 	switch (cmd) {
 	case SIOCGMIIPHY:
@@ -324,7 +319,7 @@ int phy_mii_ioctl(struct phy_device *phy
 	case SIOCGMIIREG:
 		mii_data->val_out = mdiobus_read(phydev->bus, mii_data->phy_id,
 						 mii_data->reg_num);
-		break;
+		return 0;
 
 	case SIOCSMIIREG:
 		if (mii_data->phy_id == phydev->addr) {
@@ -358,8 +353,8 @@ int phy_mii_ioctl(struct phy_device *phy
 
 		if (mii_data->reg_num == MII_BMCR &&
 		    val & BMCR_RESET)
-			ret = phy_init_hw(phydev);
-		break;
+			return phy_init_hw(phydev);
+		return 0;
 
 	case SIOCSHWTSTAMP:
 		if (phydev->drv->hwtstamp)
@@ -369,8 +364,6 @@ int phy_mii_ioctl(struct phy_device *phy
 	default:
 		return -EOPNOTSUPP;
 	}
-
-	return ret;
 }
 EXPORT_SYMBOL(phy_mii_ioctl);
 
@@ -557,8 +550,6 @@ phy_err:
  */
 int phy_start_interrupts(struct phy_device *phydev)
 {
-	int err = 0;
-
 	atomic_set(&phydev->irq_disable, 0);
 	if (request_irq(phydev->irq, phy_interrupt,
 				IRQF_SHARED,
@@ -570,9 +561,7 @@ int phy_start_interrupts(struct phy_devi
 		return 0;
 	}
 
-	err = phy_enable_interrupts(phydev);
-
-	return err;
+	return phy_enable_interrupts(phydev);
 }
 EXPORT_SYMBOL(phy_start_interrupts);
 
@@ -615,7 +604,6 @@ EXPORT_SYMBOL(phy_stop_interrupts);
  */
 void phy_change(struct work_struct *work)
 {
-	int err;
 	struct phy_device *phydev =
 		container_of(work, struct phy_device, phy_queue);
 
@@ -623,9 +611,7 @@ void phy_change(struct work_struct *work
 	    !phydev->drv->did_interrupt(phydev))
 		goto ignore;
 
-	err = phy_disable_interrupts(phydev);
-
-	if (err)
+	if (phy_disable_interrupts(phydev))
 		goto phy_err;
 
 	mutex_lock(&phydev->lock);
@@ -637,10 +623,8 @@ void phy_change(struct work_struct *work
 	enable_irq(phydev->irq);
 
 	/* Reenable interrupts */
-	if (PHY_HALTED != phydev->state)
-		err = phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
-
-	if (err)
+	if (PHY_HALTED != phydev->state &&
+	    phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED))
 		goto irq_enable_err;
 
 	/* reschedule state queue work to run as soon as possible */
@@ -953,14 +937,10 @@ static inline void mmd_phy_indirect(stru
 static int phy_read_mmd_indirect(struct mii_bus *bus, int prtad, int devad,
 				 int addr)
 {
-	u32 ret;
-
 	mmd_phy_indirect(bus, prtad, devad, addr);
 
 	/* Read the content of the MMD's selected register */
-	ret = bus->read(bus, addr, MII_MMD_DATA);
-
-	return ret;
+	return bus->read(bus, addr, MII_MMD_DATA);
 }
 
 /**
@@ -1000,8 +980,6 @@ static void phy_write_mmd_indirect(struc
  */
 int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
 {
-	int ret = -EPROTONOSUPPORT;
-
 	/* According to 802.3az,the EEE is supported only in full duplex-mode.
 	 * Also EEE feature is active when core is operating with MII, GMII
 	 * or RGMII.
@@ -1027,7 +1005,7 @@ int phy_init_eee(struct phy_device *phyd
 
 		cap = mmd_eee_cap_to_ethtool_sup_t(eee_cap);
 		if (!cap)
-			goto eee_exit;
+			return -EPROTONOSUPPORT;
 
 		/* Check which link settings negotiated and verify it in
 		 * the EEE advertising registers.
@@ -1046,7 +1024,7 @@ int phy_init_eee(struct phy_device *phyd
 		lp = mmd_eee_adv_to_ethtool_adv_t(eee_lp);
 		idx = phy_find_setting(phydev->speed, phydev->duplex);
 		if (!(lp & adv & settings[idx].setting))
-			goto eee_exit;
+			return -EPROTONOSUPPORT;
 
 		if (clk_stop_enable) {
 			/* Configure the PHY to stop receiving xMII
@@ -1063,11 +1041,10 @@ int phy_init_eee(struct phy_device *phyd
 					       MDIO_MMD_PCS, phydev->addr, val);
 		}
 
-		ret = 0; /* EEE supported */
+		return 0; /* EEE supported */
 	}
 
-eee_exit:
-	return ret;
+	return -EPROTONOSUPPORT;
 }
 EXPORT_SYMBOL(phy_init_eee);
 
Index: net-next/drivers/net/phy/phy_device.c
===================================================================
--- net-next.orig/drivers/net/phy/phy_device.c
+++ net-next/drivers/net/phy/phy_device.c
@@ -329,7 +329,6 @@ static int get_phy_id(struct mii_bus *bu
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 {
 	struct phy_c45_device_ids c45_ids = {0};
-	struct phy_device *dev = NULL;
 	u32 phy_id = 0;
 	int r;
 
@@ -341,9 +340,7 @@ struct phy_device *get_phy_device(struct
 	if ((phy_id & 0x1fffffff) == 0x1fffffff)
 		return NULL;
 
-	dev = phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
-
-	return dev;
+	return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
 }
 EXPORT_SYMBOL(get_phy_device);
 
@@ -783,7 +780,6 @@ static int genphy_config_advert(struct p
  */
 int genphy_setup_forced(struct phy_device *phydev)
 {
-	int err;
 	int ctl = 0;
 
 	phydev->pause = 0;
@@ -797,9 +793,7 @@ int genphy_setup_forced(struct phy_devic
 	if (DUPLEX_FULL == phydev->duplex)
 		ctl |= BMCR_FULLDPLX;
 
-	err = phy_write(phydev, MII_BMCR, ctl);
-
-	return err;
+	return phy_write(phydev, MII_BMCR, ctl);
 }
 EXPORT_SYMBOL(genphy_setup_forced);
 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 4/6] phy: kill excess code
  2014-01-04  1:13 [PATCH 0/6] phylib: cleanups Sergei Shtylyov
                   ` (2 preceding siblings ...)
  2014-01-04  1:17 ` [PATCH 3/6] phy: kill useless local variables Sergei Shtylyov
@ 2014-01-04  1:19 ` Sergei Shtylyov
  2014-01-04  7:36   ` Richard Cochran
       [not found]   ` <CAGVrzcadLFE-FWvgDQE5gkd6COZkGs2Q4SQdnOdKE4Deu-qa0A@mail.gmail.com>
  2014-01-04  1:21 ` [PATCH 5/6] phylib: remove unused adjust_state callback Sergei Shtylyov
                   ` (4 subsequent siblings)
  8 siblings, 2 replies; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04  1:19 UTC (permalink / raw)
  To: netdev

Remove tens of lines of unnecessary code:

- kill empty lines between a function call and its result check;

- convert assignments to initializers;

- kill useless assignments before *return*;

- kill excess empty lines.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/net/phy/phy.c        |   31 ++++------------------------
 drivers/net/phy/phy_device.c |   46 +++++++------------------------------------
 2 files changed, 13 insertions(+), 64 deletions(-)

Index: net-next/drivers/net/phy/phy.c
===================================================================
--- net-next.orig/drivers/net/phy/phy.c
+++ net-next/drivers/net/phy/phy.c
@@ -100,9 +100,7 @@ static int phy_config_interrupt(struct p
  */
 static inline int phy_aneg_done(struct phy_device *phydev)
 {
-	int retval;
-
-	retval = phy_read(phydev, MII_BMSR);
+	int retval = phy_read(phydev, MII_BMSR);
 
 	return (retval < 0) ? retval : (retval & BMSR_ANEGCOMPLETE);
 }
@@ -386,7 +384,6 @@ int phy_start_aneg(struct phy_device *ph
 		phy_sanitize_settings(phydev);
 
 	err = phydev->drv->config_aneg(phydev);
-
 	if (err < 0)
 		goto out_unlock;
 
@@ -406,7 +403,6 @@ out_unlock:
 }
 EXPORT_SYMBOL(phy_start_aneg);
 
-
 /**
  * phy_start_machine - start PHY state machine tracking
  * @phydev: the phy_device struct
@@ -498,16 +494,12 @@ static irqreturn_t phy_interrupt(int irq
  */
 static int phy_enable_interrupts(struct phy_device *phydev)
 {
-	int err;
-
-	err = phy_clear_interrupt(phydev);
+	int err = phy_clear_interrupt(phydev);
 
 	if (err < 0)
 		return err;
 
-	err = phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
-
-	return err;
+	return phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
 }
 
 /**
@@ -571,9 +563,7 @@ EXPORT_SYMBOL(phy_start_interrupts);
  */
 int phy_stop_interrupts(struct phy_device *phydev)
 {
-	int err;
-
-	err = phy_disable_interrupts(phydev);
+	int err = phy_disable_interrupts(phydev);
 
 	if (err)
 		phy_error(phydev);
@@ -597,7 +587,6 @@ int phy_stop_interrupts(struct phy_devic
 }
 EXPORT_SYMBOL(phy_stop_interrupts);
 
-
 /**
  * phy_change - Scheduled by the phy_interrupt/timer to handle PHY changes
  * @work: work_struct that describes the work to be done
@@ -630,7 +619,6 @@ void phy_change(struct work_struct *work
 	/* reschedule state queue work to run as soon as possible */
 	cancel_delayed_work_sync(&phydev->state_queue);
 	queue_delayed_work(system_power_efficient_wq, &phydev->state_queue, 0);
-
 	return;
 
 ignore:
@@ -676,7 +664,6 @@ out_unlock:
 }
 EXPORT_SYMBOL(phy_stop);
 
-
 /**
  * phy_start - start or restart a PHY device
  * @phydev: target phy_device struct
@@ -738,7 +725,6 @@ void phy_state_machine(struct work_struc
 		break;
 	case PHY_AN:
 		err = phy_read_status(phydev);
-
 		if (err < 0)
 			break;
 
@@ -770,7 +756,6 @@ void phy_state_machine(struct work_struc
 		break;
 	case PHY_NOLINK:
 		err = phy_read_status(phydev);
-
 		if (err)
 			break;
 
@@ -782,7 +767,6 @@ void phy_state_machine(struct work_struc
 		break;
 	case PHY_FORCING:
 		err = genphy_update_link(phydev);
-
 		if (err)
 			break;
 
@@ -805,7 +789,6 @@ void phy_state_machine(struct work_struc
 		break;
 	case PHY_CHANGELINK:
 		err = phy_read_status(phydev);
-
 		if (err)
 			break;
 
@@ -832,14 +815,11 @@ void phy_state_machine(struct work_struc
 		}
 		break;
 	case PHY_RESUMING:
-
 		err = phy_clear_interrupt(phydev);
-
 		if (err)
 			break;
 
 		err = phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
-
 		if (err)
 			break;
 
@@ -1108,9 +1088,8 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
  */
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
 {
-	int val;
+	int val = ethtool_adv_to_mmd_eee_adv_t(data->advertised);
 
-	val = ethtool_adv_to_mmd_eee_adv_t(data->advertised);
 	phy_write_mmd_indirect(phydev->bus, MDIO_AN_EEE_ADV, MDIO_MMD_AN,
 			       phydev->addr, val);
 
Index: net-next/drivers/net/phy/phy_device.c
===================================================================
--- net-next.orig/drivers/net/phy/phy_device.c
+++ net-next/drivers/net/phy/phy_device.c
@@ -72,9 +72,8 @@ static int phy_attach_direct(struct net_
 int phy_register_fixup(const char *bus_id, u32 phy_uid, u32 phy_uid_mask,
 		       int (*run)(struct phy_device *))
 {
-	struct phy_fixup *fixup;
+	struct phy_fixup *fixup = kzalloc(sizeof(*fixup), GFP_KERNEL);
 
-	fixup = kzalloc(sizeof(*fixup), GFP_KERNEL);
 	if (!fixup)
 		return -ENOMEM;
 
@@ -132,9 +131,7 @@ int phy_scan_fixups(struct phy_device *p
 	mutex_lock(&phy_fixup_lock);
 	list_for_each_entry(fixup, &phy_fixup_list, list) {
 		if (phy_needs_fixup(phydev, fixup)) {
-			int err;
-
-			err = fixup->run(phydev);
+			int err = fixup->run(phydev);
 
 			if (err < 0) {
 				mutex_unlock(&phy_fixup_lock);
@@ -156,7 +153,6 @@ struct phy_device *phy_device_create(str
 
 	/* We allocate the device, and initialize the default values */
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-
 	if (NULL == dev)
 		return (struct phy_device *)PTR_ERR((void *)-ENOMEM);
 
@@ -299,7 +295,6 @@ static int get_phy_id(struct mii_bus *bu
 
 	/* Grab the bits from PHYIR1, and put them in the upper half */
 	phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
-
 	if (phy_reg < 0)
 		return -EIO;
 
@@ -307,7 +302,6 @@ static int get_phy_id(struct mii_bus *bu
 
 	/* Grab the bits from PHYIR2, and put them in the lower half */
 	phy_reg = mdiobus_read(bus, addr, MII_PHYSID2);
-
 	if (phy_reg < 0)
 		return -EIO;
 
@@ -729,7 +723,6 @@ static int genphy_config_advert(struct p
 
 	/* Setup standard advertisement */
 	adv = phy_read(phydev, MII_ADVERTISE);
-
 	if (adv < 0)
 		return adv;
 
@@ -750,7 +743,6 @@ static int genphy_config_advert(struct p
 	if (phydev->supported & (SUPPORTED_1000baseT_Half |
 				 SUPPORTED_1000baseT_Full)) {
 		adv = phy_read(phydev, MII_CTRL1000);
-
 		if (adv < 0)
 			return adv;
 
@@ -803,9 +795,7 @@ EXPORT_SYMBOL(genphy_setup_forced);
  */
 int genphy_restart_aneg(struct phy_device *phydev)
 {
-	int ctl;
-
-	ctl = phy_read(phydev, MII_BMCR);
+	int ctl = phy_read(phydev, MII_BMCR);
 
 	if (ctl < 0)
 		return ctl;
@@ -815,13 +805,10 @@ int genphy_restart_aneg(struct phy_devic
 	/* Don't isolate the PHY if we're negotiating */
 	ctl &= ~BMCR_ISOLATE;
 
-	ctl = phy_write(phydev, MII_BMCR, ctl);
-
-	return ctl;
+	return phy_write(phydev, MII_BMCR, ctl);
 }
 EXPORT_SYMBOL(genphy_restart_aneg);
 
-
 /**
  * genphy_config_aneg - restart auto-negotiation or write BMCR
  * @phydev: target phy_device struct
@@ -838,10 +825,8 @@ int genphy_config_aneg(struct phy_device
 		return genphy_setup_forced(phydev);
 
 	result = genphy_config_advert(phydev);
-
 	if (result < 0) /* error */
 		return result;
-
 	if (result == 0) {
 		/* Advertisement hasn't changed, but maybe aneg was never on to
 		 * begin with?  Or maybe phy was isolated?
@@ -879,13 +864,11 @@ int genphy_update_link(struct phy_device
 
 	/* Do a fake read */
 	status = phy_read(phydev, MII_BMSR);
-
 	if (status < 0)
 		return status;
 
 	/* Read link and autonegotiation status */
 	status = phy_read(phydev, MII_BMSR);
-
 	if (status < 0)
 		return status;
 
@@ -925,12 +908,10 @@ int genphy_read_status(struct phy_device
 		if (phydev->supported & (SUPPORTED_1000baseT_Half
 					| SUPPORTED_1000baseT_Full)) {
 			lpagb = phy_read(phydev, MII_STAT1000);
-
 			if (lpagb < 0)
 				return lpagb;
 
 			adv = phy_read(phydev, MII_CTRL1000);
-
 			if (adv < 0)
 				return adv;
 
@@ -940,14 +921,12 @@ int genphy_read_status(struct phy_device
 		}
 
 		lpa = phy_read(phydev, MII_LPA);
-
 		if (lpa < 0)
 			return lpa;
 
 		phydev->lp_advertising |= mii_lpa_to_ethtool_lpa_t(lpa);
 
 		adv = phy_read(phydev, MII_ADVERTISE);
-
 		if (adv < 0)
 			return adv;
 
@@ -1016,7 +995,6 @@ static int genphy_config_init(struct phy
 
 	/* Do we support autonegotiation? */
 	val = phy_read(phydev, MII_BMSR);
-
 	if (val < 0)
 		return val;
 
@@ -1034,7 +1012,6 @@ static int genphy_config_init(struct phy
 
 	if (val & BMSR_ESTATEN) {
 		val = phy_read(phydev, MII_ESTATUS);
-
 		if (val < 0)
 			return val;
 
@@ -1089,15 +1066,11 @@ EXPORT_SYMBOL(genphy_resume);
  */
 static int phy_probe(struct device *dev)
 {
-	struct phy_device *phydev;
-	struct phy_driver *phydrv;
-	struct device_driver *drv;
+	struct phy_device *phydev = to_phy_device(dev);
+	struct device_driver *drv = phydev->dev.driver;
+	struct phy_driver *phydrv = to_phy_driver(drv);
 	int err = 0;
 
-	phydev = to_phy_device(dev);
-
-	drv = phydev->dev.driver;
-	phydrv = to_phy_driver(drv);
 	phydev->drv = phydrv;
 
 	/* Disable the interrupt if the PHY doesn't support it
@@ -1132,9 +1105,7 @@ static int phy_probe(struct device *dev)
 
 static int phy_remove(struct device *dev)
 {
-	struct phy_device *phydev;
-
-	phydev = to_phy_device(dev);
+	struct phy_device *phydev = to_phy_device(dev);
 
 	mutex_lock(&phydev->lock);
 	phydev->state = PHY_DOWN;
@@ -1161,7 +1132,6 @@ int phy_driver_register(struct phy_drive
 	new_driver->driver.remove = phy_remove;
 
 	retval = driver_register(&new_driver->driver);
-
 	if (retval) {
 		pr_err("%s: Error %d in registering driver\n",
 		       new_driver->name, retval);

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 5/6] phylib: remove unused adjust_state callback
  2014-01-04  1:13 [PATCH 0/6] phylib: cleanups Sergei Shtylyov
                   ` (3 preceding siblings ...)
  2014-01-04  1:19 ` [PATCH 4/6] phy: kill excess code Sergei Shtylyov
@ 2014-01-04  1:21 ` Sergei Shtylyov
  2014-01-04  1:23 ` [PATCH 6/6] phylib: make phy_scan_fixups() static Sergei Shtylyov
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04  1:21 UTC (permalink / raw)
  To: netdev

Remove adjust_state() callback from 'struct phy_device' since it seems to have
never been really used from the inception: phy_start_machine() has been always
called with 2nd argument equal to NULL.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/net/phy/mdio_bus.c   |    4 ++--
 drivers/net/phy/phy.c        |   17 +++--------------
 drivers/net/phy/phy_device.c |    2 +-
 include/linux/phy.h          |    7 +------
 4 files changed, 7 insertions(+), 23 deletions(-)

Index: net-next/drivers/net/phy/mdio_bus.c
===================================================================
--- net-next.orig/drivers/net/phy/mdio_bus.c
+++ net-next/drivers/net/phy/mdio_bus.c
@@ -379,7 +379,7 @@ static int mdio_bus_resume(struct device
 
 no_resume:
 	if (phydev->attached_dev && phydev->adjust_link)
-		phy_start_machine(phydev, NULL);
+		phy_start_machine(phydev);
 
 	return 0;
 }
@@ -401,7 +401,7 @@ static int mdio_bus_restore(struct devic
 	phydev->link = 0;
 	phydev->state = PHY_UP;
 
-	phy_start_machine(phydev, NULL);
+	phy_start_machine(phydev);
 
 	return 0;
 }
Index: net-next/drivers/net/phy/phy.c
===================================================================
--- net-next.orig/drivers/net/phy/phy.c
+++ net-next/drivers/net/phy/phy.c
@@ -406,21 +406,15 @@ EXPORT_SYMBOL(phy_start_aneg);
 /**
  * phy_start_machine - start PHY state machine tracking
  * @phydev: the phy_device struct
- * @handler: callback function for state change notifications
  *
  * Description: The PHY infrastructure can run a state machine
  *   which tracks whether the PHY is starting up, negotiating,
  *   etc.  This function starts the timer which tracks the state
- *   of the PHY.  If you want to be notified when the state changes,
- *   pass in the callback @handler, otherwise, pass NULL.  If you
- *   want to maintain your own state machine, do not call this
- *   function.
+ *   of the PHY.  If you want to maintain your own state machine,
+ *   do not call this function.
  */
-void phy_start_machine(struct phy_device *phydev,
-		       void (*handler)(struct net_device *))
+void phy_start_machine(struct phy_device *phydev)
 {
-	phydev->adjust_state = handler;
-
 	queue_delayed_work(system_power_efficient_wq, &phydev->state_queue, HZ);
 }
 
@@ -440,8 +434,6 @@ void phy_stop_machine(struct phy_device 
 	if (phydev->state > PHY_UP)
 		phydev->state = PHY_UP;
 	mutex_unlock(&phydev->lock);
-
-	phydev->adjust_state = NULL;
 }
 
 /**
@@ -708,9 +700,6 @@ void phy_state_machine(struct work_struc
 
 	mutex_lock(&phydev->lock);
 
-	if (phydev->adjust_state)
-		phydev->adjust_state(phydev->attached_dev);
-
 	switch (phydev->state) {
 	case PHY_DOWN:
 	case PHY_STARTING:
Index: net-next/drivers/net/phy/phy_device.c
===================================================================
--- net-next.orig/drivers/net/phy/phy_device.c
+++ net-next/drivers/net/phy/phy_device.c
@@ -424,7 +424,7 @@ int phy_connect_direct(struct net_device
 		return rc;
 
 	phy_prepare_link(phydev, handler);
-	phy_start_machine(phydev, NULL);
+	phy_start_machine(phydev);
 	if (phydev->irq > 0)
 		phy_start_interrupts(phydev);
 
Index: net-next/include/linux/phy.h
===================================================================
--- net-next.orig/include/linux/phy.h
+++ net-next/include/linux/phy.h
@@ -284,8 +284,6 @@ struct phy_c45_device_ids {
  * attached_dev: The attached enet driver's device instance ptr
  * adjust_link: Callback for the enet controller to respond to
  * changes in the link state.
- * adjust_state: Callback for the enet driver to respond to
- * changes in the state machine.
  *
  * speed, duplex, pause, supported, advertising, lp_advertising,
  * and autoneg are used like in mii_if_info
@@ -366,8 +364,6 @@ struct phy_device {
 	struct net_device *attached_dev;
 
 	void (*adjust_link)(struct net_device *dev);
-
-	void (*adjust_state)(struct net_device *dev);
 };
 #define to_phy_device(d) container_of(d, struct phy_device, dev)
 
@@ -585,8 +581,7 @@ int phy_drivers_register(struct phy_driv
 void phy_state_machine(struct work_struct *work);
 void phy_change(struct work_struct *work);
 void phy_mac_interrupt(struct phy_device *phydev, int new_link);
-void phy_start_machine(struct phy_device *phydev,
-		void (*handler)(struct net_device *));
+void phy_start_machine(struct phy_device *phydev);
 void phy_stop_machine(struct phy_device *phydev);
 int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd);
 int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd);

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 6/6] phylib: make phy_scan_fixups() static
  2014-01-04  1:13 [PATCH 0/6] phylib: cleanups Sergei Shtylyov
                   ` (4 preceding siblings ...)
  2014-01-04  1:21 ` [PATCH 5/6] phylib: remove unused adjust_state callback Sergei Shtylyov
@ 2014-01-04  1:23 ` Sergei Shtylyov
  2014-01-05  0:07   ` Sergei Shtylyov
  2014-01-04  1:28 ` [PATCH 0/6] phylib: cleanups Sergei Shtylyov
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04  1:23 UTC (permalink / raw)
  To: netdev

phy_scan_fixups() isn't and shouldn't be called by the drivers directly, so
unexport it. And since Florian Fainelli's recent patches, the function is only
called locally, so we can make it static as well.

---
 drivers/net/phy/phy_device.c |    3 +--
 include/linux/phy.h          |    1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

Index: net-next/drivers/net/phy/phy_device.c
===================================================================
--- net-next.orig/drivers/net/phy/phy_device.c
+++ net-next/drivers/net/phy/phy_device.c
@@ -124,7 +124,7 @@ static int phy_needs_fixup(struct phy_de
 }
 
 /* Runs any matching fixups for this phydev */
-int phy_scan_fixups(struct phy_device *phydev)
+static int phy_scan_fixups(struct phy_device *phydev)
 {
 	struct phy_fixup *fixup;
 
@@ -143,7 +143,6 @@ int phy_scan_fixups(struct phy_device *p
 
 	return 0;
 }
-EXPORT_SYMBOL(phy_scan_fixups);
 
 struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
 				     bool is_c45,
Index: net-next/include/linux/phy.h
===================================================================
--- net-next.orig/include/linux/phy.h
+++ net-next/include/linux/phy.h
@@ -597,7 +597,6 @@ int phy_register_fixup_for_id(const char
 		int (*run)(struct phy_device *));
 int phy_register_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask,
 		int (*run)(struct phy_device *));
-int phy_scan_fixups(struct phy_device *phydev);
 
 int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable);
 int phy_get_eee_err(struct phy_device *phydev);

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] phylib: cleanups
  2014-01-04  1:13 [PATCH 0/6] phylib: cleanups Sergei Shtylyov
                   ` (5 preceding siblings ...)
  2014-01-04  1:23 ` [PATCH 6/6] phylib: make phy_scan_fixups() static Sergei Shtylyov
@ 2014-01-04  1:28 ` Sergei Shtylyov
  2014-01-04 15:53 ` Richard Cochran
  2014-01-04 16:41 ` Sergei Shtylyov
  8 siblings, 0 replies; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04  1:28 UTC (permalink / raw)
  To: netdev, David Miller

On 01/04/2014 04:13 AM, Sergei Shtylyov wrote:

>     This patchset mainly cleans up the phylib's coding style but then also
> removes some historically unused stuff.

    Sclerosis strikes again: forgot to mention that the patches are against 
'net-next.git' repo.

> [1/6] phy: coding style fixes
> [2/6] mdio_bus: coding style fixes
> [3/6] phy: kill useless local variables
> [4/6] phy: kill excess code
> [5/6] phylib: remove unused adjust_state callback
> [6/6] phylib: make phy_scan_fixups() static

WBR, Sergei

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 3/6] phy: kill useless local variables
  2014-01-04  1:17 ` [PATCH 3/6] phy: kill useless local variables Sergei Shtylyov
@ 2014-01-04  7:34   ` Richard Cochran
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Cochran @ 2014-01-04  7:34 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: netdev

On Sat, Jan 04, 2014 at 04:17:39AM +0300, Sergei Shtylyov wrote:
> A number of functions (especially in phy.c) has local variables that were hardly
> needed in the first place -- remove them.

I scanned over this patch, and the 'improvement' is marginal if at
all, IMHO. The code was fine how it was.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/6] phy: kill excess code
  2014-01-04  1:19 ` [PATCH 4/6] phy: kill excess code Sergei Shtylyov
@ 2014-01-04  7:36   ` Richard Cochran
  2014-01-04 16:29     ` Sergei Shtylyov
       [not found]   ` <CAGVrzcadLFE-FWvgDQE5gkd6COZkGs2Q4SQdnOdKE4Deu-qa0A@mail.gmail.com>
  1 sibling, 1 reply; 24+ messages in thread
From: Richard Cochran @ 2014-01-04  7:36 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: netdev

On Sat, Jan 04, 2014 at 04:19:14AM +0300, Sergei Shtylyov wrote:
> Remove tens of lines of unnecessary code:
> 
> - kill empty lines between a function call and its result check;

...

> - kill excess empty lines.

Blank lines are not LOC. They can improve legibility.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/6] phy: kill excess code
  2014-01-04 16:29     ` Sergei Shtylyov
@ 2014-01-04 15:42       ` Richard Cochran
  2014-01-04 16:47         ` Sergei Shtylyov
  0 siblings, 1 reply; 24+ messages in thread
From: Richard Cochran @ 2014-01-04 15:42 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: netdev

On Sat, Jan 04, 2014 at 07:29:35PM +0300, Sergei Shtylyov wrote:
> 
>    Both kinds of empty lines are consistently avoided by all other
> code I had to read. They don't really improve anything.

But both your subject line and body talk about lines of code. Just
removing blank lines is not the same as removing "excess code."

Thanks,
Richard

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/6] phy: kill excess code
  2014-01-04 16:47         ` Sergei Shtylyov
@ 2014-01-04 15:51           ` Richard Cochran
  2014-01-05  0:50             ` Sergei Shtylyov
  0 siblings, 1 reply; 24+ messages in thread
From: Richard Cochran @ 2014-01-04 15:51 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: netdev

On Sat, Jan 04, 2014 at 07:47:33PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 01/04/2014 06:42 PM, Richard Cochran wrote:
> 
> >>    Both kinds of empty lines are consistently avoided by all other
> >>code I had to read. They don't really improve anything.
> 
> >But both your subject line and body talk about lines of code. Just
> >removing blank lines is not the same as removing "excess code."
> 
>    To me, all source file lines are code. So what you are
> suggesting, splitting the patch?

Yes, exactly that.

If the title reads, "fix whitespace errors" or "remove blank lines",
then I don't need to waste my time looking at it.

But if it reads, "remove excess code", then I want to make sure that
you don't go and delete something important.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] phylib: cleanups
  2014-01-04  1:13 [PATCH 0/6] phylib: cleanups Sergei Shtylyov
                   ` (6 preceding siblings ...)
  2014-01-04  1:28 ` [PATCH 0/6] phylib: cleanups Sergei Shtylyov
@ 2014-01-04 15:53 ` Richard Cochran
  2014-01-04 17:14   ` Sergei Shtylyov
  2014-01-04 16:41 ` Sergei Shtylyov
  8 siblings, 1 reply; 24+ messages in thread
From: Richard Cochran @ 2014-01-04 15:53 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: netdev

On Sat, Jan 04, 2014 at 04:13:06AM +0300, Sergei Shtylyov wrote:
> Hello.
> 
>    This patchset mainly cleans up the phylib's coding style but then also
> removes some historically unused stuff.

Why not put the author of the code onto CC?

Thanks,
Richard

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/6] phy: kill excess code
       [not found]   ` <CAGVrzcadLFE-FWvgDQE5gkd6COZkGs2Q4SQdnOdKE4Deu-qa0A@mail.gmail.com>
@ 2014-01-04 16:24     ` Sergei Shtylyov
  0 siblings, 0 replies; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04 16:24 UTC (permalink / raw)
  To: Florian Fainelli; +Cc: netdev

Hello.

On 01/04/2014 01:12 PM, Florian Fainelli wrote:

>  > Remove tens of lines of unnecessary code:

>  > - kill empty lines between a function call and its result check;

>  > - convert assignments to initializers;

>  > - kill useless assignments before *return*;

>  > - kill excess empty lines.

> I might be completely out of date on what checkpatch is picky about these days

    Yes, you probably are.

> but it seems to me like all of these changes would trigger new warnings, I

    No new warnings were added. You should really have checked before you 
spoken out.

> will take a closer look, but for now I don't see this patch making any
> reability improvements, quite the contrary.

    Apparently, tastes vary. To me, e.g. empty lines between the function call 
and result (error) check are a waste of space which is avoided by all other 
kernel code I had to read.

>  > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com
> <mailto:sergei.shtylyov@cogentembedded.com>>

>  > ---
>  >  drivers/net/phy/phy.c        |   31 ++++------------------------
>  >  drivers/net/phy/phy_device.c |   46
> +++++++------------------------------------
>  >  2 files changed, 13 insertions(+), 64 deletions(-)

    May I ask you not to quote the full patch if you're replying only to the 
header? Please show some respect to people who read your messages.

WBR, Sergei

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/6] phy: kill excess code
  2014-01-04  7:36   ` Richard Cochran
@ 2014-01-04 16:29     ` Sergei Shtylyov
  2014-01-04 15:42       ` Richard Cochran
  0 siblings, 1 reply; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04 16:29 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev

Hello.

On 01/04/2014 10:36 AM, Richard Cochran wrote:

>> Remove tens of lines of unnecessary code:

>> - kill empty lines between a function call and its result check;

> ...

>> - kill excess empty lines.

> Blank lines are not LOC. They can improve legibility.

    Both kinds of empty lines are consistently avoided by all other code I had 
to read. They don't really improve anything.

> Thanks,
> Richard

WBR, Sergei

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] phylib: cleanups
  2014-01-04  1:13 [PATCH 0/6] phylib: cleanups Sergei Shtylyov
                   ` (7 preceding siblings ...)
  2014-01-04 15:53 ` Richard Cochran
@ 2014-01-04 16:41 ` Sergei Shtylyov
  8 siblings, 0 replies; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04 16:41 UTC (permalink / raw)
  To: netdev

Hello.

On 01/04/2014 04:13 AM, Sergei Shtylyov wrote:

>     This patchset mainly cleans up the phylib's coding style but then also
> removes some historically unused stuff.

> [1/6] phy: coding style fixes
> [2/6] mdio_bus: coding style fixes

    <linux/phy.h> also causes checkpatch.pl errors/warnings/checks but I just 
forgot about it. I feel the series should be extended now...

> [3/6] phy: kill useless local variables
> [4/6] phy: kill excess code
> [5/6] phylib: remove unused adjust_state callback
> [6/6] phylib: make phy_scan_fixups() static

WBR, Sergei

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/6] phy: kill excess code
  2014-01-04 15:42       ` Richard Cochran
@ 2014-01-04 16:47         ` Sergei Shtylyov
  2014-01-04 15:51           ` Richard Cochran
  0 siblings, 1 reply; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04 16:47 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev

Hello.

On 01/04/2014 06:42 PM, Richard Cochran wrote:

>>     Both kinds of empty lines are consistently avoided by all other
>> code I had to read. They don't really improve anything.

> But both your subject line and body talk about lines of code. Just
> removing blank lines is not the same as removing "excess code."

    To me, all source file lines are code. So what you are suggesting, 
splitting the patch?

> Thanks,
> Richard

WBR, Sergei

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] phylib: cleanups
  2014-01-04 15:53 ` Richard Cochran
@ 2014-01-04 17:14   ` Sergei Shtylyov
  2014-01-04 18:53     ` Sergei Shtylyov
  0 siblings, 1 reply; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04 17:14 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev

Hello.

On 01/04/2014 06:53 PM, Richard Cochran wrote:

>>     This patchset mainly cleans up the phylib's coding style but then also
>> removes some historically unused stuff.

> Why not put the author of the code onto CC?

    Because get_maintainer.pl doesn't suggest it and because Andy Fleming 
seems no longer interested in maintaining his code written back in 2004.

> Thanks,
> Richard

WBR, Sergei

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] phylib: cleanups
  2014-01-04 17:14   ` Sergei Shtylyov
@ 2014-01-04 18:53     ` Sergei Shtylyov
  2014-01-05  8:31       ` Richard Cochran
  0 siblings, 1 reply; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-04 18:53 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev

On 01/04/2014 08:14 PM, Sergei Shtylyov wrote:

>>>     This patchset mainly cleans up the phylib's coding style but then also
>>> removes some historically unused stuff.

>> Why not put the author of the code onto CC?

>     Because get_maintainer.pl doesn't suggest it and because Andy Fleming
> seems no longer interested in maintaining his code written back in 2004.

    Besides, I don't have his current email address (the source code doesn't 
provide any).

>> Thanks,
>> Richard

WBR, Sergei

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 6/6] phylib: make phy_scan_fixups() static
  2014-01-04  1:23 ` [PATCH 6/6] phylib: make phy_scan_fixups() static Sergei Shtylyov
@ 2014-01-05  0:07   ` Sergei Shtylyov
  0 siblings, 0 replies; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-05  0:07 UTC (permalink / raw)
  To: netdev, David Miller

Hello.

On 01/04/2014 04:23 AM, Sergei Shtylyov wrote:

> phy_scan_fixups() isn't and shouldn't be called by the drivers directly, so
> unexport it. And since Florian Fainelli's recent patches, the function is only
> called locally, so we can make it static as well.

    I've even forgot to sign off this one, so reposting the whole series now, 
with 2 new patches...

WBR, Sergei

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/6] phy: kill excess code
  2014-01-04 15:51           ` Richard Cochran
@ 2014-01-05  0:50             ` Sergei Shtylyov
  0 siblings, 0 replies; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-05  0:50 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev

Hello.

On 01/04/2014 06:51 PM, Richard Cochran wrote:

>>>>     Both kinds of empty lines are consistently avoided by all other
>>>> code I had to read. They don't really improve anything.

>>> But both your subject line and body talk about lines of code. Just
>>> removing blank lines is not the same as removing "excess code."

>>     To me, all source file lines are code. So what you are
>> suggesting, splitting the patch?

> Yes, exactly that.

    OK, done.

> If the title reads, "fix whitespace errors" or "remove blank lines",
> then I don't need to waste my time looking at it.

    Maybe I'd go too far and remove too much? :-)

> But if it reads, "remove excess code", then I want to make sure that
> you don't go and delete something important.

    I don't delete the meaningful code at all, just kind of compact it. Maybe 
I should have renamed that patch...

> Thanks,
> Richard

WBR, Sergei

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] phylib: cleanups
  2014-01-04 18:53     ` Sergei Shtylyov
@ 2014-01-05  8:31       ` Richard Cochran
  2014-01-05 13:40         ` Sergei Shtylyov
  0 siblings, 1 reply; 24+ messages in thread
From: Richard Cochran @ 2014-01-05  8:31 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: netdev

On Sat, Jan 04, 2014 at 09:53:59PM +0300, Sergei Shtylyov wrote:
> 
> >    Because get_maintainer.pl doesn't suggest it and because Andy Fleming
> >seems no longer interested in maintaining his code written back in 2004.

It never hurts to ask.

Your patches are saying, "Andy, your code has poor style." I don't
agree, especially about #3. Also, maybe the deleted callback will
useful one day. After all, there are not that many phylib
drivers. Perhaps future MAC drivers will need this.
 
>    Besides, I don't have his current email address (the source code
> doesn't provide any).

Here, let me find that for you.

Thanks,
Richard
---
git log --author="Andy Fleming"

commit 39fd40274d1f3a52152ae6fc22f428d93f1a8363
Author: Andy Fleming <afleming@freescale.com>
Date:   Mon Aug 5 14:58:35 2013 -0500

    powerpc: Convert platforms to smp_generic_cpu_bootable
    
    T4, Cell, powernv, and pseries had the same implementation, so switch
    them to use a generic version. A2 apparently had a version, but
    removed it at some point, so we remove the declaration, too.
    
    Signed-off-by: Andy Fleming <afleming@freescale.com>
    Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

commit 3cd852502316d42e3e75859e92d9f0a952bb55a2
Author: Andy Fleming <afleming@freescale.com>
Date:   Mon Aug 5 14:58:34 2013 -0500

    powerpc: Add smp_generic_cpu_bootable
    
    Cell and PSeries both implemented their own versions of a
    cpu_bootable smp_op which do the same thing (well, the PSeries
    one has support for more than 2 threads). Copy the PSeries one
    to generic code, and rename it smp_generic_cpu_bootable.
    
    Signed-off-by: Andy Fleming <afleming@freescale.com>
    Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] phylib: cleanups
  2014-01-05  8:31       ` Richard Cochran
@ 2014-01-05 13:40         ` Sergei Shtylyov
  2014-01-06  8:35           ` Richard Cochran
  0 siblings, 1 reply; 24+ messages in thread
From: Sergei Shtylyov @ 2014-01-05 13:40 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev

On 05-01-2014 12:31, Richard Cochran wrote:

>>>     Because get_maintainer.pl doesn't suggest it and because Andy Fleming
>>> seems no longer interested in maintaining his code written back in 2004.

> It never hurts to ask.

    I would have probably asked if there were not so many obstacles to doing that.

> Your patches are saying, "Andy, your code has poor style."

    It's coming back from 2004, so maybe the style wasn't considered bad at 
that time...

> I don't
> agree, especially about #3.

    It's your right. However, v2 of the patchset has been applied already.

> Also, maybe the deleted callback will
> useful one day.

    We kept it for almost 10 years and it never got useful. As I understood 
DaveM's policy it is "we don't keep unused stuff".

> After all, there are not that many phylib drivers.

    Really? I find the number to be intimidating enough to not do the cleanups 
on them also in this same patchset. Anyway, the callback was intended for the 
Ethernet drivers.

> Perhaps future MAC drivers will need this.

    We can always add it back in that case. However, phy_start_machine() which 
is the only way to set the callback was not even exported, so not generally 
usable for the MAC drivers. I guess this facility wasn't well thought out from 
the start.

>>     Besides, I don't have his current email address (the source code
>> doesn't provide any).

> Here, let me find that for you.

    Thanks I found those eventually. However, my LinkedIn account told me that 
Andy left Freescale about that time (it also gave up his private email though).

> Thanks,
> Richard

WBR, Sergei

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] phylib: cleanups
  2014-01-05 13:40         ` Sergei Shtylyov
@ 2014-01-06  8:35           ` Richard Cochran
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Cochran @ 2014-01-06  8:35 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: netdev

On Sun, Jan 05, 2014 at 05:40:08PM +0400, Sergei Shtylyov wrote:
> On 05-01-2014 12:31, Richard Cochran wrote:
> >After all, there are not that many phylib drivers.
> 
>    Really? I find the number to be intimidating enough to not do the
> cleanups on them also in this same patchset. Anyway, the callback
> was intended for the Ethernet drivers.

I once made a list using cscope, in order find all phylib drivers that
are missing skb_tx_timestamp. Here is what I came up with at that
time. Discounting those files under 'phylib' and 'unlikely', I only
found 30 drivers.

Finding files #including file: linux/phy.h

* arm
  drivers/net/arm/ixp4xx_eth.c
  drivers/net/davinci_emac.c
  drivers/net/macb.c
  drivers/net/pxa168_eth.c
  drivers/net/ax88796.c
  drivers/net/smsc911x.c
* powerpc
  drivers/net/fec_mpc52xx.c
  drivers/net/fs_enet/fs_enet-main.c
  drivers/net/ll_temac_main.c
  drivers/net/mv643xx_eth.c
  drivers/net/ucc_geth.c
  drivers/net/xilinx_emaclite.c
* x86
  drivers/net/dnet.c
  drivers/net/ethoc.c
  drivers/net/fec.c
  drivers/net/r6040.c
  drivers/net/smsc9420.c
  drivers/net/stmmac/stmmac_main.c
  drivers/net/tg3.c
* mips
  drivers/net/au1000_eth.c
  drivers/net/bcm63xx_enet.h
  drivers/net/cpmac.c
  drivers/net/lantiq_etop.c
  drivers/net/octeon/octeon_mgmt.c
  drivers/net/sb1250-mac.c
  drivers/net/tc35815.c
* sparc
  drivers/net/greth.h
* superh
  drivers/net/sh_eth.c
  drivers/net/sh_eth.h
* xtensa
  drivers/net/s6gmac.c
* phylib
  drivers/net/phy/bcm63xx.c
  drivers/net/phy/broadcom.c
  drivers/net/phy/cicada.c
  drivers/net/phy/davicom.c
  drivers/net/phy/dp83640.c
  drivers/net/phy/et1011c.c
  drivers/net/phy/fixed.c
  drivers/net/phy/icplus.c
  drivers/net/phy/lxt.c
  drivers/net/phy/marvell.c
  drivers/net/phy/mdio-octeon.c
  drivers/net/phy/mdio_bus.c
  drivers/net/phy/micrel.c
  drivers/net/phy/national.c
  drivers/net/phy/phy.c
  drivers/net/phy/phy_device.c
  drivers/net/phy/qsemi.c
  drivers/net/phy/realtek.c
  drivers/net/phy/smsc.c
  drivers/net/phy/ste10Xp.c
  drivers/net/phy/vitesse.c
* unlikely
  drivers/net/pasemi_mac.h
  drivers/of/of_mdio.c
  drivers/staging/octeon/ethernet-mdio.c
  drivers/staging/octeon/ethernet-rgmii.c
  drivers/staging/octeon/ethernet.c
  include/linux/bfin_mac.h
  include/linux/fec.h
  include/linux/mdio-bitbang.h
  include/linux/of_mdio.h
  include/linux/smsc911x.h
  net/core/timestamping.c
  net/dsa/dsa_priv.h
  net/dsa/mv88e6060.c
  net/dsa/mv88e6123_61_65.c
  net/dsa/mv88e6131.c
  net/dsa/mv88e6xxx.c
  net/dsa/slave.c
 
>    Thanks I found those eventually. However, my LinkedIn account
> told me that Andy left Freescale about that time (it also gave up
> his private email though).

Okay, so you know more than I do.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2014-01-06  8:35 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-04  1:13 [PATCH 0/6] phylib: cleanups Sergei Shtylyov
2014-01-04  1:14 ` [PATCH 1/6] phy: coding style fixes Sergei Shtylyov
2014-01-04  1:16 ` [PATCH 2/6] mdio_bus: " Sergei Shtylyov
2014-01-04  1:17 ` [PATCH 3/6] phy: kill useless local variables Sergei Shtylyov
2014-01-04  7:34   ` Richard Cochran
2014-01-04  1:19 ` [PATCH 4/6] phy: kill excess code Sergei Shtylyov
2014-01-04  7:36   ` Richard Cochran
2014-01-04 16:29     ` Sergei Shtylyov
2014-01-04 15:42       ` Richard Cochran
2014-01-04 16:47         ` Sergei Shtylyov
2014-01-04 15:51           ` Richard Cochran
2014-01-05  0:50             ` Sergei Shtylyov
     [not found]   ` <CAGVrzcadLFE-FWvgDQE5gkd6COZkGs2Q4SQdnOdKE4Deu-qa0A@mail.gmail.com>
2014-01-04 16:24     ` Sergei Shtylyov
2014-01-04  1:21 ` [PATCH 5/6] phylib: remove unused adjust_state callback Sergei Shtylyov
2014-01-04  1:23 ` [PATCH 6/6] phylib: make phy_scan_fixups() static Sergei Shtylyov
2014-01-05  0:07   ` Sergei Shtylyov
2014-01-04  1:28 ` [PATCH 0/6] phylib: cleanups Sergei Shtylyov
2014-01-04 15:53 ` Richard Cochran
2014-01-04 17:14   ` Sergei Shtylyov
2014-01-04 18:53     ` Sergei Shtylyov
2014-01-05  8:31       ` Richard Cochran
2014-01-05 13:40         ` Sergei Shtylyov
2014-01-06  8:35           ` Richard Cochran
2014-01-04 16:41 ` Sergei Shtylyov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.