netdev - Re: [PATCH net-next v2 07/10] net: bcmgenet: add MDIO routines

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, 13 Feb 2014 11:50:08 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	Florian Fainelli <f.fainelli@...il.com>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"cernekee@...il.com" <cernekee@...il.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH net-next v2 07/10] net: bcmgenet: add MDIO routines

On Thu, Feb 13, 2014 at 05:29:52AM +0000, Florian Fainelli wrote:
> This patch adds support for configuring the port multiplexer hardware
> which resides in front of the GENET Ethernet MAC controller. This allows
> us to support:
> 
> - internal PHYs (using drivers/net/phy/bcm7xxx.c)
> - MoCA PHYs which are an entirely separate hardware block not covered
>   here
> - external PHYs and switches
> 
> Note that MoCA and switches are currently supported using the emulated
> "fixed PHY" driver.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
> ---
> Changes since v1:
> - fixed MDIO crash/warning when Device Tree probing fails
> - removed the use of priv->phy_type and use priv->phy_interface
>   directly
> 
>  drivers/net/ethernet/broadcom/genet/bcmmii.c | 481 +++++++++++++++++++++++++++
>  1 file changed, 481 insertions(+)
>  create mode 100644 drivers/net/ethernet/broadcom/genet/bcmmii.c

[...]

> +static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
> +{
> +       struct device_node *dn = priv->pdev->dev.of_node;
> +       struct device *kdev = &priv->pdev->dev;
> +       struct device_node *mdio_dn;
> +       const __be32 *fixed_link;

This looks a bit odd. Could we not have a common parser for fixed-link
properties?

> +       u32 propval;
> +       int ret, sz;
> +
> +       mdio_dn = of_get_next_child(dn, NULL);
> +       if (!mdio_dn) {
> +               dev_err(kdev, "unable to find MDIO bus node\n");
> +               return -ENODEV;
> +       }

Could you please check that this is the node you expect (by looking at
the compatible string list).

> +
> +       ret = of_mdiobus_register(priv->mii_bus, mdio_dn);
> +       if (ret) {
> +               dev_err(kdev, "failed to register MDIO bus\n");
> +               return ret;
> +       }
> +
> +       /* Check if we have an internal or external PHY */
> +       priv->phy_dn = of_parse_phandle(dn, "phy-handle", 0);
> +       if (priv->phy_dn) {
> +               if (!of_property_read_u32(priv->phy_dn, "max-speed", &propval))
> +                       priv->phy_speed = propval;

Is there no way to find this out without reading values directly off of
the PHY? It seems like something we should have an abstraction for.

> +       } else {
> +               /* Read the link speed from the fixed-link property */
> +               fixed_link = of_get_property(dn, "fixed-link", &sz);
> +               if (!fixed_link || sz < sizeof(*fixed_link)) {
> +                       ret = -ENODEV;
> +                       goto out;
> +               }
> +
> +               priv->phy_speed = be32_to_cpu(fixed_link[2]);

Similarly can we not have a common fixed-link parser? Or abstraction
such that you query the phy regardless of what it is and how its binding
represents this?

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html