* [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
* [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 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 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 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
* 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.