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

[Liberman Igal]

Hello Andy,
Thank you for your feedback. Some inline answers.

Regards,
Igal Liberman

> -----Original Message-----
> From: Andy Fleming [mailto:afleming@xxxxxxxxx]
> Sent: Tuesday, December 08, 2015 10:18 PM
> To: Liberman Igal-B31950 <Igal.Liberman@xxxxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; linuxppc-dev@xxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Wood Scott-B07421 <scottwood@xxxxxxxxxxxxx>;
> Bucur Madalin-Cristian-B32716 <madalin.bucur@xxxxxxxxxxxxx>;
> pebolle@xxxxxxxxxx; Joakim Tjernlund <joakim.tjernlund@xxxxxxxxxxxx>;
> ppc@xxxxxxxxxxxxxxx; stephen@xxxxxxxxxxxxxxxxxx; David Miller
> <davem@xxxxxxxxxxxxx>
> Subject: Re: [v9, 6/6] fsl/fman: Add FMan MAC driver
> 
> 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.
> 
> [...]
> 

I removed MODULE_AUTHOR as per Scott's request.
We'll change the way we note contributors.

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

OK, removed.

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

Agree, changed the driver to use priv->phy_if instead of strstr in both places.
In both 

> 
> > +               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)
> 

According to https://www.kernel.org/doc/Documentation/CodingStyle:

The preferred style for long (multi-line) comments is:

	/*
	 * This is the preferred style for multi-line
	 * comments in the Linux kernel source code.
	 * Please use it consistently.
	 *
	 * Description:  A column of asterisks on the left side,
	 * with beginning and ending almost-blank lines.
	 */

For files in net/ and drivers/net/ the preferred style for long (multi-line)
comments is a little different.

	/* The preferred comment style for files in net/ and drivers/net
	 * looks like this.
	 *
	 * It is nearly the same as the generally preferred comment style,
	 * but there is no initial almost-blank line.
	 */

> Andy
N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå