[Linux-kernel-mentees] [PATCH v1] gpio: ml: ioh: Convert to dev_pm_ops

[Linux-kernel-mentees] [PATCH v1] gpio: ml: ioh: Convert to dev_pm_ops

[Vaibhav Gupta]

Convert the legacy callback .suspend() and .resume()
to the generic ones.

Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/gpio/gpio-ml-ioh.c | 47 +++++++++-----------------------------
 1 file changed, 11 insertions(+), 36 deletions(-)

diff --git a/drivers/gpio/gpio-ml-ioh.c b/drivers/gpio/gpio-ml-ioh.c
index 92b6e958cfed..bb71dccac315 100644
--- a/drivers/gpio/gpio-ml-ioh.c
+++ b/drivers/gpio/gpio-ml-ioh.c
@@ -155,11 +155,10 @@ static int ioh_gpio_direction_input(struct gpio_chip *gpio, unsigned nr)
 	return 0;
 }
 
-#ifdef CONFIG_PM
 /*
  * Save register configuration and disable interrupts.
  */
-static void ioh_gpio_save_reg_conf(struct ioh_gpio *chip)
+static void __maybe_unused ioh_gpio_save_reg_conf(struct ioh_gpio *chip)
 {
 	int i;
 
@@ -185,7 +184,7 @@ static void ioh_gpio_save_reg_conf(struct ioh_gpio *chip)
 /*
  * This function restores the register configuration of the GPIO device.
  */
-static void ioh_gpio_restore_reg_conf(struct ioh_gpio *chip)
+static void __maybe_unused ioh_gpio_restore_reg_conf(struct ioh_gpio *chip)
 {
 	int i;
 
@@ -207,7 +206,6 @@ static void ioh_gpio_restore_reg_conf(struct ioh_gpio *chip)
 				  &chip->reg->ioh_sel_reg[i]);
 	}
 }
-#endif
 
 static int ioh_gpio_to_irq(struct gpio_chip *gpio, unsigned offset)
 {
@@ -522,10 +520,9 @@ static void ioh_gpio_remove(struct pci_dev *pdev)
 	kfree(chip);
 }
 
-#ifdef CONFIG_PM
-static int ioh_gpio_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __maybe_unused ioh_gpio_suspend(struct device *dev)
 {
-	s32 ret;
+	struct pci_dev *pdev = to_pci_dev(dev);
 	struct ioh_gpio *chip = pci_get_drvdata(pdev);
 	unsigned long flags;
 
@@ -533,36 +530,15 @@ static int ioh_gpio_suspend(struct pci_dev *pdev, pm_message_t state)
 	ioh_gpio_save_reg_conf(chip);
 	spin_unlock_irqrestore(&chip->spinlock, flags);
 
-	ret = pci_save_state(pdev);
-	if (ret) {
-		dev_err(&pdev->dev, "pci_save_state Failed-%d\n", ret);
-		return ret;
-	}
-	pci_disable_device(pdev);
-	pci_set_power_state(pdev, PCI_D0);
-	ret = pci_enable_wake(pdev, PCI_D0, 1);
-	if (ret)
-		dev_err(&pdev->dev, "pci_enable_wake Failed -%d\n", ret);
-
 	return 0;
 }
 
-static int ioh_gpio_resume(struct pci_dev *pdev)
+static int __maybe_unused ioh_gpio_resume(struct device *dev)
 {
-	s32 ret;
+	struct pci_dev *pdev = to_pci_dev(dev);
 	struct ioh_gpio *chip = pci_get_drvdata(pdev);
 	unsigned long flags;
 
-	ret = pci_enable_wake(pdev, PCI_D0, 0);
-
-	pci_set_power_state(pdev, PCI_D0);
-	ret = pci_enable_device(pdev);
-	if (ret) {
-		dev_err(&pdev->dev, "pci_enable_device Failed-%d ", ret);
-		return ret;
-	}
-	pci_restore_state(pdev);
-
 	spin_lock_irqsave(&chip->spinlock, flags);
 	iowrite32(0x01, &chip->reg->srst);
 	iowrite32(0x00, &chip->reg->srst);
@@ -571,10 +547,8 @@ static int ioh_gpio_resume(struct pci_dev *pdev)
 
 	return 0;
 }
-#else
-#define ioh_gpio_suspend NULL
-#define ioh_gpio_resume NULL
-#endif
+
+static SIMPLE_DEV_PM_OPS(ioh_gpio_pm_ops, ioh_gpio_suspend, ioh_gpio_resume);
 
 static const struct pci_device_id ioh_gpio_pcidev_id[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_ROHM, 0x802E) },
