netdev - Re: [PATCH net-next v9 1/1] net: phy: Cleanup the Edge-Rate feature in Microsemi PHYs.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161009194534.GA20767@microsemi.com>
Date:   Sun, 9 Oct 2016 21:45:35 +0200
From:   "Allan W. Nielsen" <allan.nielsen@...rosemi.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     <netdev@...r.kernel.org>, <f.fainelli@...il.com>,
        <raju.lakkaraju@...rosemi.com>
Subject: Re: [PATCH net-next v9 1/1] net: phy: Cleanup the Edge-Rate feature
 in Microsemi PHYs.

Hi,

On 07/10/16 22:22, Andrew Lunn wrote:
> Overall, this is much better. I just have a few nitpicks.
It is good to hear that we are getting closer ;-)

> dt-bindings/net/mscc-phy-vsc8531.h is removed by this patch. It would
> be good to also remove the reference.
My bad.

> You don't need edge_slowdown and vddmac in the private structure,
> since they are never used after determining what rate_magic is.
Year... I was actually a bit confused about this... But assumed that you had
some conventions about saving "input" configuration.

Well, clearly not - I will clean this up.

Actually, there is no need to store rate_magic either... It is only used in the
init function. This means that there is no need for private date...

The code could look somthing like this (it is not tested, so just look at see if
you like the idea):

#ifdef CONFIG_OF_MDIO
static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
{
	int rc;
	u8 sd;
	u16 vdd;
	struct device *dev = &phydev->mdio.dev;
	struct device_node *of_node = dev->of_node;
	u8 sd_array_size = ARRAY_SIZE(edge_table[0].slowdown);

	if (!of_node)
		return -ENODEV;

	rc = of_property_read_u16(of_node, "vsc8531,vddmac", &vdd);
	if (rc != 0)
		vdd = MSCC_VDDMAC_3300;

	rc = of_property_read_u8(of_node, "vsc8531,edge-slowdown", &sd);
	if (rc != 0)
		sd = 0;

	for (vdd = 0; vdd < ARRAY_SIZE(edge_table); vdd++)
		if (edge_table[vdd].vddmac == vddmac)
			for (sd = 0; sd < sd_array_size; sd++)
				if (edge_table[vdd].slowdown[sd] == slowdown)
					return (sd_array_size - sd - 1);

	return -EINVAL;
}
#else
static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
{
	return 0;
}
#endif /* CONFIG_OF_MDIO */


static int vsc85xx_config_init(struct phy_device *phydev)
{
	int rc, rate_magic;
...
	rate_magic = vsc85xx_edge_rate_magic_get(phydev);
	if (rate_magic < 0)
		return rate_magic;

	rc = vsc85xx_edge_rate_cntl_set(phydev, rate_magic);
	if (rc)
		return rc;
...
}

This is clearly how I would prefere it, as it would simplify the implementation.

It should be no problem to have this tested, and have a new patch avialable
tomorrow.

/Allan