Re: [PATCH 3/3] staging: mt7621-pci-phy: change driver to don't use child nodes

From: Sergio Paracuellos <sergio.paracuellos@gmail.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: NeilBrown <neil@brown.name>, Greg KH <gregkh@linuxfoundation.org>,
	driverdev-devel@linuxdriverproject.org
Subject: Re: [PATCH 3/3] staging: mt7621-pci-phy: change driver to don't use child nodes
Date: Fri, 29 Mar 2019 09:02:31 +0100	[thread overview]
Message-ID: <CAMhs-H-+Gkf2ENFe5dpQeN1kB2-pRns2mx4hc=MB0DEPhWw_Ew@mail.gmail.com> (raw)
In-Reply-To: <20190329075414.GX32590@kadam>

Hi Dan,

On Fri, Mar 29, 2019 at 8:54 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Thu, Mar 28, 2019 at 06:55:31PM +0100, Sergio Paracuellos wrote:
> >  static int mt7621_pci_phy_probe(struct platform_device *pdev)
> >  {
> >       struct device *dev = &pdev->dev;
> > @@ -299,6 +315,7 @@ static int mt7621_pci_phy_probe(struct platform_device *pdev)
> >       struct resource res;
> >       int port, ret;
> >       void __iomem *port_base;
> > +     u32 phy_num;
> >
> >       phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
> >       if (!phy)
> > @@ -325,8 +342,9 @@ static int mt7621_pci_phy_probe(struct platform_device *pdev)
> >               return PTR_ERR(port_base);
> >       }
> >
> > -     port = 0;
> > -     for_each_child_of_node(np, child_np) {
> > +     of_property_read_u32(dev->of_node, "#phy-cells", &phy_num);
> > +
> > +     for (port = 0; port < phy_num + 1; port++) {
>                        ^^^^^^^^^^^^^^^^^^  ^^^^^^
> >               struct mt7621_pci_phy_instance *instance;
> >               struct phy *pphy;
> >
> > @@ -338,7 +356,7 @@ static int mt7621_pci_phy_probe(struct platform_device *pdev)
> >
> >               phy->phys[port] = instance;
> >
> > -             pphy = devm_phy_create(dev, child_np, &mt7621_pci_phy_ops);
> > +             pphy = devm_phy_create(dev, dev->of_node, &mt7621_pci_phy_ops);
> >               if (IS_ERR(phy)) {
> >                       dev_err(dev, "failed to create phy\n");
> >                       ret = PTR_ERR(phy);
> > @@ -352,7 +370,7 @@ static int mt7621_pci_phy_probe(struct platform_device *pdev)
> >               port++;
>                 ^^^^^^
> >       }
>
> Incrementing "port" twice is probably wrong...  Or anyway, less readable
> than "port += 2".

Yes, that was a mistake in my code becase I did not delete it when the
loop was changed.

>
> To be honest, I don't know anything about device tree.  Does "phy_num"
> come from the device tree stuff that you just changed in the ealier
> patches?  (I never read those so I never learn anything about device
> tree so I am stuck in an endless doom cycle).

The first approach in v1 was to read this from #phy-cell property from
device tree. Neil points me
out this was not the correct approach and was changed to a fixed
MAX_PHYS for both phy's in v2 patches.

>
> Anyway, I am a real newbie.  Where does the plus one in
> "port < phy_num + 1" come from?  How do I verify that phy_num is less
> than phy->nphys?

In the same way, this is now a fixed port < MAX_PHYS in for loop sent
in v2 of the patches.

>
> regards,
> dan carpenter
>

Best regards,
    Sergio Paracuellos