@@ -587,8 +561,9 @@ static struct pci_driver ioh_gpio_driver = {
 	.id_table = ioh_gpio_pcidev_id,
 	.probe = ioh_gpio_probe,
 	.remove = ioh_gpio_remove,
-	.suspend = ioh_gpio_suspend,
-	.resume = ioh_gpio_resume
+	.driver = {
+		.pm = &ioh_gpio_pm_ops,
+	},
 };
 
 module_pci_driver(ioh_gpio_driver);
-- 
2.26.0

Re: [Linux-kernel-mentees] [PATCH v1] gpio: ml: ioh: Convert to dev_pm_ops

[Andy Shevchenko]

On Thu, Apr 2, 2020 at 6:52 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote:
>
> Convert the legacy callback .suspend() and .resume()
> to the generic ones.
>

Thank you for the patch.

Rather then doing this I think  the best approach is to unify gpio-pch
and gpio-ml-ioh together.
Under umbrella of the task, the clean ups like above are highly appreciated.

-- 
With Best Regards,
Andy Shevchenko

Fwd: [Linux-kernel-mentees] [PATCH v1] gpio: ml: ioh: Convert to dev_pm_ops

[Vaibhav Gupta]

> Thank you for the patch.
>
> Rather then doing this I think  the best approach is to unify gpio-pch
> and gpio-ml-ioh together.
> Under umbrella of the task, the clean ups like above are highly appreciated.

Sure! So, will the patch be accepted or we should scrap it and work on
unifying the gpio-pch and gpio-ml-ioh and other similar driver(s) (if any)?

-- Vaibhav Gupta

Re: [Linux-kernel-mentees] [PATCH v1] gpio: ml: ioh: Convert to dev_pm_ops

[Bjorn Helgaas]

On Thu, Apr 02, 2020 at 09:33:46PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 2, 2020 at 6:52 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote:
> >
> > Convert the legacy callback .suspend() and .resume()
> > to the generic ones.
> 
> Thank you for the patch.
> 
> Rather then doing this I think  the best approach is to unify gpio-pch
> and gpio-ml-ioh together.
> Under umbrella of the task, the clean ups like above are highly appreciated.

I'd be all in favor of that, but what Vaibhav is working toward is
eliminating use of legacy PM in PCI drivers.  I think unifying drivers
is really out of scope for that project.

If you'd rather leave gpio-ml-ioh.c alone for now, I suggest that
Vaibhav move on to other PCI drivers that use legacy PM.  If we
convert all the others away from legacy PM and gpio-ml-ioh.c is the
only one remaining, then I guess we can revisit this :)

Or, maybe converting gpio-ml-ioh.c now, along the lines of
226e6b866d74 ("gpio: pch: Convert to dev_pm_ops"), would be one small
step towards the eventual unification, by making gpio-pch and
gpio-ml-ioh a little more similar.

Bjorn

Re: [Linux-kernel-mentees] [PATCH v1] gpio: ml: ioh: Convert to dev_pm_ops

[Andy Shevchenko]

On Thu, Apr 2, 2020 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Apr 02, 2020 at 09:33:46PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 2, 2020 at 6:52 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote:
> > >
> > > Convert the legacy callback .suspend() and .resume()
> > > to the generic ones.
> >
> > Thank you for the patch.
> >
> > Rather then doing this I think  the best approach is to unify gpio-pch
> > and gpio-ml-ioh together.
> > Under umbrella of the task, the clean ups like above are highly appreciated.
>
> I'd be all in favor of that, but what Vaibhav is working toward is
> eliminating use of legacy PM in PCI drivers.  I think unifying drivers
> is really out of scope for that project.
>
> If you'd rather leave gpio-ml-ioh.c alone for now, I suggest that
> Vaibhav move on to other PCI drivers that use legacy PM.  If we
> convert all the others away from legacy PM and gpio-ml-ioh.c is the
> only one remaining, then I guess we can revisit this :)

Then skip this driver for good.

> Or, maybe converting gpio-ml-ioh.c now, along the lines of
> 226e6b866d74 ("gpio: pch: Convert to dev_pm_ops"), would be one small
> step towards the eventual unification, by making gpio-pch and
> gpio-ml-ioh a little more similar.

I think it will delay the real work here (very old code motivates
better to get rid of it then semi-fixed one).
Thank you for your understanding.

-- 
With Best Regards,
Andy Shevchenko

Re: [Linux-kernel-mentees] [PATCH v1] gpio: ml: ioh: Convert to dev_pm_ops

