Re: [PATCH net-next v4 4/4] net: phy: add X-Powers AC200/AC300 EPHY driver

[Andrew Lunn]

On Thu, Aug 06, 2026 at 10:51:29PM -0600, James Hilliard wrote:
> The AC200 and AC300 contain compatible Fast Ethernet link PHYs, but the
> link endpoint is inaccessible until package-specific control registers
> have powered and configured it.
> 
> Add one PHY driver which binds the link child and joins its parent
> Ethernet PHY package. Fixed package compatibles select the corresponding
> backend. The generic ACx00 compatible reads one packed configuration field
> and selects the backend before touching backend-specific resources. The
> AC300 path therefore does not resolve or enable the candidate AC200 I2C
> device.
> 
> AC200 obtains the regmap of the referenced I2C MFD and keeps it bound
> with a device link. If CONFIG_OF_DYNAMIC is available

Please drop all the CONFIG_OF_DYNAMIC code.

>  drivers/net/phy/Kconfig               |  11 +
>  drivers/net/phy/Makefile              |   3 +
>  drivers/net/phy/xpowers-acx00-ac200.c | 388 ++++++++++++++++++++++++
>  drivers/net/phy/xpowers-acx00-ac300.c | 404 +++++++++++++++++++++++++
>  drivers/net/phy/xpowers-acx00-main.c  | 536 ++++++++++++++++++++++++++++++++++
>  drivers/net/phy/xpowers-acx00.h       |  28 ++

Since there are 4 files, i would suggest a subdirectory.

> +struct ac200_ephy_ctl {
> +	struct acx00_ephy_control control;
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct mutex lock; /* Serializes power sequencing and state. */
> +	u16 ephy_ctl;

What is this locking against?

> +	client = of_find_i2c_device_by_node(ac200_node);
> +	of_node_put(ac200_node);
> +	if (!client) {
> +		ret = IS_ENABLED(CONFIG_I2C) ? -EPROBE_DEFER : -ENODEV;
> +		return ERR_PTR(dev_err_probe(dev, ret,
> +					     "AC200 device is not registered\n"));
> +	}
> +
> +	if (!device_link_add(dev, &client->dev,
> +			     DL_FLAG_AUTOREMOVE_CONSUMER)) {
> +		ret = dev_err_probe(dev, -EINVAL,
> +				    "failed to link AC200 device\n");
> +		goto out_put_client;
> +	}
> +
> +	if (!device_trylock(&client->dev)) {
> +		ret = dev_err_probe(dev, -EPROBE_DEFER,
> +				    "AC200 driver is not ready\n");
> +		goto out_put_client;
> +	}
> +
> +	if (device_is_bound(&client->dev))
> +		priv->regmap = dev_get_regmap(&client->dev, NULL);
> +	device_unlock(&client->dev);
> +	if (!priv->regmap) {
> +		ret = dev_err_probe(dev, -EPROBE_DEFER,
> +				    "AC200 driver is not ready\n");
> +		goto out_error;
> +	}

Please take a look at syscon, and syscon_regmap_lookup_by_phandle().
I think it will make this code a lot simpler.

    Andrew

---
pw-bot: cr