Le 18/12/2015 01:30, shh.xie@gmail.com a écrit :
[snip]

+struct training_state_machine {
+	bool bin_m1_late_early;
+	bool bin_long_late_early;
+	bool bin_m1_stop;
+	bool bin_long_stop;
+	bool tx_complete;
+	bool an_ok;
+	bool link_up;
+	bool running;
+	bool sent_init;
+	int m1_min_max_cnt;
+	int long_min_max_cnt;

Can you change this into a succession of enum values to make it clear
how many states there are, and how to get from one to the other?

[snip]

+
+static void init_state_machine(struct training_state_machine *s_m)
+{
+	s_m->bin_m1_late_early = true;
+	s_m->bin_long_late_early = false;
+	s_m->bin_m1_stop = false;
+	s_m->bin_long_stop = false;
+	s_m->tx_complete = false;
+	s_m->an_ok = false;
+	s_m->link_up = false;
+	s_m->running = false;
+	s_m->sent_init = false;
+	s_m->m1_min_max_cnt = 0;
+	s_m->long_min_max_cnt = 0;

That alone is a good indication that your state machine is not readable.

+}
+
+void tune_tecr0(struct fsl_xgkr_inst *inst)
+{
+	struct per_lane_ctrl_status *reg_base;
+	u32 val;
+
+	reg_base = (struct per_lane_ctrl_status *)inst->reg_base;

Typical practice is not to cast a register base address into a pointer
to a structure, but instead use offsets to the base register address,
which is less error prone and more readable, anything that uses
inst->reg_base in this driver is modeled after a structure pattern,
please fix that.

+
+	val = TECR0_INIT |
+		inst->adpt_eq << ZERO_COE_SHIFT |
+		inst->ratio_preq << PRE_COE_SHIFT |
+		inst->ratio_pst1q << POST_COE_SHIFT;
+
+	/* reset the lane */
+	iowrite32be(ioread32be(&reg_base->gcr0) & ~GCR0_RESET_MASK,
+		    &reg_base->gcr0);
+	udelay(1);
+	iowrite32be(val, &reg_base->tecr0);
+	udelay(1);
+	/* unreset the lane */
+	iowrite32be(ioread32be(&reg_base->gcr0) | GCR0_RESET_MASK,
+		    &reg_base->gcr0);
+	udelay(1);

Since this is a reset control register, you might want to explore using
the reset controller subsystem in case this register serves multiple
peripherals.

[snip]

I skipped through all the state machine logic, but you should consider
trying to simplify it using different enum values and a switch() case()
statements to make it easy to audit and understand.

+
+static int fsl_backplane_probe(struct phy_device *phydev)
+{
+	struct fsl_xgkr_inst *xgkr_inst;
+	struct device_node *child, *parent, *lane_node;
+	const char *lane_name;
+	int len;
+	int ret;
+	u32 mode;
+	u32 lane[2];
+
+	child = phydev->dev.of_node;
+	parent = of_get_parent(child);
+	if (!parent) {
+		dev_err(&phydev->dev, "could not get parent node");
+		return 0;
+	}
+
+	lane_name = of_get_property(parent, "lane-instance", &len);
+	if (!lane_name)
+		return 0;
+
+	if (strstr(lane_name, "1000BASE-KX"))
+		mode = BACKPLANE_1G_KX;
+	else
+		mode = BACKPLANE_10G_KR;

That seems like I could put whatever value in "lane-instance" and as
long as it contains 1000BASE-KX we are golden, that does not sound very
robust nor well defined.

+
+	lane_node = of_parse_phandle(child, "lane-handle", 0);
+	if (lane_node) {

Treat this as an error so you can return early and reduce indentation.

+		struct resource res_lane;
+
+		ret = of_address_to_resource(lane_node, 0, &res_lane);
+		if (ret) {
+			dev_err(&phydev->dev, "could not obtain memory map\n");
+			return ret;
+		}
+
+		ret = of_property_read_u32_array(child, "lane-range",
+						 (u32 *)&lane, 2);
+		if (ret) {
+			dev_info(&phydev->dev, "could not get lane-range\n");
+			return -EINVAL;
+		}

Is not the standard "ranges" property usable here?

+
+		phydev->priv = devm_ioremap_nocache(&phydev->dev,
+						    res_lane.start + lane[0],
+						    lane[1]);

What about the "reg" property here?

+
+		if (!phydev->priv) {
+			of_node_put(lane_node);
+			return -EINVAL;
+		}
+		of_node_put(lane_node);
+	} else {
+		return -EINVAL;
+	}
+
+	if (mode == BACKPLANE_1G_KX) {
+		phydev->speed = SPEED_1000;
+		/* configure the lane for 1000BASE-KX */
+		lane_set_1gkx(phydev->priv);
+		return 0;
+	}
+
+	xgkr_inst = kzalloc(sizeof(*xgkr_inst), GFP_KERNEL);
+	if (!xgkr_inst)
+		goto mem_err1;

devm_kzalloc()?

+
+	xgkr_inst->reg_base = phydev->priv;
+	xgkr_inst->bus = phydev->bus;
+	xgkr_inst->phydev = phydev;
+	phydev->priv = xgkr_inst;
+
+	if (mode == BACKPLANE_10G_KR) {
+		phydev->speed = SPEED_10000;
+		init_inst(xgkr_inst, 1);
+		INIT_DELAYED_WORK(&xgkr_inst->xgkr_wk, xgkr_wq_state_machine);
+	}
+
+	return 0;
+mem_err1:
+	dev_err(&phydev->dev, "failed to allocate memory\n");
+	return -ENOMEM;
+}
+
+static int fsl_backplane_aneg_done(struct phy_device *phydev)
+{
+	return 1;
+}
+
+static int fsl_backplane_config_aneg(struct phy_device *phydev)
+{
+	struct fsl_xgkr_inst *xgkr_inst = (struct fsl_xgkr_inst *)phydev->priv;

No casting needed from void *

[snip]

+
+static int fsl_backplane_read_status(struct phy_device *phydev)
+{
+	int val;
+
+	phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
+
+	if (val & AN_LNK_UP_MASK)
+		phydev->link = 1;
+	else
+		phydev->link = 0;

This sounds fairly generic, candidate for a helper function?


-- 
Florian