[Bjorn Helgaas]

[+cc linux-pci]

On Thu, Apr 02, 2020 at 11:23:27PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 2, 2020 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Apr 02, 2020 at 09:33:46PM +0300, Andy Shevchenko wrote:
> > > On Thu, Apr 2, 2020 at 6:52 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote:
> > > >
> > > > Convert the legacy callback .suspend() and .resume()
> > > > to the generic ones.
> > >
> > > Thank you for the patch.
> > >
> > > Rather then doing this I think the best approach is to unify gpio-pch
> > > and gpio-ml-ioh together.
> > > Under umbrella of the task, the clean ups like above are highly
> > > appreciated.
> >
> > I'd be all in favor of that, but what Vaibhav is working toward is
> > eliminating use of legacy PM in PCI drivers.  I think unifying drivers
> > is really out of scope for that project.
> >
> > If you'd rather leave gpio-ml-ioh.c alone for now, I suggest that
> > Vaibhav move on to other PCI drivers that use legacy PM.  If we
> > convert all the others away from legacy PM and gpio-ml-ioh.c is the
> > only one remaining, then I guess we can revisit this :)
> 
> Then skip this driver for good.
> 
> > Or, maybe converting gpio-ml-ioh.c now, along the lines of
> > 226e6b866d74 ("gpio: pch: Convert to dev_pm_ops"), would be one small
> > step towards the eventual unification, by making gpio-pch and
> > gpio-ml-ioh a little more similar.
> 
> I think it will delay the real work here (very old code motivates
> better to get rid of it then semi-fixed one).

With respect, I think it is unreasonable to use the fact that
gpio-ml-ioh and gpio-pch should be unified to hold up the conversion
of gpio-ml-ioh to generic power management.

I do not want to skip gpio-ml-ioh for good, because it is one of the
few remaining drivers that use the legacy PCI PM interfaces.  We are
very close to being able to remove a significant amount of ugly code
from the PCI core.

gpio-ml-ioh and gpio-pch do look quite similar, and no doubt it would
be great to unify them.  But without datasheets or hardware to test,
that's not a trivial task, and I don't think that burden should fall
on anyone who wants to make any improvements to these drivers.

Another alternative would be to remove legacy PCI PM usage
(ioh_gpio_suspend() and ioh_gpio_resume()) from gpio-ml-ioh.  That
would mean gpio-ml-ioh wouldn't support power management at all, which
isn't a good thing, but maybe it would be even more motivation to
unify it with gpio-pch (which has already been converted by
226e6b866d74 ("gpio: pch: Convert to dev_pm_ops"))?

Bjorn

Re: [Linux-kernel-mentees] [PATCH v1] gpio: ml: ioh: Convert to dev_pm_ops

[Andy Shevchenko]

On Thu, Jul 08, 2021 at 04:47:06PM -0500, Bjorn Helgaas wrote:
> On Thu, Apr 02, 2020 at 11:23:27PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 2, 2020 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Thu, Apr 02, 2020 at 09:33:46PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Apr 2, 2020 at 6:52 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote:
> > > > >
> > > > > Convert the legacy callback .suspend() and .resume()
> > > > > to the generic ones.
> > > >
> > > > Thank you for the patch.
> > > >
> > > > Rather then doing this I think the best approach is to unify gpio-pch
> > > > and gpio-ml-ioh together.
> > > > Under umbrella of the task, the clean ups like above are highly
> > > > appreciated.
> > >
> > > I'd be all in favor of that, but what Vaibhav is working toward is
> > > eliminating use of legacy PM in PCI drivers.  I think unifying drivers
> > > is really out of scope for that project.
> > >
> > > If you'd rather leave gpio-ml-ioh.c alone for now, I suggest that
> > > Vaibhav move on to other PCI drivers that use legacy PM.  If we
> > > convert all the others away from legacy PM and gpio-ml-ioh.c is the
> > > only one remaining, then I guess we can revisit this :)
> > 
> > Then skip this driver for good.
> > 
> > > Or, maybe converting gpio-ml-ioh.c now, along the lines of
> > > 226e6b866d74 ("gpio: pch: Convert to dev_pm_ops"), would be one small
> > > step towards the eventual unification, by making gpio-pch and
> > > gpio-ml-ioh a little more similar.
> > 
> > I think it will delay the real work here (very old code motivates
> > better to get rid of it then semi-fixed one).
> 
> With respect, I think it is unreasonable to use the fact that
> gpio-ml-ioh and gpio-pch should be unified to hold up the conversion
> of gpio-ml-ioh to generic power management.
> 
> I do not want to skip gpio-ml-ioh for good, because it is one of the
> few remaining drivers that use the legacy PCI PM interfaces.  We are
> very close to being able to remove a significant amount of ugly code
> from the PCI core.

