On Tue, Aug 14, 2018 at 12:43 PM, Dan Carpenter
<dan.carpenter@xxxxxxxxxx> 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@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().
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
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
