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

[James Hilliard]

On Sun, Aug 9, 2026 at 9:51 AM Andrew Lunn <andrew@xxxxxxx> wrote:
>
> 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.

Removed in v5.

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

Moved to xpowers subdirectory in v5.

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

In v2, the AC200 backend was a separate control device. The PHY driver
obtained its callback table from the supplier and invoked power,
interface, and LED operations from probe, reset, PM, and LED paths. The
control driver could independently invoke power_off() from its remove
and shutdown callbacks.

The mutex therefore serialized calls crossing those two driver
lifecycles. It prevented, for example, supplier shutdown from running
power_off() halfway through a PHY power_on() sequence, and protected
cached state such as powered, phy_addr, interface, and LED
configuration. The device link protected lifetime and removal ordering,
but did not serialize every callback.

When v3 removed the standalone control devices and made each backend
private to its PHY instance, that second entry domain disappeared. The
mutex then became an unnecessary carryover through v3/v4 and has been
removed in v5.

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

That does simplify the consumer. I have converted the AC200 path to use
syscon in v5. The AC200 I2C driver creates its paged regmap as before and
publishes it for the AC200 OF node. The PHY resolves x-powers,ac200
with syscon_regmap_lookup_by_phandle(), so the device_trylock(),
device_is_bound() and dev_get_regmap() sequence is gone. The lookup
returns -EPROBE_DEFER until the provider has published its regmap.

Since this is an I2C-backed regmap rather than a generic MMIO syscon, and
the AC200 driver can be unbound, I added managed registration for
externally created syscon regmaps. It removes the syscon entry before
devres releases the regmap, avoiding a stale global entry after provider
removal.

The PHY still resolves the I2C device to establish a device link and
obtain the chip-wide input clock. The device link provides supplier
lifetime and teardown ordering; syscon is now used for the regmap lookup.

>
>     Andrew
>
> ---
> pw-bot: cr