[RFC PATCH v5 3/3] net: phy: Add gmiitorgmii converter support

Hi Florian,

	Thanks for the review...

quoted

This converter sits between the MAC and the external phy MAC <==>
GMII2RGMII <==> RGMII_PHY

This looks good, just a few things, see below:

Thanks...

quoted

+config XILINX_GMII2RGMII
+       tristate "Xilinx GMII2RGMII converter driver"
+       default y

Don't force that, or at least make the default based on the potential
users/drivers here.

Ok sure will fix in the next version...

quoted

+       ---help---
+         This driver support xilinx GMII to RGMII IP core it provides
+         the Reduced Gigabit Media Independent Interface(RGMII) between
+         Ethernet physical media devices and the Gigabit Ethernet controller.
+
 endif # PHYLIB

<snip>

quoted

+#define XILINX_GMII2RGMII_REG		0x10
+#define XILINX_GMII2RGMII_SPEED_MASK	0x2040

BMCR_SPEED1000 | BMCR_SPEED100 would be clearer here.

Sure will fix...

quoted

+
+struct gmii2rgmii {
+	struct phy_device *phy_dev;
+	struct phy_driver *phy_drv;
+	struct phy_driver conv_phy_drv;
+	int addr;
+};
+
+static int xgmiitorgmii_read_status(struct phy_device *phydev) {
+	struct gmii2rgmii *priv = (struct gmii2rgmii *)phydev->priv;

Casting is not required here, priv is void *.

Ok will remove...

quoted

+	u16 val = 0;
+
+	priv->phy_drv->read_status(phydev);
+
+	val = mdiobus_read(phydev->mdio.bus, priv->addr,

XILINX_GMII2RGMII_REG);

quoted

+	val &= XILINX_GMII2RGMII_SPEED_MASK;
+
+	switch (phydev->speed) {
+	case SPEED_1000:
+		val |= BMCR_SPEED1000;

Is the fall through really intentional here? See genphy_setup_forced() for
instance.

Ok will fix...

quoted

+	case SPEED_100:
+		val |= BMCR_SPEED100;
+	case SPEED_10:
+		val |= BMCR_SPEED10;
+	}
+
+	mdiobus_write(phydev->mdio.bus, priv->addr,

XILINX_GMII2RGMII_REG,

quoted

+val);
+
+	return 0;
+}

[snip]

quoted

+static int __init xgmiitorgmii_init(void) {
+	return mdio_driver_register(&xgmiitorgmii_driver);
+}
+module_init(xgmiitorgmii_init);
+
+static void __exit xgmiitorgmii_cleanup(void) {
+	mdio_driver_unregister(&xgmiitorgmii_driver);
+}
+module_exit(xgmiitorgmii_cleanup);

mdio_module_driver() does eliminate a bit of this boilerplate code.

Sure will fix in the next version...

Regards,
Kedar.

--
Florian