Re: [v9, 6/6] fsl/fman: Add FMan MAC driver

[Andy Fleming]

On Thu, Dec 3, 2015 at 1:19 AM,  <igal.liberman@xxxxxxxxxxxxx> wrote:
> From: Igal Liberman <igal.liberman@xxxxxxxxxxxxx>
>
> This patch adds the Ethernet MAC driver supporting the three
> different types of MACs: dTSEC, tGEC and mEMAC.
>
> Signed-off-by: Igal Liberman <igal.liberman@xxxxxxxxxxxxx>

[...]

> +
> +MODULE_LICENSE("Dual BSD/GPL");
> +
> +MODULE_AUTHOR("Emil Medve <Emilian.Medve@xxxxxxxxxxxxx>");

I imagine this email address doesn't exist anymore, or won't soon.
This is also an issue in the ethernet driver (with my old address).
I'm not sure what the right approach is, but we shouldn't be putting
obsolete email addresses in the kernel.

[...]

> +static void mac_exception(void *_mac_dev, enum fman_mac_exceptions ex)
> +{
> +       struct mac_device       *mac_dev;
> +       struct mac_priv_s       *priv;
> +
> +       mac_dev = (struct mac_device *)_mac_dev;


Don't cast a void *

[...]

> +static int mac_probe(struct platform_device *_of_dev)
> +{
> +       int                      err, i, lenp, nph;
> +       struct device           *dev;
> +       struct device_node      *mac_node, *dev_node, *tbi_node;
> +       struct mac_device       *mac_dev;
> +       struct platform_device  *of_dev;
> +       struct resource          res;
> +       struct mac_priv_s       *priv;
> +       const u8                *mac_addr;
> +       const char              *char_prop;

[...]

> +       /* Get the PHY connection type */
> +       char_prop = (const char *)of_get_property(mac_node,
> +                                               "phy-connection-type", NULL);
> +       if (!char_prop) {
> +               dev_warn(dev,
> +                        "of_get_property(%s, phy-connection-type) failed. Defaulting to MII\n",
> +                        mac_node->full_name);
> +               priv->phy_if = PHY_INTERFACE_MODE_MII;
> +       } else {
> +               priv->phy_if = str2phy(char_prop);
> +       }
> +
> +       priv->speed             = phy2speed[priv->phy_if];
> +       priv->max_speed         = priv->speed;
> +       mac_dev->if_support     = DTSEC_SUPPORTED;
> +       /* We don't support half-duplex in SGMII mode */
> +       if (strstr(char_prop, "sgmii"))

This causes a crash if the device tree does not have a
"phy-connection-type" for this node. Also, you already have parsed the
property, and put the result in priv->phy_if. Why not use this to do
all of these determinations?


> +               mac_dev->if_support &= ~(SUPPORTED_10baseT_Half |
> +                                       SUPPORTED_100baseT_Half);
> +
> +       /* Gigabit support (no half-duplex) */
> +       if (priv->max_speed == 1000)
> +               mac_dev->if_support |= SUPPORTED_1000baseT_Full;
> +
> +       /* The 10G interface only supports one mode */
> +       if (strstr(char_prop, "xgmii"))
> +               mac_dev->if_support = SUPPORTED_10000baseT_Full;

Here, too.


[...]

> +       err = mac_dev->init(mac_dev);
> +       if (err < 0) {
> +               dev_err(dev, "mac_dev->init() = %d\n", err);
> +               of_node_put(priv->phy_node);
> +               goto _return_dev_set_drvdata;
> +       }
> +
> +       /* pause frame autonegotiation enabled */
> +       mac_dev->autoneg_pause = true;
> +
> +       /* by intializing the values to false, force FMD to enable PAUSE frames
> +        * on RX and TX
> +        */

Minor comment format issue (leave blank line at top of block comment)

Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/