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

[Luca Ceresoli]

On Mon Dec 15, 2025 at 4:08 PM CET, Théo Lebrun wrote:
> 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 at 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?

If you apply all the changes I have mention, yes, but in doubt you can
avoid it and I'll review your next version. Re-reviewing is much faster
than reviewing the first time (last famous words).

Luca

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