Hi Dan,
On Mon, Jun 7, 2021 at 1:10 PM Sergio Paracuellos
<sergio.paracuellos@xxxxxxxxx> wrote:
>
> Hi Dan,
>
> On Mon, Jun 7, 2021 at 12:37 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx>
> > > >
> > > > 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
> >
