On Tue, Aug 14, 2018 at 12:43 PM, Dan Carpenter <dan.carpen...@oracle.com> wrote: > On Sun, Aug 12, 2018 at 08:45:49PM +0200, Sergio Paracuellos wrote: >> Driver probe function is a mess and shall be refactored a lot. At first >> make use of assert and deassert control factoring out a new function >> called 'mt7621_pcie_enable_port'. >> >> Signed-off-by: Sergio Paracuellos <sergio.paracuel...@gmail.com> >> --- >> drivers/staging/mt7621-pci/pci-mt7621.c | 92 >> ++++++++++++++++----------------- >> 1 file changed, 45 insertions(+), 47 deletions(-) >> >> diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c >> b/drivers/staging/mt7621-pci/pci-mt7621.c >> index d6e8a6d..0b1ac5b 100644 >> --- a/drivers/staging/mt7621-pci/pci-mt7621.c >> +++ b/drivers/staging/mt7621-pci/pci-mt7621.c >> @@ -486,6 +486,48 @@ static int mt7621_pcie_parse_dt(struct mt7621_pcie >> *pcie) >> return 0; >> } >> >> +static void mt7621_pcie_port_free(struct mt7621_pcie_port *port) >> +{ >> + struct mt7621_pcie *pcie = port->pcie; >> + struct device *dev = pcie->dev; >> + >> + devm_iounmap(dev, port->base); >> + list_del(&port->list); >> + devm_kfree(dev, port); >> +} >> + >> +static void mt7621_pcie_enable_port(struct mt7621_pcie_port *port) >> +{ >> + struct mt7621_pcie *pcie = port->pcie; >> + struct device *dev = pcie->dev; >> + u32 slot = port->slot; >> + u32 val = 0; >> + int err; >> + >> + err = clk_prepare_enable(port->pcie_clk); >> + if (err) { >> + dev_err(dev, "failed to enable pcie%d clock\n", slot); >> + mt7621_pcie_port_free(port); >> + return; >> + } > > This is abit ugly. It's a layering violation. We should return negative > error codes on failure and the caller calls mt7621_pcie_port_free().
Thanks, for the feedback, Dan. I agree with the layering violation, I'll redo this after this changes are tested and see if they work as expected. > Also I don't understand why we're freeing devm_ resources, can't we > just call list_del() in the mt7621_pci_probe() and get rid of the > mt7621_pcie_port_free() function? Maybe is cleaner to do in the way you are pointing out here and just call list_del in the probe function. > > regards, > dan carpenter > Best regards, Sergio Paracuellos _______________________________________________ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel