From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752589AbdATWBj (ORCPT ); Fri, 20 Jan 2017 17:01:39 -0500 Received: from mail-qk0-f193.google.com ([209.85.220.193]:33457 "EHLO mail-qk0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752276AbdATWAJ (ORCPT ); Fri, 20 Jan 2017 17:00:09 -0500 MIME-Version: 1.0 In-Reply-To: <1484932972-3442-1-git-send-email-tomas.winkler@intel.com> References: <1484932972-3442-1-git-send-email-tomas.winkler@intel.com> From: Andy Shevchenko Date: Sat, 21 Jan 2017 00:00:07 +0200 Message-ID: Subject: Re: [char-misc-next] mei: simplify error handling via devres function. To: Tomas Winkler Cc: Greg Kroah-Hartman , Alexander Usyskin , "linux-kernel@vger.kernel.org" , Andy Shevchenko Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 20, 2017 at 7:22 PM, Tomas Winkler wrote: > Use devm_ and pcim_ functions to make error handling > simpler and code smaller and tidier. > > Based on original patch by > mei: me: use managed functions pcim_* and devm_* > Andy Shevchenko > https://lkml.org/lkml/2016/2/1/339 Perhaps you may leave SoB. But let me review the code. > Cc: Andy Shevchenko > Signed-off-by: Tomas Winkler > Signed-off-by: Alexander Usyskin > - * Return: The mei_device_device pointer on success, NULL on failure. > + * Return: The mei_device pointer on success, NULL on failure. This is not related. > -struct mei_device *mei_me_dev_init(struct pci_dev *pdev, > - const struct mei_cfg *cfg) > +struct mei_device *devm_mei_me_init(struct pci_dev *pdev, > + const struct mei_cfg *cfg) It's not exactly in devm_ namespace. (They have no corresponding devm_*_fini, etc.) Better to leave same name in spite of calling devm_* inside. > -struct mei_device *mei_txe_dev_init(struct pci_dev *pdev) > +struct mei_device *devm_mei_txe_init(struct pci_dev *pdev) Ditto. > end: > + pci_set_drvdata(pdev, NULL); Not needed. > static void mei_me_remove(struct pci_dev *pdev) > { > struct mei_device *dev; > - struct mei_me_hw *hw; > > dev = pci_get_drvdata(pdev); > if (!dev) > @@ -276,33 +260,19 @@ static void mei_me_remove(struct pci_dev *pdev) > if (mei_pg_is_enabled(dev)) > pm_runtime_get_noresume(&pdev->dev); > + pci_set_drvdata(pdev, NULL); Ditto. > - free_irq(pdev->irq, dev); > + devm_free_irq(&pdev->dev, pdev->irq, dev); > pci_disable_msi(pdev); All three not needed > return 0; > @@ -75,22 +64,22 @@ static int mei_txe_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > { > struct mei_device *dev; > struct mei_txe_hw *hw; > + const int mask = BIT(SEC_BAR) | BIT(BRIDGE_BAR); First line? > + memcpy(hw->mem_addr, pcim_iomap_table(pdev), sizeof(hw->mem_addr)); Why? It is kept by PCI core, you don't need a copy. > end: > + pci_set_drvdata(pdev, NULL); Redundant. > static void mei_txe_remove(struct pci_dev *pdev) > { > + pci_set_drvdata(pdev, NULL); Ditto. > @@ -256,7 +210,7 @@ static int mei_txe_pci_suspend(struct device *device) > - free_irq(pdev->irq, dev); > + devm_free_irq(&pdev->dev, pdev->irq, dev); > pci_disable_msi(pdev); All are redundant. -- With Best Regards, Andy Shevchenko