On Fri, Mar 29, 2019 at 4:06 AM NeilBrown <neil@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > .../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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
