Re: [PATCH] net: phy: adds backplane driver for Freescale's PCS PHY

[Florian Fainelli <f.fainelli@gmail.com>]

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