Makes sense (1).

> gpio-ml-ioh and gpio-pch do look quite similar, and no doubt it would
> be great to unify them.  But without datasheets or hardware to test,

Datasheets are publicly available (at least one may google and find some
information about those PCH chips). I have in possession the hardware for
gpio-pch. I can easily test that part at least.

> that's not a trivial task, and I don't think that burden should fall
> on anyone who wants to make any improvements to these drivers.

> Another alternative would be to remove legacy PCI PM usage
> (ioh_gpio_suspend() and ioh_gpio_resume()) from gpio-ml-ioh.  That
> would mean gpio-ml-ioh wouldn't support power management at all, which
> isn't a good thing, but maybe it would be even more motivation to
> unify it with gpio-pch (which has already been converted by
> 226e6b866d74 ("gpio: pch: Convert to dev_pm_ops"))?

With regard to (1) probably we may exceptionally accept the fix to gpio-ml-ioh,
but I really prefer to do the much more _useful_ job on it by unifying the two.

-- 
With Best Regards,
Andy Shevchenko

Re: [Linux-kernel-mentees] [PATCH v1] gpio: ml: ioh: Convert to dev_pm_ops

[Bjorn Helgaas]

On Mon, Jul 12, 2021 at 02:48:12PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 08, 2021 at 04:47:06PM -0500, Bjorn Helgaas wrote:
> > On Thu, Apr 02, 2020 at 11:23:27PM +0300, Andy Shevchenko wrote:
> > > On Thu, Apr 2, 2020 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Thu, Apr 02, 2020 at 09:33:46PM +0300, Andy Shevchenko wrote:
> > > > > On Thu, Apr 2, 2020 at 6:52 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote:
> > > > > >
> > > > > > Convert the legacy callback .suspend() and .resume()
> > > > > > to the generic ones.
> > > > >
> > > > > Thank you for the patch.
> > > > >
> > > > > Rather then doing this I think the best approach is to unify gpio-pch
> > > > > and gpio-ml-ioh together.
> > > > > Under umbrella of the task, the clean ups like above are highly
> > > > > appreciated.
> > > >
> > > > I'd be all in favor of that, but what Vaibhav is working toward is
> > > > eliminating use of legacy PM in PCI drivers.  I think unifying drivers
> > > > is really out of scope for that project.
> > > >
> > > > If you'd rather leave gpio-ml-ioh.c alone for now, I suggest that
> > > > Vaibhav move on to other PCI drivers that use legacy PM.  If we
> > > > convert all the others away from legacy PM and gpio-ml-ioh.c is the
> > > > only one remaining, then I guess we can revisit this :)
> > > 
> > > Then skip this driver for good.
> > > 
> > > > Or, maybe converting gpio-ml-ioh.c now, along the lines of
> > > > 226e6b866d74 ("gpio: pch: Convert to dev_pm_ops"), would be one small
> > > > step towards the eventual unification, by making gpio-pch and
> > > > gpio-ml-ioh a little more similar.
> > > 
> > > I think it will delay the real work here (very old code motivates
> > > better to get rid of it then semi-fixed one).
> > 
> > With respect, I think it is unreasonable to use the fact that
> > gpio-ml-ioh and gpio-pch should be unified to hold up the conversion
> > of gpio-ml-ioh to generic power management.
> > 
> > I do not want to skip gpio-ml-ioh for good, because it is one of the
> > few remaining drivers that use the legacy PCI PM interfaces.  We are
> > very close to being able to remove a significant amount of ugly code
> > from the PCI core.
> 
> Makes sense (1).
> 
> > gpio-ml-ioh and gpio-pch do look quite similar, and no doubt it would
> > be great to unify them.  But without datasheets or hardware to test,
> 
> Datasheets are publicly available (at least one may google and find some
> information about those PCH chips). I have in possession the hardware for
> gpio-pch. I can easily test that part at least.

If you have a URL for those datasheets, can you share it?  I spent
some time looking but all I found was 1-2 page marketing brochures.

