[PATCH 0/5] staging: mt7621-pci: some required changes after first review

[PATCH 0/5] staging: mt7621-pci: some required changes after first review

[Sergio Paracuellos]

Hi,

Some things must be updated before get this a new attempt to get this
mainlined:
    - Device tree bindings need to move som stuff into child root nodes. 
    - Use relaxed version of readl and writel.
    - Other minor stuff.

After this is merged in staging tree, I will move with 'git mv' this
driver into 'drivers/pci/controller' as I've been suggested to do by
Bjorn.

Thanks in advance for your time.

Best regards,
    Sergio Paracuellos

Sergio Paracuellos (5):
  staging: mt7621-pci: make cleaner 'mt7621_pcie_enable_ports'
  staging: mt7621-pci: remove 'RALINK_PCI_BAR0SETUP_ADDR' definition
  staging: mt7621-pci: use {readl|writel}_relaxed instead of
    readl/writel
  staging: mt7621-dts: move some properties into root port child nodes
  staging: mt7621-pci: parse some dt properties from root port child
    nodes

 drivers/staging/mt7621-dts/mt7621.dtsi  | 21 ++++++----
 drivers/staging/mt7621-pci/pci-mt7621.c | 56 +++++++++++--------------
 2 files changed, 36 insertions(+), 41 deletions(-)

-- 
2.25.1

[PATCH 1/5] staging: mt7621-pci: make cleaner 'mt7621_pcie_enable_ports'

[Sergio Paracuellos]

Function 'mt7621_pcie_enable_ports' call 'mt7621_pcie_enable_port'
for each available pcie port. Instead of having two for loops
there just move needed initialization. There is one setting
that can be removed which is the set for 'PCI_COMMAND_MASTER'
bit. Pci drivers are in charge of set that bit if is is really
needed and should be not a mission of the controller to do that.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index fe1945819d25..c14fc48e74fc 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -499,15 +499,18 @@ static void mt7621_pcie_enable_port(struct mt7621_pcie_port *port)
 	/* configure class code and revision ID */
 	pcie_write(pcie, PCIE_CLASS_CODE | PCIE_REVISION_ID,
 		   offset + RALINK_PCI_CLASS);
+
+	/* configure RC FTS number to 250 when it leaves L0s */
+	val = read_config(pcie, slot, PCIE_FTS_NUM);
+	val &= ~PCIE_FTS_NUM_MASK;
+	val |= PCIE_FTS_NUM_L0(0x50);
+	write_config(pcie, slot, PCIE_FTS_NUM, val);
 }
 
 static int mt7621_pcie_enable_ports(struct mt7621_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
 	struct mt7621_pcie_port *port;
-	u8 num_slots_enabled = 0;
-	u32 slot;
-	u32 val;
 	int err;
 
 	/* Setup MEMWIN and IOWIN */
@@ -518,27 +521,16 @@ static int mt7621_pcie_enable_ports(struct mt7621_pcie *pcie)
 		if (port->enabled) {
 			err = clk_prepare_enable(port->clk);
 			if (err) {
-				dev_err(dev, "enabling clk pcie%d\n", slot);
+				dev_err(dev, "enabling clk pcie%d\n",
+					port->slot);
 				return err;
 			}
 
 			mt7621_pcie_enable_port(port);
 			dev_info(dev, "PCIE%d enabled\n", port->slot);
-			num_slots_enabled++;
 		}
 	}
 
-	for (slot = 0; slot < num_slots_enabled; slot++) {
-		val = read_config(pcie, slot, PCI_COMMAND);
-		val |= PCI_COMMAND_MASTER;
-		write_config(pcie, slot, PCI_COMMAND, val);
-		/* configure RC FTS number to 250 when it leaves L0s */
-		val = read_config(pcie, slot, PCIE_FTS_NUM);
-		val &= ~PCIE_FTS_NUM_MASK;
-		val |= PCIE_FTS_NUM_L0(0x50);
-		write_config(pcie, slot, PCIE_FTS_NUM, val);
-	}
-
 	return 0;
 }
 
-- 
2.25.1

[PATCH 2/5] staging: mt7621-pci: remove 'RALINK_PCI_BAR0SETUP_ADDR' definition

[Sergio Paracuellos]

Instead of define RALINK_PCI_BAR0SETUP_ADDR just use standard
pci defnition for this which is 'PCI_BASE_ADDRESS_0'.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index c14fc48e74fc..b83c338a2e3d 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -47,7 +47,6 @@
 #define MT7621_PCIE_OFFSET		0x2000
 #define MT7621_NEXT_PORT		0x1000
 
