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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DEUNYYW0Y23E.2SA0SOCS99NA0@bootlin.com>
Date: Wed, 10 Dec 2025 17:06:32 +0100
From: "Luca Ceresoli" <luca.ceresoli@...tlin.com>
To: Théo Lebrun <theo.lebrun@...tlin.com>, "Vladimir
 Kondratiev" <vladimir.kondratiev@...ileye.com>,
 Grégory Clement <gregory.clement@...tlin.com>, "Rob
 Herring" <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>,
 "Conor Dooley" <conor+dt@...nel.org>, "Vinod Koul" <vkoul@...nel.org>,
 "Kishon Vijay Abraham I" <kishon@...nel.org>, "Michael Turquette"
 <mturquette@...libre.com>, "Stephen Boyd" <sboyd@...nel.org>, "Philipp
 Zabel" <p.zabel@...gutronix.de>, "Thomas Bogendoerfer"
 <tsbogend@...ha.franken.de>
Cc: <linux-mips@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <linux-phy@...ts.infradead.org>,
 <linux-clk@...r.kernel.org>, Benoît Monin
 <benoit.monin@...tlin.com>, "Maxime Chevallier"
 <maxime.chevallier@...tlin.com>, "Tawfik Bayouk"
 <tawfik.bayouk@...ileye.com>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v4 2/7] phy: Add driver for EyeQ5 Ethernet PHY wrapper

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