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

Sergio Paracuellos sergio.paracuellos at gmail.com
Fri Mar 29 05:57:44 UTC 2019


On Fri, Mar 29, 2019 at 4:06 AM NeilBrown <neil at brown.name> wrote:
>
> On Thu, Mar 28 2019, Sergio Paracuellos wrote:
>
> > Device tree has been simplified to don't use child nodes and use
> > the #phy-cells property instead. Change the driver accordly implementing
> > custom 'xlate' function to return the correct phy for each port.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos at gmail.com>
> > ---
> >  .../staging/mt7621-pci-phy/pci-mt7621-phy.c   | 26 ++++++++++++++++---
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
> > index 98c06308143c..557e6a53fc1e 100644
> > --- a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
> > +++ b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
> > @@ -78,6 +78,8 @@
> >
> >  #define RG_PE1_FRC_MSTCKDIV                  BIT(5)
> >
> > +#define MAX_PHYS     2
> > +
> >  /**
> >   * struct mt7621_pci_phy_instance - Mt7621 Pcie PHY device
> >   * @phy: pointer to the kernel PHY device
> > @@ -289,6 +291,20 @@ static const struct phy_ops mt7621_pci_phy_ops = {
> >       .owner          = THIS_MODULE,
> >  };
> >
> > +static struct phy *mt7621_pcie_phy_of_xlate(struct device *dev,
> > +                                         struct of_phandle_args *args)
> > +{
> > +     struct mt7621_pci_phy *mt7621_phy = dev_get_drvdata(dev);
> > +
> > +     if (args->args_count == 0)
> > +             return mt7621_phy->phys[0]->phy;
> > +
> > +     if (WARN_ON(args->args[0] >= MAX_PHYS))
> > +             return ERR_PTR(-ENODEV);
> > +
> > +     return mt7621_phy->phys[args->args[0]]->phy;
> > +}
> > +
> >  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) {
>
> This isn't the old place that you depend on the children nodes.
> A little earlier, you have
>
>        phy->nphys = of_get_child_count(np);
>
> which now sets nphys to zero.  I changed this to
>
>        phy->nphys = MAX_PHYS;

True, thanks for catching this. Changed in v2.

>
> > +     of_property_read_u32(dev->of_node, "#phy-cells", &phy_num);
> > +
> > +     for (port = 0; port < phy_num + 1; port++) {
>
> I don't think it is correct to use #phy-cells as the number of phys.
> #phy-cells is the number of arguments - the args_count seen in
> mt7621_pci_phy_probe.

I know and agree maybe is a little weird. I shew the same approach somewhere
in driver code but maybe is not the right thing to do.

> I think you should either assume phy_num is MAX_PHYS, or have built-in
> knowledge of when it is 1 and when it is too.
> There is minimal cost to allocating an extra phy, so I would go with
> MAX_PHYS.

Ok, I've assume MAX_PHYS for both phy's in v2.

>
>
> >               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++;
>
> This "port++" duplicates the "port++" that you introduced in the "for"
> loop above.
>
> Thanks,
> NeilBrown

Thanks for the review.

Best regards,
     Sergio Paracuellos
>
> >       }
> >
> > -     provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> > +     provider = devm_of_phy_provider_register(dev, mt7621_pcie_phy_of_xlate);
> >
> >       return PTR_ERR_OR_ZERO(provider);
> >
> > --
> > 2.19.1


More information about the devel mailing list