From: Liberman Igal <Igal.Liberman@freescale.com>
To: Andy Fleming <afleming@gmail.com>
CC: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Scott Wood <scottwood@freescale.com>,
        Madalin-Cristian Bucur <madalin.bucur@freescale.com>,
        "pebolle@tiscali.nl" <pebolle@tiscali.nl>,
        Joakim Tjernlund <joakim.tjernlund@transmode.se>,
        "ppc@mindchasers.com" <ppc@mindchasers.com>,
        "stephen@networkplumber.org" <stephen@networkplumber.org>,
        David Miller <davem@davemloft.net>
Subject: RE: [v9, 6/6] fsl/fman: Add FMan MAC driver
Thread-Topic: [v9, 6/6] fsl/fman: Add FMan MAC driver
Thread-Index: AQHRLbsquh8t2gL+4EKqwOHuND14I57Bj64AgApvxJA=
Date: Tue, 15 Dec 2015 11:55:57 +0000
Message-ID: <DM2PR03MB50969DA7ABC7D4641BCF1C7E6EE0@DM2PR03MB509.namprd03.prod.outlook.com>
References: <1449127157-9400-1-git-send-email-igal.liberman@freescale.com>
	<1449127157-9400-7-git-send-email-igal.liberman@freescale.com>
 <CAKWjMd7EDx8OnYGu-QJXM++fy=W5i2E74Fe4r9zNjUV_giyUKw@mail.gmail.com>
In-Reply-To: <CAKWjMd7EDx8OnYGu-QJXM++fy=W5i2E74Fe4r9zNjUV_giyUKw@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Dec 2015 11:55:58.0077
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 710a03f5-10f6-4d38-9ff4-a80b81da590d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR03MB1483
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id tBFBuCFS000776
Content-Length: 5400
Lines: 158

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

Regards,
Igal Liberman

> -----Original Message-----
> From: Andy Fleming [mailto:afleming@gmail.com]
> Sent: Tuesday, December 08, 2015 10:18 PM
> To: Liberman Igal-B31950 <Igal.Liberman@freescale.com>
> Cc: netdev@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; Wood Scott-B07421 <scottwood@freescale.com>;
> Bucur Madalin-Cristian-B32716 <madalin.bucur@freescale.com>;
> pebolle@tiscali.nl; Joakim Tjernlund <joakim.tjernlund@transmode.se>;
> ppc@mindchasers.com; stephen@networkplumber.org; David Miller
> <davem@davemloft.net>
> Subject: Re: [v9, 6/6] fsl/fman: Add FMan MAC driver
> 
> On Thu, Dec 3, 2015 at 1:19 AM,  <igal.liberman@freescale.com> wrote:
> > From: Igal Liberman <igal.liberman@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.liberman@freescale.com>
> 
> [...]
> 
> > +
> > +MODULE_LICENSE("Dual BSD/GPL");
> > +
> > +MODULE_AUTHOR("Emil Medve <Emilian.Medve@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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?