lkml.org 
[lkml]   [2020]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v1] dmaengine: pch_dma: use generic power management
    On Mon, Jul 27, 2020 at 02:19:14PM +0300, Andy Shevchenko wrote:
    > On Mon, Jul 27, 2020 at 1:16 PM Vinod Koul <vkoul@kernel.org> wrote:
    > > On 20-07-20, 17:07, Vaibhav Gupta wrote:
    > > > Drivers using legacy PM have to manage PCI states and device's PM states
    > > > themselves. They also need to take care of configuration registers.
    > > >
    > > > With improved and powerful support of generic PM, PCI Core takes care of
    > > > above mentioned, device-independent, jobs.
    > > >
    > > > This driver makes use of PCI helper functions like
    > > > pci_save/restore_state(), pci_enable/disable_device(),
    > > > and pci_set_power_state() to do required operations. In generic mode, they
    > > > are no longer needed.
    > > >
    > > > Change function parameter in both .suspend() and .resume() to
    > > > "struct device*" type. Use dev_get_drvdata() to get drv data.
    > >
    > > You are doing too many things in One patch. Do consider splitting them
    > > up to a change per patch. for example using __maybe could be one patch,
    > > removing code is suspend-resume callbacks would be other one.
    >
    > Vinod, while I completely agree with you in general, in this case it
    > would make more unnecessary churn and will be rather unhelpful in all
    > ways: for the contributor to do this work, for the reader to collect
    > all the pieces. It also will be a bisectability issue, because the
    > #ifdeffery replacement (IIRC you need to move from CONFIG_PM to
    > CONFIG_PM_SLEEP). I really don't see any advantages of the splitting
    > here.
    >
    > > > Compile-tested only.
    > >
    > > I would like to see some testing before we merge this
    >
    > I have hardware to test (Intel Minnowboard v1) but have no time. And
    > taking into account that I did similar changes for many other drivers,
    > I can give my
    > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
    > and take responsibility if somebody complains in the future (I don't
    > believe it will be one).
    >
    Thanks! :D
    > P.S. Another scenario if Vaibhav can find that board (there were
    > dozens of thousands at least produced and floating on the second hand
    > market) and test himself. It may be good since he touches the full lot
    > of PCH (EGT20) drivers.
    >
    I cannot promise, but surely will try to get my hands on it :)

    Thanks
    Vaibhav Gupta
    > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
    > > > ---
    > > > drivers/dma/pch_dma.c | 35 +++++++++--------------------------
    > > > 1 file changed, 9 insertions(+), 26 deletions(-)
    > > >
    > > > diff --git a/drivers/dma/pch_dma.c b/drivers/dma/pch_dma.c
    > > > index a3b0b4c56a19..e93005837e3f 100644
    > > > --- a/drivers/dma/pch_dma.c
    > > > +++ b/drivers/dma/pch_dma.c
    > > > @@ -735,8 +735,7 @@ static irqreturn_t pd_irq(int irq, void *devid)
    > > > return ret0 | ret2;
    > > > }
    > > >
    > > > -#ifdef CONFIG_PM
    > > > -static void pch_dma_save_regs(struct pch_dma *pd)
    > > > +static void __maybe_unused pch_dma_save_regs(struct pch_dma *pd)
    > > > {
    > > > struct pch_dma_chan *pd_chan;
    > > > struct dma_chan *chan, *_c;
    > > > @@ -759,7 +758,7 @@ static void pch_dma_save_regs(struct pch_dma *pd)
    > > > }
    > > > }
    > > >
    > > > -static void pch_dma_restore_regs(struct pch_dma *pd)
    > > > +static void __maybe_unused pch_dma_restore_regs(struct pch_dma *pd)
    > > > {
    > > > struct pch_dma_chan *pd_chan;
    > > > struct dma_chan *chan, *_c;
    > > > @@ -782,40 +781,25 @@ static void pch_dma_restore_regs(struct pch_dma *pd)
    > > > }
    > > > }
    > > >
    > > > -static int pch_dma_suspend(struct pci_dev *pdev, pm_message_t state)
    > > > +static int __maybe_unused pch_dma_suspend(struct device *dev)
    > > > {
    > > > - struct pch_dma *pd = pci_get_drvdata(pdev);
    > > > + struct pch_dma *pd = dev_get_drvdata(dev);
    > > >
    > > > if (pd)
    > > > pch_dma_save_regs(pd);
    > > >
    > > > - pci_save_state(pdev);
    > > > - pci_disable_device(pdev);
    > > > - pci_set_power_state(pdev, pci_choose_state(pdev, state));
    > > > -
    > > > return 0;
    > > > }
    > > >
    > > > -static int pch_dma_resume(struct pci_dev *pdev)
    > > > +static int __maybe_unused pch_dma_resume(struct device *dev)
    > > > {
    > > > - struct pch_dma *pd = pci_get_drvdata(pdev);
    > > > - int err;
    > > > -
    > > > - pci_set_power_state(pdev, PCI_D0);
    > > > - pci_restore_state(pdev);
    > > > -
    > > > - err = pci_enable_device(pdev);
    > > > - if (err) {
    > > > - dev_dbg(&pdev->dev, "failed to enable device\n");
    > > > - return err;
    > > > - }
    > > > + struct pch_dma *pd = dev_get_drvdata(dev);
    > > >
    > > > if (pd)
    > > > pch_dma_restore_regs(pd);
    > > >
    > > > return 0;
    > > > }
    > > > -#endif
    > > >
    > > > static int pch_dma_probe(struct pci_dev *pdev,
    > > > const struct pci_device_id *id)
    > > > @@ -993,15 +977,14 @@ static const struct pci_device_id pch_dma_id_table[] = {
    > > > { 0, },
    > > > };
    > > >
    > > > +static SIMPLE_DEV_PM_OPS(pch_dma_pm_ops, pch_dma_suspend, pch_dma_resume);
    > > > +
    > > > static struct pci_driver pch_dma_driver = {
    > > > .name = DRV_NAME,
    > > > .id_table = pch_dma_id_table,
    > > > .probe = pch_dma_probe,
    > > > .remove = pch_dma_remove,
    > > > -#ifdef CONFIG_PM
    > > > - .suspend = pch_dma_suspend,
    > > > - .resume = pch_dma_resume,
    > > > -#endif
    > > > + .driver.pm = &pch_dma_pm_ops,
    > > > };
    > > >
    > > > module_pci_driver(pch_dma_driver);
    > > > --
    > > > 2.27.0
    > >
    > > --
    > > ~Vinod
    >
    >
    >
    > --
    > With Best Regards,
    > Andy Shevchenko

    \
     
     \ /
      Last update: 2020-07-27 15:10    [W:2.834 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site