| 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
| ||
|
Message-Id: <DEYVVCWBOZSH.2ZY41YCHLS8FU@bootlin.com>
Date: Mon, 15 Dec 2025 16:08:38 +0100
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: "Luca Ceresoli" <luca.ceresoli@...tlin.com>,
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
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@...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.
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
Powered by blists - more mailing lists