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

[Sergio Paracuellos]

On Tue, Aug 14, 2018 at 12:43 PM, Dan Carpenter
<dan.carpenter at 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.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().

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