RE: [RFC PATCH v4 2/2] net: phy: Add gmiitorgmii converter support

[Punnaiah Choudary Kalluri]

Hi Kedar,

> -----Original Message-----
> From: Kedareswara rao Appana [mailto:appana.durga.rao@xxxxxxxxxx]
> Sent: Monday, August 08, 2016 12:45 PM
> To: robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; Michal Simek
> <michals@xxxxxxxxxx>; Soren Brinkmann <sorenb@xxxxxxxxxx>; Appana
> Durga Kedareswara Rao <appanad@xxxxxxxxxx>; f.fainelli@xxxxxxxxx;
> andrew@xxxxxxx; Punnaiah Choudary Kalluri <punnaia@xxxxxxxxxx>;
> Anirudha Sarangi <anirudh@xxxxxxxxxx>
> Cc: devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> Subject: [RFC PATCH v4 2/2] net: phy: Add gmiitorgmii converter support
> 
> This patch adds support for gmiitorgmii converter.
> 
> The GMII to RGMII IP core provides the Reduced Gigabit Media
> Independent Interface (RGMII) between Ethernet physical media
> Devices and the Gigabit Ethernet controller. This core can
> Switch dynamically between the three different speed modes of
> Operation by configuring the converter register through mdio write.
> 
> MDIO interface is used to set operating speed of Ethernet MAC.
> 
> Signed-off-by: Kedareswara rao Appana <appanad@xxxxxxxxxx>
> ---
> Thanks a lot Andrew for your inputs.
> Changes for v4:
> --> Updated phydev speed for all 3 speeds as suggested by zhuyj.
> Changes for v3:
> --> Updated the driver as suggested by Andrew.
> Changes for v2:
> --> Passed struct xphy pointer directly to the fix_mac_speed
> API as suggested by the Florian.
> --> Added checks for the phy-node fail case as suggested
> by the Florian
> 
> +
> +#define XILINX_GMII2RGMII_REG	0x10
> +#define BMCR_SPEED10		0x00

Move this macro to mii.h

> +
> +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.
           

> +	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.

Regards,
Punnaiah

> +}
> +
> +static const struct of_device_id xgmiitorgmii_of_match[] = {
> +	{ .compatible = "xlnx,gmii-to-rgmii-1.0" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, xgmiitorgmii_of_match);
> +
> +static struct mdio_driver xgmiitorgmii_driver = {
> +	.probe	= xgmiitorgmii_probe,
> +	.mdiodrv.driver = {
> +		.name = "xgmiitorgmii",
> +		.of_match_table = xgmiitorgmii_of_match,
> +	},
> +};
> +
> +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);
> +
> +MODULE_DESCRIPTION("Xilinx GMII2RGMII converter driver");
> +MODULE_LICENSE("GPL");
> --
> 2.1.2