> > that's not a trivial task, and I don't think that burden should fall
> > on anyone who wants to make any improvements to these drivers.
> 
> > Another alternative would be to remove legacy PCI PM usage
> > (ioh_gpio_suspend() and ioh_gpio_resume()) from gpio-ml-ioh.  That
> > would mean gpio-ml-ioh wouldn't support power management at all, which
> > isn't a good thing, but maybe it would be even more motivation to
> > unify it with gpio-pch (which has already been converted by
> > 226e6b866d74 ("gpio: pch: Convert to dev_pm_ops"))?
> 
> With regard to (1) probably we may exceptionally accept the fix to
> gpio-ml-ioh, but I really prefer to do the much more _useful_ job on
> it by unifying the two.

Should Vaibhav re-post this patch, or do you want to pull it from the
archives?  I just checked and it still applies cleanly to v5.14-rc1.

Here it is for reference:
  https://lore.kernel.org/lkml/20200402155057.30667-1-vaibhavgupta40@gmail.com/

I'll post a couple small patches toward unifying them.  They don't do
the whole job but they're baby steps.

Bjorn

Re: [Linux-kernel-mentees] [PATCH v1] gpio: ml: ioh: Convert to dev_pm_ops

[Andy Shevchenko]

On Tue, Jul 13, 2021 at 1:36 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Jul 12, 2021 at 02:48:12PM +0300, Andy Shevchenko wrote:
> > On Thu, Jul 08, 2021 at 04:47:06PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Apr 02, 2020 at 11:23:27PM +0300, Andy Shevchenko wrote:

...

> > Datasheets are publicly available (at least one may google and find some
> > information about those PCH chips). I have in possession the hardware for
> > gpio-pch. I can easily test that part at least.
>
> If you have a URL for those datasheets, can you share it?  I spent
> some time looking but all I found was 1-2 page marketing brochures.

It's a part of the so called EG20T PCH. It's part of in particular
Intel Galileo (Quark SoC) and Intel Minnowboard (v1) (Atom E6xx SoC).
Hence the easily found links:

http://minnowboard.outof.biz/MinnowBoard.html
https://www.elinux.org/Minnowboard:Minnow_Original
https://en.wikipedia.org/wiki/List_of_Intel_Atom_microprocessors

https://www.intel.com/content/www/us/en/embedded/products/quark/x1000/documentation.html?grouping=EMT_Content%20Type&sort=title:asc
(Chapter 19)

https://ark.intel.com/content/www/us/en/ark/products/codename/37567/products-formerly-tunnel-creek.html

Hmm... Funny, the document #324211 can't be downloaded
https://download.intel.com/embedded/chipsets/datasheet/324211.pdf

I guess you may ping Intel and tell them that they should play nice
when talking about "open hardware" (MinnowBoard initiative).
Nevertheless, the (Old? #457798 is a specification update under NDA.
Okay, it refers to rev 8, while Mouser, see below, provides rev 9)
copy is available on other sites, such as

https://www.mouser.tw/pdfdocs/Intel_Platform_Controller_Hub_EG20T_datasheet.pdf
(Chapter 16)

> > > that's not a trivial task, and I don't think that burden should fall
> > > on anyone who wants to make any improvements to these drivers.
> >
> > > Another alternative would be to remove legacy PCI PM usage
> > > (ioh_gpio_suspend() and ioh_gpio_resume()) from gpio-ml-ioh.  That
> > > would mean gpio-ml-ioh wouldn't support power management at all, which
> > > isn't a good thing, but maybe it would be even more motivation to
> > > unify it with gpio-pch (which has already been converted by
> > > 226e6b866d74 ("gpio: pch: Convert to dev_pm_ops"))?
> >
> > With regard to (1) probably we may exceptionally accept the fix to
> > gpio-ml-ioh, but I really prefer to do the much more _useful_ job on
> > it by unifying the two.
>
> Should Vaibhav re-post this patch, or do you want to pull it from the
> archives?  I just checked and it still applies cleanly to v5.14-rc1.
>
> Here it is for reference:
>   https://lore.kernel.org/lkml/20200402155057.30667-1-vaibhavgupta40@gmail.com/

I'll take from the archives.

> I'll post a couple small patches toward unifying them.  They don't do
> the whole job but they're baby steps.

Thanks! I look forward to seeing them soon!

-- 
With Best Regards,
Andy Shevchenko

Re: [Linux-kernel-mentees] [PATCH v1] gpio: ml: ioh: Convert to dev_pm_ops

[Andy Shevchenko]

On Thu, Apr 02, 2020 at 09:20:58PM +0530, Vaibhav Gupta wrote:
> Convert the legacy callback .suspend() and .resume()
> to the generic ones.

Pushed to my review and testing queue with some amendments, thanks!

-- 
With Best Regards,
Andy Shevchenko