[PATCH v2 04/20] staging: mt7621-pci: factor out 'mt7621_pcie_enable_port' function

Dan Carpenter dan.carpenter at oracle.com
Tue Aug 14 10:43:53 UTC 2018


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 at 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().
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



More information about the devel mailing list