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.paracuellos@xxxxxxxxx>
> ---
> 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().
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?
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
