* [PATCH 0/3] staging: mt7621-pci-phy: simplify driver to don't use child nodes in DT @ 2019-03-28 17:55 Sergio Paracuellos 2019-03-28 17:55 ` [PATCH 1/3] staging: mt7621-dts: simplify pcie phy bindings Sergio Paracuellos ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Sergio Paracuellos @ 2019-03-28 17:55 UTC (permalink / raw) To: neil; +Cc: gregkh, driverdev-devel This series change both bindings and driver to don't use child nodes in the device tree. This changes are inspired after Rob's bindings review which can be found here: http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2019-March/133123.html Hope this helps. Best regards, Sergio Paracuellos Sergio Paracuellos (3): staging: mt7621-dts: simplify pcie phy bindings staging: mt7621-pci-phy: update bindings documentation staging: mt7621-pci-phy: change driver to don't use child nodes drivers/staging/mt7621-dts/mt7621.dtsi | 23 ++-------- .../mediatek,mt7621-pci-phy.txt | 44 ++++--------------- .../staging/mt7621-pci-phy/pci-mt7621-phy.c | 26 +++++++++-- 3 files changed, 34 insertions(+), 59 deletions(-) -- 2.19.1 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] staging: mt7621-dts: simplify pcie phy bindings 2019-03-28 17:55 [PATCH 0/3] staging: mt7621-pci-phy: simplify driver to don't use child nodes in DT Sergio Paracuellos @ 2019-03-28 17:55 ` Sergio Paracuellos 2019-03-29 3:01 ` NeilBrown 2019-03-28 17:55 ` [PATCH 2/3] staging: mt7621-pci-phy: update bindings documentation Sergio Paracuellos 2019-03-28 17:55 ` [PATCH 3/3] staging: mt7621-pci-phy: change driver to don't use child nodes Sergio Paracuellos 2 siblings, 1 reply; 10+ messages in thread From: Sergio Paracuellos @ 2019-03-28 17:55 UTC (permalink / raw) To: neil; +Cc: gregkh, driverdev-devel If each phy port doesn't have its own resources, then we don't need child nodes. Handle it using #phy-cells to 1 or 0 conveniently. Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> --- drivers/staging/mt7621-dts/mt7621.dtsi | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi index 17020e24abd2..7a85d8b086bb 100644 --- a/drivers/staging/mt7621-dts/mt7621.dtsi +++ b/drivers/staging/mt7621-dts/mt7621.dtsi @@ -491,7 +491,7 @@ reset-names = "pcie", "pcie0", "pcie1", "pcie2"; clocks = <&clkctrl 24 &clkctrl 25 &clkctrl 26>; clock-names = "pcie0", "pcie1", "pcie2"; - phys = <&pcie0_port>, <&pcie1_port>, <&pcie2_port>; + phys = <&pcie0_phy 0>, <&pcie0_phy 1>, <&pcie1_phy>; phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2"; pcie@0,0 { @@ -522,29 +522,12 @@ pcie0_phy: pcie-phy@1e149000 { compatible = "mediatek,mt7621-pci-phy"; reg = <0x1e149000 0x0700>; - #address-cells = <1>; - #size-cells = <0>; - - pcie0_port: pcie-phy@0 { - reg = <0>; - #phy-cells = <0>; - }; - - pcie1_port: pcie-phy@1 { - reg = <1>; - #phy-cells = <0>; - }; + #phy-cells = <1>; }; pcie1_phy: pcie-phy@1e14a000 { compatible = "mediatek,mt7621-pci-phy"; reg = <0x1e14a000 0x0700>; - #address-cells = <1>; - #size-cells = <0>; - - pcie2_port: pcie-phy@0 { - reg = <0>; - #phy-cells = <0>; - }; + #phy-cells = <0>; }; }; -- 2.19.1 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] staging: mt7621-dts: simplify pcie phy bindings 2019-03-28 17:55 ` [PATCH 1/3] staging: mt7621-dts: simplify pcie phy bindings Sergio Paracuellos @ 2019-03-29 3:01 ` NeilBrown 2019-03-29 5:54 ` Sergio Paracuellos 0 siblings, 1 reply; 10+ messages in thread From: NeilBrown @ 2019-03-29 3:01 UTC (permalink / raw) To: Sergio Paracuellos; +Cc: gregkh, driverdev-devel [-- Attachment #1.1: Type: text/plain, Size: 1889 bytes --] On Thu, Mar 28 2019, Sergio Paracuellos wrote: > If each phy port doesn't have its own resources, then we don't need > child nodes. Handle it using #phy-cells to 1 or 0 conveniently. > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> > --- > drivers/staging/mt7621-dts/mt7621.dtsi | 23 +++-------------------- > 1 file changed, 3 insertions(+), 20 deletions(-) > > diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi > index 17020e24abd2..7a85d8b086bb 100644 > --- a/drivers/staging/mt7621-dts/mt7621.dtsi > +++ b/drivers/staging/mt7621-dts/mt7621.dtsi > @@ -491,7 +491,7 @@ > reset-names = "pcie", "pcie0", "pcie1", "pcie2"; > clocks = <&clkctrl 24 &clkctrl 25 &clkctrl 26>; > clock-names = "pcie0", "pcie1", "pcie2"; > - phys = <&pcie0_port>, <&pcie1_port>, <&pcie2_port>; > + phys = <&pcie0_phy 0>, <&pcie0_phy 1>, <&pcie1_phy>; I would be more comfortable if this was phys = <&pcie0_phy 0>, <&pcie0_phy 1>, <&pcie1_phy 0>; and pcie-phy@1 had #phy-cells = <1>; Thanks, NeilBrown > phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2"; > > pcie@0,0 { > @@ -522,29 +522,12 @@ > pcie0_phy: pcie-phy@1e149000 { > compatible = "mediatek,mt7621-pci-phy"; > reg = <0x1e149000 0x0700>; > - #address-cells = <1>; > - #size-cells = <0>; > - > - pcie0_port: pcie-phy@0 { > - reg = <0>; > - #phy-cells = <0>; > - }; > - > - pcie1_port: pcie-phy@1 { > - reg = <1>; > - #phy-cells = <0>; > - }; > + #phy-cells = <1>; > }; > > pcie1_phy: pcie-phy@1e14a000 { > compatible = "mediatek,mt7621-pci-phy"; > reg = <0x1e14a000 0x0700>; > - #address-cells = <1>; > - #size-cells = <0>; > - > - pcie2_port: pcie-phy@0 { > - reg = <0>; > - #phy-cells = <0>; > - }; > + #phy-cells = <0>; > }; > }; > -- > 2.19.1 [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] [-- Attachment #2: Type: text/plain, Size: 169 bytes --] _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] staging: mt7621-dts: simplify pcie phy bindings 2019-03-29 3:01 ` NeilBrown @ 2019-03-29 5:54 ` Sergio Paracuellos 0 siblings, 0 replies; 10+ messages in thread From: Sergio Paracuellos @ 2019-03-29 5:54 UTC (permalink / raw) To: NeilBrown; +Cc: Greg KH, driverdev-devel Hi Neil, Thanks for the feedback. On Fri, Mar 29, 2019 at 4:01 AM NeilBrown <neil@brown.name> wrote: > > On Thu, Mar 28 2019, Sergio Paracuellos wrote: > > > If each phy port doesn't have its own resources, then we don't need > > child nodes. Handle it using #phy-cells to 1 or 0 conveniently. > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> > > --- > > drivers/staging/mt7621-dts/mt7621.dtsi | 23 +++-------------------- > > 1 file changed, 3 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi > > index 17020e24abd2..7a85d8b086bb 100644 > > --- a/drivers/staging/mt7621-dts/mt7621.dtsi > > +++ b/drivers/staging/mt7621-dts/mt7621.dtsi > > @@ -491,7 +491,7 @@ > > reset-names = "pcie", "pcie0", "pcie1", "pcie2"; > > clocks = <&clkctrl 24 &clkctrl 25 &clkctrl 26>; > > clock-names = "pcie0", "pcie1", "pcie2"; > > - phys = <&pcie0_port>, <&pcie1_port>, <&pcie2_port>; > > + phys = <&pcie0_phy 0>, <&pcie0_phy 1>, <&pcie1_phy>; > > I would be more comfortable if this was > > phys = <&pcie0_phy 0>, <&pcie0_phy 1>, <&pcie1_phy 0>; > > and pcie-phy@1 had #phy-cells = <1>; Yes, maybe is more clear when looking at the phy client bindings. Changed in v2. > > Thanks, > NeilBrown Best regards, Sergio Paracuellos > > > phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2"; > > > > pcie@0,0 { > > @@ -522,29 +522,12 @@ > > pcie0_phy: pcie-phy@1e149000 { > > compatible = "mediatek,mt7621-pci-phy"; > > reg = <0x1e149000 0x0700>; > > - #address-cells = <1>; > > - #size-cells = <0>; > > - > > - pcie0_port: pcie-phy@0 { > > - reg = <0>; > > - #phy-cells = <0>; > > - }; > > - > > - pcie1_port: pcie-phy@1 { > > - reg = <1>; > > - #phy-cells = <0>; > > - }; > > + #phy-cells = <1>; > > }; > > > > pcie1_phy: pcie-phy@1e14a000 { > > compatible = "mediatek,mt7621-pci-phy"; > > reg = <0x1e14a000 0x0700>; > > - #address-cells = <1>; > > - #size-cells = <0>; > > - > > - pcie2_port: pcie-phy@0 { > > - reg = <0>; > > - #phy-cells = <0>; > > - }; > > + #phy-cells = <0>; > > }; > > }; > > -- > > 2.19.1 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] staging: mt7621-pci-phy: update bindings documentation 2019-03-28 17:55 [PATCH 0/3] staging: mt7621-pci-phy: simplify driver to don't use child nodes in DT Sergio Paracuellos 2019-03-28 17:55 ` [PATCH 1/3] staging: mt7621-dts: simplify pcie phy bindings Sergio Paracuellos @ 2019-03-28 17:55 ` Sergio Paracuellos 2019-03-28 17:55 ` [PATCH 3/3] staging: mt7621-pci-phy: change driver to don't use child nodes Sergio Paracuellos 2 siblings, 0 replies; 10+ messages in thread From: Sergio Paracuellos @ 2019-03-28 17:55 UTC (permalink / raw) To: neil; +Cc: gregkh, driverdev-devel Device tree has been simplified to use phy-cells instead of using child nodes. Update documentation accordly. Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> --- .../mediatek,mt7621-pci-phy.txt | 44 ++++--------------- 1 file changed, 9 insertions(+), 35 deletions(-) diff --git a/drivers/staging/mt7621-pci-phy/mediatek,mt7621-pci-phy.txt b/drivers/staging/mt7621-pci-phy/mediatek,mt7621-pci-phy.txt index 33a8a698bdd0..0d217d596e8d 100644 --- a/drivers/staging/mt7621-pci-phy/mediatek,mt7621-pci-phy.txt +++ b/drivers/staging/mt7621-pci-phy/mediatek,mt7621-pci-phy.txt @@ -3,45 +3,19 @@ Mediatek Mt7621 PCIe PHY Required properties: - compatible: must be "mediatek,mt7621-pci-phy" - reg: base address and length of the PCIe PHY block -- #address-cells: must be 1 -- #size-cells: must be 0 - -Each PCIe PHY should be represented by a child node - -Required properties For the child node: -- reg: the PHY ID -0 - PCIe RC 0 -1 - PCIe RC 1 -- #phy-cells: must be 0 +- #phy-cells: must be <1> for pcie0_phy and must be <0> for pcie1_phy. Example: - pcie0_phy: pcie-phy@1a149000 { + pcie0_phy: pcie-phy@1e149000 { compatible = "mediatek,mt7621-pci-phy"; - reg = <0x1a149000 0x0700>; - #address-cells = <1>; - #size-cells = <0>; - - pcie0_port: pcie-phy@0 { - reg = <0>; - #phy-cells = <0>; - }; - - pcie1_port: pcie-phy@1 { - reg = <1>; - #phy-cells = <0>; - }; + reg = <0x1e149000 0x0700>; + #phy-cells = <1>; }; - pcie1_phy: pcie-phy@1a14a000 { + pcie1_phy: pcie-phy@1e14a000 { compatible = "mediatek,mt7621-pci-phy"; - reg = <0x1a14a000 0x0700>; - #address-cells = <1>; - #size-cells = <0>; - - pcie2_port: pcie-phy@0 { - reg = <0>; - #phy-cells = <0>; - }; + reg = <0x1e14a000 0x0700>; + #phy-cells = <0>; }; /* users of the PCIe phy */ @@ -49,6 +23,6 @@ Example: pcie: pcie@1e140000 { ... ... - phys = <&pcie0_port>, <&pcie1_port>, <&pcie2_port>; + phys = <&pcie0_phy 0>, <&pcie0_phy 1>, <&pcie1_phy>; phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2"; - }; \ No newline at end of file + }; -- 2.19.1 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] staging: mt7621-pci-phy: change driver to don't use child nodes 2019-03-28 17:55 [PATCH 0/3] staging: mt7621-pci-phy: simplify driver to don't use child nodes in DT Sergio Paracuellos 2019-03-28 17:55 ` [PATCH 1/3] staging: mt7621-dts: simplify pcie phy bindings Sergio Paracuellos 2019-03-28 17:55 ` [PATCH 2/3] staging: mt7621-pci-phy: update bindings documentation Sergio Paracuellos @ 2019-03-28 17:55 ` Sergio Paracuellos 2019-03-29 3:06 ` NeilBrown 2019-03-29 7:54 ` Dan Carpenter 2 siblings, 2 replies; 10+ messages in thread From: Sergio Paracuellos @ 2019-03-28 17:55 UTC (permalink / raw) To: neil; +Cc: gregkh, driverdev-devel 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@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) { + 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++; } - 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@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] staging: mt7621-pci-phy: change driver to don't use child nodes 2019-03-28 17:55 ` [PATCH 3/3] staging: mt7621-pci-phy: change driver to don't use child nodes Sergio Paracuellos @ 2019-03-29 3:06 ` NeilBrown 2019-03-29 5:57 ` Sergio Paracuellos 2019-03-29 7:54 ` Dan Carpenter 1 sibling, 1 reply; 10+ messages in thread From: NeilBrown @ 2019-03-29 3:06 UTC (permalink / raw) To: Sergio Paracuellos; +Cc: gregkh, driverdev-devel [-- Attachment #1.1: Type: text/plain, Size: 3623 bytes --] 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@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; > + 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 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. > 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 > } > > - 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 [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] [-- Attachment #2: Type: text/plain, Size: 169 bytes --] _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] staging: mt7621-pci-phy: change driver to don't use child nodes 2019-03-29 3:06 ` NeilBrown @ 2019-03-29 5:57 ` Sergio Paracuellos 0 siblings, 0 replies; 10+ messages in thread From: Sergio Paracuellos @ 2019-03-29 5:57 UTC (permalink / raw) To: NeilBrown; +Cc: Greg KH, driverdev-devel On Fri, Mar 29, 2019 at 4:06 AM NeilBrown <neil@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@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 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] staging: mt7621-pci-phy: change driver to don't use child nodes 2019-03-28 17:55 ` [PATCH 3/3] staging: mt7621-pci-phy: change driver to don't use child nodes Sergio Paracuellos 2019-03-29 3:06 ` NeilBrown @ 2019-03-29 7:54 ` Dan Carpenter 2019-03-29 8:02 ` Sergio Paracuellos 1 sibling, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2019-03-29 7:54 UTC (permalink / raw) To: Sergio Paracuellos; +Cc: neil, gregkh, driverdev-devel 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". 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). 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? regards, dan carpenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] staging: mt7621-pci-phy: change driver to don't use child nodes 2019-03-29 7:54 ` Dan Carpenter @ 2019-03-29 8:02 ` Sergio Paracuellos 0 siblings, 0 replies; 10+ messages in thread From: Sergio Paracuellos @ 2019-03-29 8:02 UTC (permalink / raw) To: Dan Carpenter; +Cc: NeilBrown, Greg KH, driverdev-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-03-29 8:02 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-28 17:55 [PATCH 0/3] staging: mt7621-pci-phy: simplify driver to don't use child nodes in DT Sergio Paracuellos 2019-03-28 17:55 ` [PATCH 1/3] staging: mt7621-dts: simplify pcie phy bindings Sergio Paracuellos 2019-03-29 3:01 ` NeilBrown 2019-03-29 5:54 ` Sergio Paracuellos 2019-03-28 17:55 ` [PATCH 2/3] staging: mt7621-pci-phy: update bindings documentation Sergio Paracuellos 2019-03-28 17:55 ` [PATCH 3/3] staging: mt7621-pci-phy: change driver to don't use child nodes Sergio Paracuellos 2019-03-29 3:06 ` NeilBrown 2019-03-29 5:57 ` Sergio Paracuellos 2019-03-29 7:54 ` Dan Carpenter 2019-03-29 8:02 ` Sergio Paracuellos
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.