diff mbox series

[2/2] ata: ahci_ceva: add missing enable regulator API for Xilinx GT PHY support

Message ID 1705604904-471889-3-git-send-email-radhey.shyam.pandey@amd.com
State New
Headers
Series ata: ahci_ceva: fix xilinx GT PHY support |

Commit Message

Pandey, Radhey Shyam Jan. 18, 2024, 7:08 p.m. UTC 
  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>
---
 drivers/ata/ahci_ceva.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Damien Le Moal Jan. 22, 2024, 10:29 a.m. UTC | #1 
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>
Niklas Cassel Jan. 22, 2024, 1:28 p.m. UTC | #2 
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 mbox series

Patch

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;
 }