On Mon, Mar 10, 2014 at 11:14:37PM +0000, Vince Bridgers wrote:
> This patch adds the main driver and header file for the Altera Triple
> Speed Ethernet driver.
>
> Signed-off-by: Vince Bridgers <vbridgers2013@xxxxxxxxx>
> ---
> V2: - Address all comments from initial RFC (thanks Florian!)
> - Moved asm/cacheflush.h to bottom of header includes
> - Remove barriers since DMA APIs ensure memory consistency
> - Add max-frame-size property
> - Use accessor function of_get_phy_mode instead of custom function
> - Remove custom function to access devicetree, use of APIs
> - Deprecate use of Altera devicetree properties where standard
> ones exist
> - Remove use of netif_carrier_off - not necessary where it was used
> - Move probing of phy to the probe routine instead of open
> - use ether_addr_copy instead of memcpy
> ---
> drivers/net/ethernet/altera/altera_tse.h | 486 ++++++++
> drivers/net/ethernet/altera/altera_tse_main.c | 1546 +++++++++++++++++++++++++
> 2 files changed, 2032 insertions(+)
> create mode 100644 drivers/net/ethernet/altera/altera_tse.h
> create mode 100644 drivers/net/ethernet/altera/altera_tse_main.c
[...]
> +static int altera_tse_mdio_create(struct net_device *dev, unsigned int id)
> +{
> + struct altera_tse_private *priv = netdev_priv(dev);
> + int ret;
> + int i;
> + struct device_node *mdio_node;
> + struct mii_bus *mdio;
> +
> + mdio_node = of_find_compatible_node(priv->device->of_node, NULL,
> + "altr,tse-mdio");
This doesn't find a subnode, as the binding requires. This finds _any_
compatible node in the tree.
There doesn't seem to be an of_find_compatible_child_node, but you can
use for_each_child_of_node and of_device_is_compatible to achieve the
same effect.
[...]
> +static struct phy_device *connect_local_phy(struct net_device *dev)
> +{
> + struct altera_tse_private *priv = netdev_priv(dev);
> + struct phy_device *phydev = NULL;
> + char phy_id_fmt[MII_BUS_ID_SIZE + 3];
> + int ret;
> +
> + if (priv->phy_addr != POLL_PHY) {
> + snprintf(phy_id_fmt, MII_BUS_ID_SIZE + 3, PHY_ID_FMT,
> + priv->mdio->id, priv->phy_addr);
> +
> + dev_dbg(priv->device, "trying to attach to %s\n", phy_id_fmt);
> +
> + phydev = phy_connect(dev, phy_id_fmt, &altera_tse_adjust_link,
> + priv->phy_iface);
> + if (IS_ERR(phydev))
> + netdev_err(dev, "Could not attach to PHY\n");
> +
> + } else {
> + phydev = phy_find_first(priv->mdio);
> + if (phydev == NULL) {
> + netdev_err(dev, "No PHY found\n");
> + return phydev;
> + }
> +
> + ret = phy_connect_direct(dev, phydev, &altera_tse_adjust_link,
> + priv->phy_iface);
> + if (ret != 0) {
> + netdev_err(dev, "Could not attach to PHY\n");
> + phydev = NULL;
> + }
> + }
> + return phydev;
> +}
> +
> +/* Initialize driver's PHY state, and attach to the PHY
> + */
> +static int init_phy(struct net_device *dev)
> +{
> + struct altera_tse_private *priv = netdev_priv(dev);
> + struct phy_device *phydev;
> + struct device_node *phynode;
> +
> + priv->oldlink = 0;
> + priv->oldspeed = 0;
> + priv->oldduplex = -1;
> +
> + phynode = of_parse_phandle(priv->device->of_node, "phy-handle", 0);
> +
> + if (!phynode) {
> + netdev_dbg(dev, "no phy-handle found\n");
> + if (!priv->mdio) {
> + netdev_err(dev,
> + "No phy-handle nor local mdio specified\n");
> + return -ENODEV;
> + }
> + phydev = connect_local_phy(dev);
> + } else {
> + netdev_dbg(dev, "phy-handle found\n");
> + phydev = of_phy_connect(dev, phynode,
> + &altera_tse_adjust_link, 0, priv->phy_iface);
> + }
Be aware that of_phy_connect can return NULL (e.g. if the phy-handle
phandle pointed to a node that wasn't a phy). You'll want to check it's
not NULL before dereferencing it below.
The same seems to be true of connect_local_phy, so the check can be
shared.
[...]
> + /* get hash filter settings for this instance */
> + if (of_property_read_u32(pdev->dev.of_node, "altr,enable-hash",
> + &priv->hash_filter)) {
> + dev_err(&pdev->dev, "cannot obtain altr,enable-hash\n");
> + priv->hash_filter = 0;
> + }
> +
> + /* get supplemental address settings for this instance */
> + if (of_property_read_u32(pdev->dev.of_node, "altr,enable-sup-addr",
> + &priv->added_unicast)) {
> + dev_err(&pdev->dev, "cannot obtain altr,enable-sup-addr\n");
> + priv->added_unicast = 0;
> + }
Please change these to boolean/empty properties and use
of_property_read_bool.
> +
> + /* Max MTU is 1500, ETH_DATA_LEN */
> + priv->max_mtu = ETH_DATA_LEN;
> +
> + /* Get the max mtu from the device tree. Note that the
> + * "max-frame-size" parameter is actually max mtu. Definition
> + * in the ePAPR v1.1 spec and usage differ, so go with usage.
> + */
> + of_property_read_u32(pdev->dev.of_node, "max-frame-size",
> + &priv->max_mtu);
> +
> + /* The DMA buffer size already accounts for an alignment bias
> + * to avoid unaligned access exceptions for the NIOS processor,
> + */
> + priv->rx_dma_buf_sz = ALTERA_RXDMABUFFER_SIZE;
> +
> + /* get default MAC address from device tree */
> + macaddr = of_get_property(pdev->dev.of_node, "local-mac-address", &len);
> + if (macaddr && len == ETH_ALEN)
> + ether_addr_copy(ndev->dev_addr, macaddr);
Any reason we couldn't use of_get_mac_address here? It also checks that
the address is valid.
Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
