| Message ID | 1705604904-471889-3-git-send-email-radhey.shyam.pandey@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | ata: ahci_ceva: fix xilinx GT PHY support | expand |
On 1/19/24 04:08, Radhey Shyam Pandey wrote: > From: Piyush Mehta <piyush.mehta@amd.com> > > The regulators API are disabled and enabled, during suspend and resume, > respectively. The following warning notice shows up on the initial suspend > because the enable regulators API is unaddressed in the probe: > > regulator-dummy: Underflow of regulator enable count > > Added the ahci_platform_enable_regulators API in probe to maintain the > regulator enabled and disabled ref count. > > Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support xilinx GT phy") > Signed-off-by: Piyush Mehta <piyush.mehta@amd.com> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com> Looks OK to me. Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Hello Radhey, On Fri, Jan 19, 2024 at 12:38:24AM +0530, Radhey Shyam Pandey wrote: > From: Piyush Mehta <piyush.mehta@amd.com> > > The regulators API are disabled and enabled, during suspend and resume, > respectively. The following warning notice shows up on the initial suspend > because the enable regulators API is unaddressed in the probe: Please be a bit more specific in your commit message. e.g. during system suspend, ahci_platform_suspend() calls ahci_platform_disable_resources() which calls ahci_platform_disable_regulators() which calls regulator_disable() for all regulators found in the controller. > > regulator-dummy: Underflow of regulator enable count > > Added the ahci_platform_enable_regulators API in probe to maintain the > regulator enabled and disabled ref count. s/Added/Add/ "Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour." see: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes > > Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support xilinx GT phy") > Signed-off-by: Piyush Mehta <piyush.mehta@amd.com> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com> > --- > drivers/ata/ahci_ceva.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c > index bfc513f1d0b3..1c56f0cabb11 100644 > --- a/drivers/ata/ahci_ceva.c > +++ b/drivers/ata/ahci_ceva.c > @@ -219,9 +219,14 @@ static int ceva_ahci_probe(struct platform_device *pdev) > if (rc) > return rc; > } else { > - rc = ahci_platform_enable_clks(hpriv); > + rc = ahci_platform_enable_regulators(hpriv); > if (rc) > return rc; > + > + rc = ahci_platform_enable_clks(hpriv); > + if (rc) > + goto disable_regulator; > + Like I wrote in patch 1/2, I would prefer if you could somehow get ahci_platform_enable_resources() to work for your platform, so that you don't need to copy paste all of ahci_platform_enable_resources() to your driver. If it does not work to simply add a reset_control_assert() + usleep(), considering that this function is essentially a copy paste of ahci_platform_enable_resources(), I would still prefer the addition of a new flag, and keep the extra logic needed in libahci_platform.c, so that the code is kept in the same place, rather than to copy paste the whole function to your driver. Kind regards, Niklas > /* Assert the controller reset */ > reset_control_assert(cevapriv->rst); > > @@ -340,6 +345,9 @@ static int ceva_ahci_probe(struct platform_device *pdev) > disable_clks: > ahci_platform_disable_clks(hpriv); > > +disable_regulator: > + ahci_platform_disable_regulators(hpriv); > + > return rc; > } > > -- > 2.34.1 >
diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c index bfc513f1d0b3..1c56f0cabb11 100644 --- a/drivers/ata/ahci_ceva.c +++ b/drivers/ata/ahci_ceva.c @@ -219,9 +219,14 @@ static int ceva_ahci_probe(struct platform_device *pdev) if (rc) return rc; } else { - rc = ahci_platform_enable_clks(hpriv); + rc = ahci_platform_enable_regulators(hpriv); if (rc) return rc; + + rc = ahci_platform_enable_clks(hpriv); + if (rc) + goto disable_regulator; + /* Assert the controller reset */ reset_control_assert(cevapriv->rst); @@ -340,6 +345,9 @@ static int ceva_ahci_probe(struct platform_device *pdev) disable_clks: ahci_platform_disable_clks(hpriv); +disable_regulator: + ahci_platform_disable_regulators(hpriv); + return rc; }