Re: [PATCH v4 2/7] phy: Add driver for EyeQ5 Ethernet PHY wrapper

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

On Mon Nov 24, 2025 at 3:41 PM CET, Théo Lebrun wrote:
> EyeQ5 embeds a system-controller called OLB. It features many unrelated
> registers, and some of those are registers used to configure the
> integration of the RGMII/SGMII Cadence PHY used by MACB/GEM instances.
>
> Wrap in a neat generic PHY provider, exposing two PHYs with standard
> phy_init() / phy_set_mode() / phy_power_on() operations.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

[...]

> --- /dev/null
> +++ b/drivers/phy/phy-eyeq5-eth.c
> @@ -0,0 +1,254 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/array_size.h>
> +#include <linux/auxiliary_bus.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/bug.h>
> +#include <linux/cleanup.h>
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/lockdep.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/phy.h>
> +#include <linux/phy/phy.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>

Are all these include files really needed? At a quick glance bitfield.h,
cleanup.h and lockdep.h look unused in this file.

> +#define EQ5_PHY_COUNT	2

[...]

> +static const struct phy_ops eq5_phy_ops = {
> +	.init		= eq5_phy_init,
> +	.exit		= eq5_phy_exit,
> +	.set_mode	= eq5_phy_set_mode,
> +	.power_on	= eq5_phy_power_on,
> +	.power_off	= eq5_phy_power_off,
> +};
> +
> +static struct phy *eq5_phy_xlate(struct device *dev,
> +				 const struct of_phandle_args *args)
> +{
> +	struct eq5_phy_private *priv = dev_get_drvdata(dev);
> +
> +	if (args->args_count != 1 || args->args[0] > 1)

Maybe, for better clarity:

	if (args->args_count != 1 || args->args[0] >= EQ5_PHY_COUNT)

> +		return ERR_PTR(-EINVAL);
> +
> +	return priv->phys[args->args[0]].phy;
> +}
> +
> +static int eq5_phy_probe_phy(struct eq5_phy_private *priv, unsigned int index,
> +			     void __iomem *base, unsigned int gp,
> +			     unsigned int sgmii)
> +{
> +	struct eq5_phy_inst *inst = &priv->phys[index];
> +	struct device *dev = priv->dev;
> +	struct phy *phy;
> +
> +	phy = devm_phy_create(dev, dev->of_node, &eq5_phy_ops);
> +	if (IS_ERR(phy)) {
> +		dev_err(dev, "failed to create PHY %u\n", index);
> +		return PTR_ERR(phy);
> +	}

Why not dev_err_probe()? It would make code more concise too:

	phy = devm_phy_create(dev, dev->of_node, &eq5_phy_ops);
	if (IS_ERR(phy))
		return dev_err_probe(dev, PTR_ERR(phy), "failed to create PHY %u\n", index);

> +
> +	inst->priv = priv;
> +	inst->phy = phy;
> +	inst->gp = base + gp;
> +	inst->sgmii = base + sgmii;
> +	inst->phy_interface = PHY_INTERFACE_MODE_NA;
> +	phy_set_drvdata(phy, inst);
> +
> +	return 0;
> +}
> +
> +static int eq5_phy_probe(struct auxiliary_device *adev,
> +			 const struct auxiliary_device_id *id)
> +{
> +	struct device *dev = &adev->dev;
> +	struct phy_provider *provider;
> +	struct eq5_phy_private *priv;
> +	void __iomem *base;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	dev_set_drvdata(dev, priv);
> +
> +	base = (void __iomem *)dev_get_platdata(dev);
> +
> +	ret = eq5_phy_probe_phy(priv, 0, base, EQ5_PHY0_GP, EQ5_PHY0_SGMII);
> +	if (ret)
> +		return ret;
> +
> +	ret = eq5_phy_probe_phy(priv, 1, base, EQ5_PHY1_GP, EQ5_PHY1_SGMII);
> +	if (ret)
> +		return ret;
> +
> +	provider = devm_of_phy_provider_register(dev, eq5_phy_xlate);
> +	if (IS_ERR(provider)) {
> +		dev_err(dev, "registering provider failed\n");
> +		return PTR_ERR(provider);
> +	}

As above, why not dev_err_probe()?

Other than the above minor issues, LGTM. This driver looks cleanly
implemented.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy