netdev - Re: [PATCH v2 net-next 1/2] net: phy: Add Edge-rate driver for 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]

Date:   Thu, 8 Sep 2016 15:14:15 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Raju Lakkaraju <Raju.Lakkaraju@...rosemi.com>
Cc:     netdev@...r.kernel.org, f.fainelli@...il.com,
        Allan.Nielsen@...rosemi.com
Subject: Re: [PATCH v2 net-next 1/2] net: phy: Add Edge-rate driver for
 Microsemi PHYs.

On Thu, Sep 08, 2016 at 02:47:21PM +0530, Raju Lakkaraju wrote:
> From: Raju Lakkaraju <Raju.Lakkaraju@...rosemi.com>
> 
> Used Device Tree to configure the Edge-rate as per review comments and
> re-sending code for review
> 
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@...rosemi.com>
> ---
>  drivers/net/phy/mscc.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++

Hi Raju

You need to also document the new property in the device tree binding
documentation.

> +static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
> +				      u8     edge_rate)

No spaces place.

> +#ifdef CONFIG_OF_MDIO
> +static int vsc8531_of_init(struct phy_device *phydev)
> +{
> +	int rc;
> +	struct vsc8531_private *vsc8531 = phydev->priv;
> +	struct device *dev = &phydev->mdio.dev;
> +	struct device_node *of_node = dev->of_node;
> +
> +	if (!of_node)
> +		return -ENODEV;
> +
> +	rc = of_property_read_u8(of_node, "vsc8531,edge-rate",
> +				 &vsc8531->edge_rate);

Until you have written the Documentation, it is hard for me to tell,
but device tree bindings should use real units, like seconds, Ohms,
Farads, etc. Is the edge rate in nS? Or is it some magic value which
just gets written into the register?

> +
> +	return rc;
> +}
> +#else
> +static int vsc8531_of_init(struct phy_device *phydev)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_OF_MDIO */
> +
>  static int vsc85xx_config_init(struct phy_device *phydev)
>  {
>  	int rc;
> +	struct vsc8531_private *vsc8531;
> +
> +	if (!phydev->priv) {

How can this happen?

> +		vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531),
> +				       GFP_KERNEL);
> +		if (!vsc8531)
> +			return -ENOMEM;
> +
> +		phydev->priv = vsc8531;
> +		rc = vsc8531_of_init(phydev);
> +		if (rc)
> +			return rc;
> +	} else {
> +		vsc8531 = (struct vsc8531_private *)phydev->priv;
> +	}
>  
>  	rc = vsc85xx_default_config(phydev);
>  	if (rc)
>  		return rc;
> +
> +	rc = vsc85xx_edge_rate_cntl_set(phydev, vsc8531->edge_rate);

If there is no vsc8531,edge-rate property in device tree, is the phy
going to work O.K, if you configure it for 0nS edges? Or should there
be some default value assigned?

Thanks
	Andrew