Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <2b646678611c3ed7546d880f8875d57a6800c281.camel@maquefel.me>
Subject: Re: [PATCH v5 17/39] net: cirrus: add DT support for Cirrus EP93xx
From:   Nikita Shubin <nikita.shubin@maquefel.me>
To:     Andy Shevchenko <andriy.shevchenko@intel.com>
Cc:     Hartley Sweeten <hsweeten@visionengravers.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Alexander Sverdlin <alexander.sverdlin@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>
Date:   Fri, 08 Dec 2023 14:10:29 +0300
In-Reply-To: <ZV3xFjmU56hwBfLc@smile.fi.intel.com>
References: <20231122-ep93xx-v5-0-d59a76d5df29@maquefel.me>
         <20231122-ep93xx-v5-17-d59a76d5df29@maquefel.me>
         <ZV3xFjmU56hwBfLc@smile.fi.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 
MIME-Version: 1.0
Precedence: bulk

Hello Andy!

Thanks for review (i am counting all review not only this patch), only
this comment below raised a question:

On Wed, 2023-11-22 at 14:16 +0200, Andy Shevchenko wrote:
> On Wed, Nov 22, 2023 at 11:59:55AM +0300, Nikita Shubin wrote:
> > - add OF ID match table
> > - get phy_id from the device tree, as part of mdio
> > - copy_addr is now always used, as there is no SoC/board that
> > aren't
> > - dropped platform header
>=20
> ...
>=20
> > =C2=A0#include <linux/interrupt.h>
> > =C2=A0#include <linux/moduleparam.h>
> > =C2=A0#include <linux/platform_device.h>
> > +#include <linux/of.h>
>=20
> Perhaps more ordering?
>=20
> > =C2=A0#include <linux/delay.h>
> > =C2=A0#include <linux/io.h>
> > =C2=A0#include <linux/slab.h>
>=20
> ...
>=20
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0err =3D of_property_read_u32=
(np, "reg", &phy_id);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0of_node_put(np);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (err)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0return dev_err_probe(&pdev->dev, -ENOENT, "Failed
> > to locate \"phy_id\"\n");
>=20
> Why shadowing the actual error code?

of_property_read_u32 returns NULL on error.

>=20
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0dev =3D alloc_etherdev(sizeo=
f(struct ep93xx_priv));
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (dev =3D=3D NULL) {
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0err =3D -ENOMEM;
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0goto err_out;
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
>=20