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

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello Luca,

On Wed Dec 10, 2025 at 5:06 PM CET, Luca Ceresoli wrote:
> 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.

Yes good catch, after having checked all symbols used, updates are:
- Add delay.h for udelay(), gfp_types.h for GFP_* alloc flags,
  module.h for MODULE_* macros.
- Drop array_size.h, bug.h, cleanup.h, container_of.h, lockdep.h,
  mutex.h.

We do need bitfield.h for FIELD_PREP().

>
>> +#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)

Done, indeed the old magic value was not a good idea.

>
>> +		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);

Because I had forgotten. :-) Thanks!

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

Good idea once again.

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

Thanks for the review. Does that imply I can append your Rb trailer?

Thanks Luca,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com