Re: [RFC net-next PATCH v2 1/2] net/fsl: add ACPI support for mdio bus - Russell King

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Calvin Johnson <calvin.johnson@oss.nxp.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	"Rajesh V . Bikkina" <rajesh.bikkina@nxp.com>,
	Madalin Bucur <madalin.bucur@oss.nxp.com>,
	netdev@vger.kernel.org, Pankaj Bansal <pankaj.bansal@nxp.com>,
	linux-kernel@vger.kernel.org,
	Jeremy Linton <jeremy.linton@arm.com>,
	Marcin Wojtas <mw@semihalf.com>,
	Diana Madalina Craciun <diana.craciun@nxp.com>,
	linux-acpi@vger.kernel.org,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	Cristi Sovaiala <cristian.sovaiala@nxp.com>,
	Varun Sethi <V.Sethi@nxp.com>,
	linux.cj@gmail.com, Ioana Ciornei <ioana.ciornei@nxp.com>,
	Laurentiu Tudor <laurentiu.tudor@nxp.com>,
	Makarand Pawagi <makarand.pawagi@nxp.com>,
	"David S. Miller" <davem@davemloft.net>,
	linux-arm-kernel@lists.infradead.org,
	Florin Laurentiu Chiculita <florinlaurentiu.chiculita@nxp.com>
Subject: Re: [RFC net-next PATCH v2 1/2] net/fsl: add ACPI support for mdio bus
Date: Sat, 18 Apr 2020 12:41:16 +0100	[thread overview]
Message-ID: <20200418114116.GU25745@shell.armlinux.org.uk> (raw)
In-Reply-To: <20200418105432.11233-2-calvin.johnson@oss.nxp.com>

On Sat, Apr 18, 2020 at 04:24:31PM +0530, Calvin Johnson wrote:
> @@ -241,18 +244,81 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
>  	return value;
>  }
>  
> +/* Extract the clause 22 phy ID from the compatible string of the form
> + * ethernet-phy-idAAAA.BBBB

This comment is incorrect.  What about clause 45 PHYs?

> + */
> +static int xgmac_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
> +{
> +	const char *cp;
> +	unsigned int upper, lower;
> +	int ret;
> +
> +	ret = fwnode_property_read_string(fwnode, "compatible", &cp);
> +	if (!ret) {
> +		if (sscanf(cp, "ethernet-phy-id%4x.%4x",
> +			   &upper, &lower) == 2) {
> +			*phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
> +			return 0;
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +static int xgmac_mdiobus_register_phy(struct mii_bus *bus,
> +				      struct fwnode_handle *child, u32 addr)
> +{
> +	struct phy_device *phy;
> +	bool is_c45 = false;
> +	int rc;
> +	const char *cp;
> +	u32 phy_id;
> +
> +	fwnode_property_read_string(child, "compatible", &cp);
> +	if (!strcmp(cp, "ethernet-phy-ieee802.3-c45"))
> +		is_c45 = true;
> +
> +	if (!is_c45 && !xgmac_get_phy_id(child, &phy_id))
> +		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> +	else
> +		phy = get_phy_device(bus, addr, is_c45);
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);
> +
> +	phy->irq = bus->irq[addr];
> +
> +	/* Associate the fwnode with the device structure so it
> +	 * can be looked up later.
> +	 */
> +	phy->mdio.dev.fwnode = child;
> +
> +	/* All data is now stored in the phy struct, so register it */
> +	rc = phy_device_register(phy);
> +	if (rc) {
> +		phy_device_free(phy);
> +		fwnode_handle_put(child);
> +		return rc;
> +	}
> +
> +	dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
> +
> +	return 0;

You seem to be duplicating the OF implementation in a private driver,
converting it to fwnode.  This is not how we develop the Linux kernel.
We fix subsystem problems by fixing the subsystems, not by throwing
what should be subsystem code into private drivers.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel