Hi Punnaiah, Thanks for the review... > > + > > +#define XILINX_GMII2RGMII_REG 0x10 > > +#define BMCR_SPEED10 0x00 > > Move this macro to mii.h Sure will fix in the next version... > > > + > > +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; > > + u16 val = 0; > > + > > + priv->phy_drv->read_status(phydev); > > + > > + val = mdiobus_read(phydev->mdio.bus, priv->addr, > > XILINX_GMII2RGMII_REG); > > + > > Since its read and then modify, you should mask the speed field With zero and > then write new value. > Sure will fix in the next version... > > > + switch (phydev->speed) { > > + case SPEED_1000: > > + val |= BMCR_SPEED1000; > > + case SPEED_100: > > + val |= BMCR_SPEED100; > > + case SPEED_10: > > + val |= BMCR_SPEED10; > > + } > > + > > + mdiobus_write(phydev->mdio.bus, priv->addr, > > XILINX_GMII2RGMII_REG, val); > > + > > + return 0; > > +} > > + > > +int xgmiitorgmii_probe(struct mdio_device *mdiodev) { > > + struct device *dev = &mdiodev->dev; > > + struct device_node *np = dev->of_node, *phy_node; > > + struct gmii2rgmii *priv; > > + int ret; > > + > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + phy_node = of_parse_phandle(np, "phy-handle", 0); > > + if (IS_ERR(phy_node)) { > > + dev_err(dev, "Couldn't parse phy-handle\n"); > > + ret = -ENODEV; > > + goto out; > > + } > > + > > + priv->phy_dev = of_phy_find_device(phy_node); > > + if (!priv->phy_dev) { > > + ret = -EPROBE_DEFER; > > + dev_info(dev, "Couldn't find phydev\n"); > > + goto out; > > + } > > + > > + priv->addr = mdiodev->addr; > > + priv->phy_drv = priv->phy_dev->drv; > > + memcpy(&priv->conv_phy_drv, priv->phy_dev->drv, > > + sizeof(struct phy_driver)); > > + priv->conv_phy_drv.read_status = xgmiitorgmii_read_status; > > + priv->phy_dev->priv = priv; > > + priv->phy_dev->drv = &priv->conv_phy_drv; > > + > > + return 0; > > +out: > > + return ret; > > Since there is no resource cleaning here, you could consider Return from this > function in above conditions and avoid goto here. > Sure will fix in the next version... Regards, Kedar. > Regards, > Punnaiah -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html