Hello Andy, Thank you for your feedback. Some inline answers. Regards, Igal Liberman
> -----Original Message----- > From: Andy Fleming [mailto:aflem...@gmail.com] > Sent: Tuesday, December 08, 2015 10:18 PM > To: Liberman Igal-B31950 <igal.liber...@freescale.com> > Cc: netdev@vger.kernel.org; linuxppc-...@lists.ozlabs.org; linux- > ker...@vger.kernel.org; Wood Scott-B07421 <scottw...@freescale.com>; > Bucur Madalin-Cristian-B32716 <madalin.bu...@freescale.com>; > pebo...@tiscali.nl; Joakim Tjernlund <joakim.tjernl...@transmode.se>; > p...@mindchasers.com; step...@networkplumber.org; David Miller > <da...@davemloft.net> > Subject: Re: [v9, 6/6] fsl/fman: Add FMan MAC driver > > On Thu, Dec 3, 2015 at 1:19 AM, <igal.liber...@freescale.com> wrote: > > From: Igal Liberman <igal.liber...@freescale.com> > > > > This patch adds the Ethernet MAC driver supporting the three different > > types of MACs: dTSEC, tGEC and mEMAC. > > > > Signed-off-by: Igal Liberman <igal.liber...@freescale.com> > > [...] > > > + > > +MODULE_LICENSE("Dual BSD/GPL"); > > + > > +MODULE_AUTHOR("Emil Medve <emilian.me...@freescale.com>"); > > 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�����r��y����b�X��ǧv�^�){.n�+���z�^�)����w* jg��� �����ݢj/���z�ޖ��2�ޙ����&�)ߡ�a���� �G���h��j:+v���w��٥
