From: Piyush Mehta <[email protected]>
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 <[email protected]>
Signed-off-by: Radhey Shyam Pandey <[email protected]>
---
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;
+
/* 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
On 1/19/24 04:08, Radhey Shyam Pandey wrote:
> From: Piyush Mehta <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Radhey Shyam Pandey <[email protected]>
Looks OK to me.
Reviewed-by: Damien Le Moal <[email protected]>
--
Damien Le Moal
Western Digital Research
Hello Radhey,
On Fri, Jan 19, 2024 at 12:38:24AM +0530, Radhey Shyam Pandey wrote:
> From: Piyush Mehta <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Radhey Shyam Pandey <[email protected]>
> ---
> 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
>