-#define RALINK_PCI_BAR0SETUP_ADDR	0x0010
 #define RALINK_PCI_ID			0x0030
 #define RALINK_PCI_CLASS		0x0034
 #define RALINK_PCI_SUBID		0x0038
@@ -494,7 +493,7 @@ static void mt7621_pcie_enable_port(struct mt7621_pcie_port *port)
 
 	/* map 2G DDR region */
 	pcie_write(pcie, PCIE_BAR_MAP_MAX | PCIE_BAR_ENABLE,
-		   offset + RALINK_PCI_BAR0SETUP_ADDR);
+		   offset + PCI_BASE_ADDRESS_0);
 
 	/* configure class code and revision ID */
 	pcie_write(pcie, PCIE_CLASS_CODE | PCIE_REVISION_ID,
-- 
2.25.1

[PATCH 3/5] staging: mt7621-pci: use {readl|writel}_relaxed instead of readl/writel

[Sergio Paracuellos]

The driver does not perform DMA, so it's safe to use the relaxed version
for both readl and writel operations.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index b83c338a2e3d..8d14d0f9f769 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -109,32 +109,32 @@ struct mt7621_pcie {
 
 static inline u32 pcie_read(struct mt7621_pcie *pcie, u32 reg)
 {
-	return readl(pcie->base + reg);
+	return readl_relaxed(pcie->base + reg);
 }
 
 static inline void pcie_write(struct mt7621_pcie *pcie, u32 val, u32 reg)
 {
-	writel(val, pcie->base + reg);
+	writel_relaxed(val, pcie->base + reg);
 }
 
 static inline void pcie_rmw(struct mt7621_pcie *pcie, u32 reg, u32 clr, u32 set)
 {
-	u32 val = readl(pcie->base + reg);
+	u32 val = readl_relaxed(pcie->base + reg);
 
 	val &= ~clr;
 	val |= set;
-	writel(val, pcie->base + reg);
+	writel_relaxed(val, pcie->base + reg);
 }
 
 static inline u32 pcie_port_read(struct mt7621_pcie_port *port, u32 reg)
 {
-	return readl(port->base + reg);
+	return readl_relaxed(port->base + reg);
 }
 
 static inline void pcie_port_write(struct mt7621_pcie_port *port,
 				   u32 val, u32 reg)
 {
-	writel(val, port->base + reg);
+	writel_relaxed(val, port->base + reg);
 }
 
 static inline u32 mt7621_pci_get_cfgaddr(unsigned int bus, unsigned int slot,
@@ -151,7 +151,7 @@ static void __iomem *mt7621_pcie_map_bus(struct pci_bus *bus,
 	u32 address = mt7621_pci_get_cfgaddr(bus->number, PCI_SLOT(devfn),
 					     PCI_FUNC(devfn), where);
 
-	writel(address, pcie->base + RALINK_PCI_CONFIG_ADDR);
+	writel_relaxed(address, pcie->base + RALINK_PCI_CONFIG_ADDR);
 
 	return pcie->base + RALINK_PCI_CONFIG_DATA + (where & 3);
 }
-- 
2.25.1

[PATCH 4/5] staging: mt7621-dts: move some properties into root port child nodes

[Sergio Paracuellos]

After a review of the bindings 'clocks', 'resets' and 'phys' must
be moved into root port child nodes. Hence, move all of them.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-dts/mt7621.dtsi | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index 840ba0c3ffed..ecfe2f2cf75a 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -500,15 +500,6 @@ pcie: pcie@1e140000 {
 
 		status = "disabled";
 
-		resets = <&rstctrl 24>, <&rstctrl 25>, <&rstctrl 26>;
-		reset-names = "pcie0", "pcie1", "pcie2";
-		clocks = <&sysc MT7621_CLK_PCIE0>,
-			 <&sysc MT7621_CLK_PCIE1>,
-			 <&sysc MT7621_CLK_PCIE2>;
-		clock-names = "pcie0", "pcie1", "pcie2";
-		phys = <&pcie0_phy 1>, <&pcie2_phy 0>;
-		phy-names = "pcie-phy0", "pcie-phy2";
-
 		reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>;
 
 		pcie@0,0 {
@@ -519,6 +510,10 @@ pcie@0,0 {
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 0>;
 			interrupt-map = <0 0 0 0 &gic GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>;
+			resets = <&rstctrl 24>;
+			clocks = <&sysc MT7621_CLK_PCIE0>;
+			phys = <&pcie0_phy 1>;
+			phy-names = "pcie-phy0";
 			ranges;
 		};
 
@@ -530,6 +525,10 @@ pcie@1,0 {
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 0>;
 			interrupt-map = <0 0 0 0 &gic GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>;
+			resets = <&rstctrl 25>;
+			clocks = <&sysc MT7621_CLK_PCIE1>;
+			phys = <&pcie0_phy 1>;
+			phy-names = "pcie-phy1";
 			ranges;
 		};
 
@@ -541,6 +540,10 @@ pcie@2,0 {
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 0>;
 			interrupt-map = <0 0 0 0 &gic GIC_SHARED 25 IRQ_TYPE_LEVEL_HIGH>;
+			resets = <&rstctrl 26>;
+			clocks = <&sysc MT7621_CLK_PCIE2>;
+			phys = <&pcie2_phy 0>;
+			phy-names = "pcie-phy2";
 			ranges;
 		};
 	};
-- 
2.25.1

[PATCH 5/5] staging: mt7621-pci: parse some dt properties from root port child nodes

[Sergio Paracuellos]

Properties 'clocks', 'resets' and 'phys' have been moved from parent
node to the root port childs. Hence we have to adapt the way device
tree is parsed in driver code to properly align things and make all
the stuff work.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 8d14d0f9f769..a88ca98aeb9a 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -290,6 +290,7 @@ static int mt7621_pci_parse_request_of_pci_ranges(struct pci_host_bridge *host)
 }
 
 static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
+				  struct device_node *node,
 				  int slot)
 {
 	struct mt7621_pcie_port *port;
@@ -305,24 +306,24 @@ static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
 	if (IS_ERR(port->base))
 		return PTR_ERR(port->base);
 
-	snprintf(name, sizeof(name), "pcie%d", slot);
-	port->clk = devm_clk_get(dev, name);
+	port->clk = devm_get_clk_from_child(dev, node, NULL);
 	if (IS_ERR(port->clk)) {
 		dev_err(dev, "failed to get pcie%d clock\n", slot);
 		return PTR_ERR(port->clk);
 	}
 
-	snprintf(name, sizeof(name), "pcie%d", slot);
-	port->pcie_rst = devm_reset_control_get_exclusive(dev, name);
+	port->pcie_rst = of_reset_control_get_exclusive(node, NULL);
 	if (PTR_ERR(port->pcie_rst) == -EPROBE_DEFER) {
 		dev_err(dev, "failed to get pcie%d reset control\n", slot);
 		return PTR_ERR(port->pcie_rst);
 	}
 
 	snprintf(name, sizeof(name), "pcie-phy%d", slot);
-	port->phy = devm_phy_get(dev, name);
-	if (IS_ERR(port->phy) && slot != 1)
+	port->phy = devm_of_phy_get(dev, node, name);
+	if (IS_ERR(port->phy)) {
+		dev_err(dev, "failed to get pcie-phy%d\n", slot);
 		return PTR_ERR(port->phy);
+	}
 
 	port->gpio_rst = devm_gpiod_get_index_optional(dev, "reset", slot,
 						       GPIOD_OUT_LOW);
@@ -363,7 +364,7 @@ static int mt7621_pcie_parse_dt(struct mt7621_pcie *pcie)
 
 		slot = PCI_SLOT(err);
 
-		err = mt7621_pcie_parse_port(pcie, slot);
+		err = mt7621_pcie_parse_port(pcie, child, slot);
 		if (err) {
 			of_node_put(child);
 			return err;
-- 
2.25.1

Re: [PATCH 5/5] staging: mt7621-pci: parse some dt properties from root port child nodes

[Dan Carpenter]

On Sat, Jun 05, 2021 at 09:30:23AM +0200, Sergio Paracuellos wrote:
> Properties 'clocks', 'resets' and 'phys' have been moved from parent
> node to the root port childs. Hence we have to adapt the way device
> tree is parsed in driver code to properly align things and make all
> the stuff work.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

It sounds like this commit needs a fixes tag?  What does "to properly
align things and make all the stuff work." in terms of what the user
sees?

regards,
dan carpenter

Re: [PATCH 5/5] staging: mt7621-pci: parse some dt properties from root port child nodes

[Sergio Paracuellos]

Hi Dan,

On Mon, Jun 7, 2021 at 8:59 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Sat, Jun 05, 2021 at 09:30:23AM +0200, Sergio Paracuellos wrote:
> > Properties 'clocks', 'resets' and 'phys' have been moved from parent
> > node to the root port childs. Hence we have to adapt the way device
> > tree is parsed in driver code to properly align things and make all
> > the stuff work.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>
> It sounds like this commit needs a fixes tag?  What does "to properly
> align things and make all the stuff work." in terms of what the user
> sees?

I submitted this driver to get mainlined and when bindings have been
reviewed I've been told to move this stuff into child nodes. Until now
all was also being properly working but with these properties defined
in the parent node. So I don't think any Fixes tag is needed here. So
hopefully changes on this patchset are the last need to get this
properly mainlined. I've been told to just make a 'git mv' without
zero changes from the staging driver, that's why I am submitting
changes to staging before.

>
> regards,
> dan carpenter

Best regards,
    Sergio Paracuellos

>

Re: [PATCH 5/5] staging: mt7621-pci: parse some dt properties from root port child nodes

[Dan Carpenter]

On Mon, Jun 07, 2021 at 09:11:13AM +0200, Sergio Paracuellos wrote:
> Hi Dan,
> 
> On Mon, Jun 7, 2021 at 8:59 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Sat, Jun 05, 2021 at 09:30:23AM +0200, Sergio Paracuellos wrote:
> > > Properties 'clocks', 'resets' and 'phys' have been moved from parent
> > > node to the root port childs. Hence we have to adapt the way device
> > > tree is parsed in driver code to properly align things and make all
> > > the stuff work.
> > >
> > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> >
> > It sounds like this commit needs a fixes tag?  What does "to properly
> > align things and make all the stuff work." in terms of what the user
> > sees?
> 
> I submitted this driver to get mainlined and when bindings have been
> reviewed I've been told to move this stuff into child nodes. Until now
> all was also being properly working but with these properties defined
> in the parent node. So I don't think any Fixes tag is needed here. So
> hopefully changes on this patchset are the last need to get this
> properly mainlined. I've been told to just make a 'git mv' without
> zero changes from the staging driver, that's why I am submitting
> changes to staging before.

I'm really trying to understand how this affects the user experience but
it sounds like you don't know either but you were told it was the
correct thing and it seems to work?  That's not ideal but I can live
with it I guess...  I guess hopefully no change but it's just a
correctness issue?

Btw, we moved from devm_reset_control_get_exclusive() to
of_reset_control_get_exclusive().  Does that mean we need to add a call
to reset_control_put() in the remove() path?

regards,
dan carpenter

Re: [PATCH 5/5] staging: mt7621-pci: parse some dt properties from root port child nodes

[Sergio Paracuellos]

Hi Dan,

On Mon, Jun 7, 2021 at 12:37 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, Jun 07, 2021 at 09:11:13AM +0200, Sergio Paracuellos wrote:
> > Hi Dan,
> >
> > On Mon, Jun 7, 2021 at 8:59 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Sat, Jun 05, 2021 at 09:30:23AM +0200, Sergio Paracuellos wrote:
> > > > Properties 'clocks', 'resets' and 'phys' have been moved from parent
> > > > node to the root port childs. Hence we have to adapt the way device
> > > > tree is parsed in driver code to properly align things and make all
> > > > the stuff work.
> > > >
> > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > >
> > > It sounds like this commit needs a fixes tag?  What does "to properly
> > > align things and make all the stuff work." in terms of what the user
> > > sees?
> >
> > I submitted this driver to get mainlined and when bindings have been
> > reviewed I've been told to move this stuff into child nodes. Until now
> > all was also being properly working but with these properties defined
> > in the parent node. So I don't think any Fixes tag is needed here. So
> > hopefully changes on this patchset are the last need to get this
> > properly mainlined. I've been told to just make a 'git mv' without
> > zero changes from the staging driver, that's why I am submitting
> > changes to staging before.
>
> I'm really trying to understand how this affects the user experience but
> it sounds like you don't know either but you were told it was the
> correct thing and it seems to work?

What do you mean with "user experience" here? So to work with the
future mainlined driver we need the dts file to be aligned with device
tree parsing code. If we move these properties into child nodes
(previous patch do this) and the code is properly aligned, for the
user the change is transparent. This SoC is mostly used in openWRT
where new versions compile new code and device tree completely so I
don't expect any compatibility problems also because of these changes,
AFAICT.

> That's not ideal but I can live
> with it I guess...  I guess hopefully no change but it's just a
> correctness issue?

Seems that the bindings are more correct, moving the properties into
child nodes.

>
> Btw, we moved from devm_reset_control_get_exclusive() to
> of_reset_control_get_exclusive().  Does that mean we need to add a call
> to reset_control_put() in the remove() path?

Yes this has moved because we need to access the child node with
'device_node' instead of 'device'. It seems there is not another
possibility with devm_* like the ones we have with clk and phy apis.
Ok, so I have to manually call 'reset_control_put'. Will add it, thanks.

Best regards,
   Sergio Paracuellos

> regards,
> dan carpenter
>

Re: [PATCH 5/5] staging: mt7621-pci: parse some dt properties from root port child nodes

[Sergio Paracuellos]

Hi Dan,

On Mon, Jun 7, 2021 at 1:10 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi Dan,
>
> On Mon, Jun 7, 2021 at 12:37 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Mon, Jun 07, 2021 at 09:11:13AM +0200, Sergio Paracuellos wrote:
> > > Hi Dan,
> > >
> > > On Mon, Jun 7, 2021 at 8:59 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Sat, Jun 05, 2021 at 09:30:23AM +0200, Sergio Paracuellos wrote:
> > > > > Properties 'clocks', 'resets' and 'phys' have been moved from parent
> > > > > node to the root port childs. Hence we have to adapt the way device
> > > > > tree is parsed in driver code to properly align things and make all
> > > > > the stuff work.
> > > > >
> > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > >
> > > > It sounds like this commit needs a fixes tag?  What does "to properly
> > > > align things and make all the stuff work." in terms of what the user
> > > > sees?
> > >
> > > I submitted this driver to get mainlined and when bindings have been
> > > reviewed I've been told to move this stuff into child nodes. Until now
> > > all was also being properly working but with these properties defined
> > > in the parent node. So I don't think any Fixes tag is needed here. So
> > > hopefully changes on this patchset are the last need to get this
> > > properly mainlined. I've been told to just make a 'git mv' without
> > > zero changes from the staging driver, that's why I am submitting
> > > changes to staging before.
> >
> > I'm really trying to understand how this affects the user experience but
> > it sounds like you don't know either but you were told it was the
> > correct thing and it seems to work?
>
> What do you mean with "user experience" here? So to work with the
> future mainlined driver we need the dts file to be aligned with device
> tree parsing code. If we move these properties into child nodes
> (previous patch do this) and the code is properly aligned, for the
> user the change is transparent. This SoC is mostly used in openWRT
> where new versions compile new code and device tree completely so I
> don't expect any compatibility problems also because of these changes,
> AFAICT.
>
> > That's not ideal but I can live
> > with it I guess...  I guess hopefully no change but it's just a
> > correctness issue?
>
> Seems that the bindings are more correct, moving the properties into
> child nodes.
>
> >
> > Btw, we moved from devm_reset_control_get_exclusive() to
> > of_reset_control_get_exclusive().  Does that mean we need to add a call
> > to reset_control_put() in the remove() path?
>
> Yes this has moved because we need to access the child node with
> 'device_node' instead of 'device'. It seems there is not another
> possibility with devm_* like the ones we have with clk and phy apis.
> Ok, so I have to manually call 'reset_control_put'. Will add it, thanks.

Should this be enough for error and remove paths, right?

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c
b/drivers/staging/mt7621-pci/pci-mt7621.c
index 2e1cd5cc1eec..8e20c5ede48d 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -294,6 +294,7 @@ static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
        struct device *dev = pcie->dev;
        struct platform_device *pdev = to_platform_device(dev);
        char name[10];
+       int err;

        port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
        if (!port)
@@ -319,14 +320,16 @@ static int mt7621_pcie_parse_port(struct
mt7621_pcie *pcie,
        port->phy = devm_of_phy_get(dev, node, name);
        if (IS_ERR(port->phy)) {
                dev_err(dev, "failed to get pcie-phy%d\n", slot);
-               return PTR_ERR(port->phy);
+               err = PTR_ERR(port->phy);
+               goto remove_reset;
        }

        port->gpio_rst = devm_gpiod_get_index_optional(dev, "reset", slot,
                                                       GPIOD_OUT_LOW);
        if (IS_ERR(port->gpio_rst)) {
                dev_err(dev, "Failed to get GPIO for PCIe%d\n", slot);
-               return PTR_ERR(port->gpio_rst);
+               err = PTR_ERR(port->gpio_rst);
+               goto remove_reset;
        }

        port->slot = slot;
@@ -336,6 +339,10 @@ static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
        list_add_tail(&port->list, &pcie->ports);

        return 0;
+
+remove_reset:
+       reset_control_put(port->pcie_rst);
+       return err;
 }

 static int mt7621_pcie_parse_dt(struct mt7621_pcie *pcie)
@@ -596,6 +603,17 @@ static int mt7621_pci_probe(struct platform_device *pdev)
        return mt7621_pcie_register_host(bridge);
 }

+static int mt7621_pci_remove(struct platform_device *pdev)
+{
+       struct mt7621_pcie *pcie = platform_get_drvdata(pdev);
+       struct mt7621_pcie_port *port;
+
+       list_for_each_entry(port, &pcie->ports, list)
+               reset_control_put(port->pcie_rst);
+
+       return 0;
+}
+
 static const struct of_device_id mt7621_pci_ids[] = {
        { .compatible = "mediatek,mt7621-pci" },
        {},
@@ -604,6 +622,7 @@ MODULE_DEVICE_TABLE(of, mt7621_pci_ids);

 static struct platform_driver mt7621_pci_driver = {
        .probe = mt7621_pci_probe,
+       .remove = mt7621_pci_remove,
        .driver = {
                .name = "mt7621-pci",
                .of_match_table = of_match_ptr(mt7621_pci_ids),

Thanks,
    Sergio Paracuellos
>
> Best regards,
>    Sergio Paracuellos
>
> > regards,
> > dan carpenter
> >

Re: [PATCH 5/5] staging: mt7621-pci: parse some dt properties from root port child nodes

[Dan Carpenter]

On Mon, Jun 07, 2021 at 01:30:58PM +0200, Sergio Paracuellos wrote:
> Hi Dan,
> 
> On Mon, Jun 7, 2021 at 1:10 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > Hi Dan,
> >
> > On Mon, Jun 7, 2021 at 12:37 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Mon, Jun 07, 2021 at 09:11:13AM +0200, Sergio Paracuellos wrote:
> > > > Hi Dan,
> > > >
> > > > On Mon, Jun 7, 2021 at 8:59 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > >
> > > > > On Sat, Jun 05, 2021 at 09:30:23AM +0200, Sergio Paracuellos wrote:
> > > > > > Properties 'clocks', 'resets' and 'phys' have been moved from parent
> > > > > > node to the root port childs. Hence we have to adapt the way device
> > > > > > tree is parsed in driver code to properly align things and make all
> > > > > > the stuff work.
> > > > > >
> > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > >
> > > > > It sounds like this commit needs a fixes tag?  What does "to properly
> > > > > align things and make all the stuff work." in terms of what the user
> > > > > sees?
> > > >
> > > > I submitted this driver to get mainlined and when bindings have been
> > > > reviewed I've been told to move this stuff into child nodes. Until now
> > > > all was also being properly working but with these properties defined
> > > > in the parent node. So I don't think any Fixes tag is needed here. So
> > > > hopefully changes on this patchset are the last need to get this
> > > > properly mainlined. I've been told to just make a 'git mv' without
> > > > zero changes from the staging driver, that's why I am submitting
> > > > changes to staging before.
> > >
> > > I'm really trying to understand how this affects the user experience but
> > > it sounds like you don't know either but you were told it was the
> > > correct thing and it seems to work?
> >
> > What do you mean with "user experience" here? So to work with the
> > future mainlined driver we need the dts file to be aligned with device
> > tree parsing code. If we move these properties into child nodes
> > (previous patch do this) and the code is properly aligned, for the
> > user the change is transparent. This SoC is mostly used in openWRT
> > where new versions compile new code and device tree completely so I
> > don't expect any compatibility problems also because of these changes,
> > AFAICT.
> >
> > > That's not ideal but I can live
> > > with it I guess...  I guess hopefully no change but it's just a
> > > correctness issue?
> >
> > Seems that the bindings are more correct, moving the properties into
> > child nodes.
> >
> > >
> > > Btw, we moved from devm_reset_control_get_exclusive() to
> > > of_reset_control_get_exclusive().  Does that mean we need to add a call
> > > to reset_control_put() in the remove() path?
> >
> > Yes this has moved because we need to access the child node with
> > 'device_node' instead of 'device'. It seems there is not another
> > possibility with devm_* like the ones we have with clk and phy apis.
> > Ok, so I have to manually call 'reset_control_put'. Will add it, thanks.
> 
> Should this be enough for error and remove paths, right?

Looks good to me...  I'm not an expert on this at all...  (When I ask
a question about something it's never rhetorical question so if you had
told me it wasn't required then I would have accepted that as an answer
as well).

regards,
dan carpenter

Re: [PATCH 5/5] staging: mt7621-pci: parse some dt properties from root port child nodes

[Sergio Paracuellos]

On Mon, Jun 7, 2021 at 2:05 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, Jun 07, 2021 at 01:30:58PM +0200, Sergio Paracuellos wrote:
> > Hi Dan,
> >
> > On Mon, Jun 7, 2021 at 1:10 PM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > >
> > > Hi Dan,
> > >
> > > On Mon, Jun 7, 2021 at 12:37 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Mon, Jun 07, 2021 at 09:11:13AM +0200, Sergio Paracuellos wrote:
> > > > > Hi Dan,
> > > > >
> > > > > On Mon, Jun 7, 2021 at 8:59 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > >
> > > > > > On Sat, Jun 05, 2021 at 09:30:23AM +0200, Sergio Paracuellos wrote:
> > > > > > > Properties 'clocks', 'resets' and 'phys' have been moved from parent
> > > > > > > node to the root port childs. Hence we have to adapt the way device
> > > > > > > tree is parsed in driver code to properly align things and make all
> > > > > > > the stuff work.
> > > > > > >
> > > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > > >
> > > > > > It sounds like this commit needs a fixes tag?  What does "to properly
> > > > > > align things and make all the stuff work." in terms of what the user
> > > > > > sees?
> > > > >
> > > > > I submitted this driver to get mainlined and when bindings have been
> > > > > reviewed I've been told to move this stuff into child nodes. Until now
> > > > > all was also being properly working but with these properties defined
> > > > > in the parent node. So I don't think any Fixes tag is needed here. So
> > > > > hopefully changes on this patchset are the last need to get this
> > > > > properly mainlined. I've been told to just make a 'git mv' without
> > > > > zero changes from the staging driver, that's why I am submitting
> > > > > changes to staging before.
> > > >
> > > > I'm really trying to understand how this affects the user experience but
> > > > it sounds like you don't know either but you were told it was the
> > > > correct thing and it seems to work?
> > >
> > > What do you mean with "user experience" here? So to work with the
> > > future mainlined driver we need the dts file to be aligned with device
> > > tree parsing code. If we move these properties into child nodes
> > > (previous patch do this) and the code is properly aligned, for the
> > > user the change is transparent. This SoC is mostly used in openWRT
> > > where new versions compile new code and device tree completely so I
> > > don't expect any compatibility problems also because of these changes,
> > > AFAICT.
> > >
> > > > That's not ideal but I can live
> > > > with it I guess...  I guess hopefully no change but it's just a
> > > > correctness issue?
> > >
> > > Seems that the bindings are more correct, moving the properties into
> > > child nodes.
> > >
> > > >
> > > > Btw, we moved from devm_reset_control_get_exclusive() to
> > > > of_reset_control_get_exclusive().  Does that mean we need to add a call
> > > > to reset_control_put() in the remove() path?
> > >
> > > Yes this has moved because we need to access the child node with
> > > 'device_node' instead of 'device'. It seems there is not another
> > > possibility with devm_* like the ones we have with clk and phy apis.
> > > Ok, so I have to manually call 'reset_control_put'. Will add it, thanks.
> >
> > Should this be enough for error and remove paths, right?
>
> Looks good to me...  I'm not an expert on this at all...  (When I ask
> a question about something it's never rhetorical question so if you had
> told me it wasn't required then I would have accepted that as an answer
> as well).

I see :). But in this case since we are getting that without using
devm_* APIS the put is needed, AFAICT.

Thanks,
    Sergio Paracuellos

>
> regards,
> dan carpenter
>

Re: [PATCH 5/5] staging: mt7621-pci: parse some dt properties from root port child nodes

[Dan Carpenter]

On Mon, Jun 07, 2021 at 01:10:57PM +0200, Sergio Paracuellos wrote:
> Hi Dan,
> 
> On Mon, Jun 7, 2021 at 12:37 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Mon, Jun 07, 2021 at 09:11:13AM +0200, Sergio Paracuellos wrote:
> > > Hi Dan,
> > >
> > > On Mon, Jun 7, 2021 at 8:59 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Sat, Jun 05, 2021 at 09:30:23AM +0200, Sergio Paracuellos wrote:
> > > > > Properties 'clocks', 'resets' and 'phys' have been moved from parent
> > > > > node to the root port childs. Hence we have to adapt the way device
> > > > > tree is parsed in driver code to properly align things and make all
> > > > > the stuff work.
> > > > >
> > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > >
> > > > It sounds like this commit needs a fixes tag?  What does "to properly
> > > > align things and make all the stuff work." in terms of what the user
> > > > sees?
> > >
> > > I submitted this driver to get mainlined and when bindings have been
> > > reviewed I've been told to move this stuff into child nodes. Until now
> > > all was also being properly working but with these properties defined
> > > in the parent node. So I don't think any Fixes tag is needed here. So
> > > hopefully changes on this patchset are the last need to get this
> > > properly mainlined. I've been told to just make a 'git mv' without
> > > zero changes from the staging driver, that's why I am submitting
> > > changes to staging before.
> >
> > I'm really trying to understand how this affects the user experience but
> > it sounds like you don't know either but you were told it was the
> > correct thing and it seems to work?
> 
> What do you mean with "user experience" here? So to work with the
> future mainlined driver we need the dts file to be aligned with device
> tree parsing code. If we move these properties into child nodes
> (previous patch do this) and the code is properly aligned, for the
> user the change is transparent. This SoC is mostly used in openWRT
> where new versions compile new code and device tree completely so I
> don't expect any compatibility problems also because of these changes,
> AFAICT.
> 

Basically what I want from the commit message is that "I do not expect
most users to notice this change, but it is the correct thing to do".
The original commit message said "make all that stuff work" and it
wasn't clear which things were not working...

regards,
dan carpenter

Re: [PATCH 5/5] staging: mt7621-pci: parse some dt properties from root port child nodes

[Sergio Paracuellos]

On Mon, Jun 7, 2021 at 3:20 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, Jun 07, 2021 at 01:10:57PM +0200, Sergio Paracuellos wrote:
> > Hi Dan,
> >
> > On Mon, Jun 7, 2021 at 12:37 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Mon, Jun 07, 2021 at 09:11:13AM +0200, Sergio Paracuellos wrote:
> > > > Hi Dan,
> > > >
> > > > On Mon, Jun 7, 2021 at 8:59 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > >
> > > > > On Sat, Jun 05, 2021 at 09:30:23AM +0200, Sergio Paracuellos wrote:
> > > > > > Properties 'clocks', 'resets' and 'phys' have been moved from parent
> > > > > > node to the root port childs. Hence we have to adapt the way device
> > > > > > tree is parsed in driver code to properly align things and make all
> > > > > > the stuff work.
> > > > > >
> > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > >
> > > > > It sounds like this commit needs a fixes tag?  What does "to properly
> > > > > align things and make all the stuff work." in terms of what the user
> > > > > sees?
> > > >
> > > > I submitted this driver to get mainlined and when bindings have been
> > > > reviewed I've been told to move this stuff into child nodes. Until now
> > > > all was also being properly working but with these properties defined
> > > > in the parent node. So I don't think any Fixes tag is needed here. So
> > > > hopefully changes on this patchset are the last need to get this
> > > > properly mainlined. I've been told to just make a 'git mv' without
> > > > zero changes from the staging driver, that's why I am submitting
> > > > changes to staging before.
> > >
> > > I'm really trying to understand how this affects the user experience but
> > > it sounds like you don't know either but you were told it was the
> > > correct thing and it seems to work?
> >
> > What do you mean with "user experience" here? So to work with the
> > future mainlined driver we need the dts file to be aligned with device
> > tree parsing code. If we move these properties into child nodes
> > (previous patch do this) and the code is properly aligned, for the
> > user the change is transparent. This SoC is mostly used in openWRT
> > where new versions compile new code and device tree completely so I
> > don't expect any compatibility problems also because of these changes,
> > AFAICT.
> >
>
> Basically what I want from the commit message is that "I do not expect
> most users to notice this change, but it is the correct thing to do".
> The original commit message said "make all that stuff work" and it
> wasn't clear which things were not working...

I expected the part of "properties xxxx have been moved"  and the
"align things" stuff to be clear enough but it seems I failed in my
intention :).

>
> regards,
> dan carpenter
>

Best regards,
    Sergio Paracuellos