Messages in this thread | | | From | "Winkler, Tomas" <> | Subject | RE: [char-misc-next] mei: simplify error handling via devres function. | Date | Sat, 21 Jan 2017 10:12:24 +0000 |
| |
> > On Fri, Jan 20, 2017 at 7:22 PM, Tomas Winkler <tomas.winkler@intel.com> > 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 > > <andriy.shevchenko@linux.intel.com> > > https://lkml.org/lkml/2016/2/1/339 > > Perhaps you may leave SoB. > But let me review the code.
No problem, though we also had some internal code before, you were just first to send it out ;)
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com> > > > - * 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.
Okay, got it. > > > -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. Please explain, we rely on pci_get_drvdata() returning NULL when unregistered. > > > 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 I believe we need it on suspend as we are going over irq request again in resume. Please provide more info you if you still insist. > > > 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? Please be more verbose. > > > + 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.
There is no simple accessor for that, it's easier to copy the two dwords then going over the function calls.
> > 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.
Thanks Tomas
| |