Re: [PATCH net-next v2 9/9] Altera TSE: Add main and header file for Altera Ethernet Driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 11, 2014 at 4:44 AM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> 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.

Double doh! Will address, thank you.

>
> [...]
>
>> +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.

Doh! Will address, thank you.

>
> [...]
>
>> +       /* 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.

Will do, thank you.

>
>> +
>> +       /* 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.

No reason, an oversight. I'll address.

>
> Cheers,
> Mark.

Thank you for taking the time to review and comment. I'll address the
comments and respin the patch.

All the best,

Vince
--
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




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux