by Liberman Igal

[permalink] [raw]

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

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

Regards,
Igal Liberman

> -----Original Message-----
> From: Andy Fleming [mailto:[email protected]]
> Sent: Tuesday, December 08, 2015 10:18 PM
> To: Liberman Igal-B31950 <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; Wood Scott-B07421 <[email protected]>;
> Bucur Madalin-Cristian-B32716 <[email protected]>;
> [email protected]; Joakim Tjernlund <[email protected]>;
> [email protected]; [email protected]; David Miller
> <[email protected]>
> Subject: Re: [v9, 6/6] fsl/fman: Add FMan MAC driver
>
> On Thu, Dec 3, 2015 at 1:19 AM, <[email protected]> wrote:
> > From: Igal Liberman <[email protected]>
> >
> > This patch adds the Ethernet MAC driver supporting the three different
> > types of MACs: dTSEC, tGEC and mEMAC.
> >
> > Signed-off-by: Igal Liberman <[email protected]>
>
> [...]
>
> > +
> > +MODULE_LICENSE("Dual BSD/GPL");
> > +
> > +MODULE_AUTHOR("Emil Medve <[email protected]>");
>
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?