linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ata: libahci_platform: Fix regulator_get_optional() misuse
@ 2019-09-02 17:22 Mark Brown
  0 siblings, 0 replies; 4+ messages in thread
From: Mark Brown @ 2019-09-02 17:22 UTC (permalink / raw)
  To: Hans de Goede, Jens Axboe; +Cc: linux-ide, linux-kernel, Mark Brown

This driver is using regulator_get_optional() to handle all the supplies
that it handles, and only ever enables and disables all supplies en masse
without ever doing any other configuration of the device to handle missing
power. These are clear signs that the API is being misused - it should only
be used for supplies that may be physically absent from the system and in
these cases the hardware usually needs different configuration if the
supply is missing. Instead use normal regualtor_get(), if the supply is
not described in DT then the framework will substitute a dummy regulator in
so no special handling is needed by the consumer driver.

In the case of the PHY regulator the handling in the driver is a hack to
deal with integrated PHYs; the supplies are only optional in the sense
that that there's some confusion in the code about where they're bound to.
From a code point of view they function exactly as normal supplies so can
be treated as such. It'd probably be better to model this by instantiating
a PHY object for integrated PHYs.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/ata/libahci_platform.c | 38 +++++++++++++---------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 9e9583a6bba9..57882b3e46eb 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -153,17 +153,13 @@ int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
 {
 	int rc, i;
 
-	if (hpriv->ahci_regulator) {
-		rc = regulator_enable(hpriv->ahci_regulator);
-		if (rc)
-			return rc;
-	}
+	rc = regulator_enable(hpriv->ahci_regulator);
+	if (rc)
+		return rc;
 
-	if (hpriv->phy_regulator) {
-		rc = regulator_enable(hpriv->phy_regulator);
-		if (rc)
-			goto disable_ahci_pwrs;
-	}
+	rc = regulator_enable(hpriv->phy_regulator);
+	if (rc)
+		goto disable_ahci_pwrs;
 
 	for (i = 0; i < hpriv->nports; i++) {
 		if (!hpriv->target_pwrs[i])
@@ -181,11 +177,9 @@ int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
 		if (hpriv->target_pwrs[i])
 			regulator_disable(hpriv->target_pwrs[i]);
 
-	if (hpriv->phy_regulator)
-		regulator_disable(hpriv->phy_regulator);
+	regulator_disable(hpriv->phy_regulator);
 disable_ahci_pwrs:
-	if (hpriv->ahci_regulator)
-		regulator_disable(hpriv->ahci_regulator);
+	regulator_disable(hpriv->ahci_regulator);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(ahci_platform_enable_regulators);
@@ -207,10 +201,8 @@ void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv)
 		regulator_disable(hpriv->target_pwrs[i]);
 	}
 
-	if (hpriv->ahci_regulator)
-		regulator_disable(hpriv->ahci_regulator);
-	if (hpriv->phy_regulator)
-		regulator_disable(hpriv->phy_regulator);
+	regulator_disable(hpriv->ahci_regulator);
+	regulator_disable(hpriv->phy_regulator);
 }
 EXPORT_SYMBOL_GPL(ahci_platform_disable_regulators);
 /**
@@ -359,7 +351,7 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
 	struct regulator *target_pwr;
 	int rc = 0;
 
-	target_pwr = regulator_get_optional(dev, "target");
+	target_pwr = regulator_get(dev, "target");
 
 	if (!IS_ERR(target_pwr))
 		hpriv->target_pwrs[port] = target_pwr;
@@ -436,16 +428,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		hpriv->clks[i] = clk;
 	}
 
-	hpriv->ahci_regulator = devm_regulator_get_optional(dev, "ahci");
+	hpriv->ahci_regulator = devm_regulator_get(dev, "ahci");
 	if (IS_ERR(hpriv->ahci_regulator)) {
 		rc = PTR_ERR(hpriv->ahci_regulator);
-		if (rc == -EPROBE_DEFER)
+		if (rc != 0)
 			goto err_out;
-		rc = 0;
-		hpriv->ahci_regulator = NULL;
 	}
 
-	hpriv->phy_regulator = devm_regulator_get_optional(dev, "phy");
+	hpriv->phy_regulator = devm_regulator_get(dev, "phy");
 	if (IS_ERR(hpriv->phy_regulator)) {
 		rc = PTR_ERR(hpriv->phy_regulator);
 		if (rc == -EPROBE_DEFER)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] ata: libahci_platform: Fix regulator_get_optional() misuse
  2019-10-16 10:51 Mark Brown
  2019-10-17 12:07 ` Hans de Goede
@ 2019-10-25 20:22 ` Jens Axboe
  1 sibling, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2019-10-25 20:22 UTC (permalink / raw)
  To: Mark Brown, Hans de Goede; +Cc: linux-ide

On 10/16/19 4:51 AM, Mark Brown wrote:
> This driver is using regulator_get_optional() to handle all the supplies
> that it handles, and only ever enables and disables all supplies en masse
> without ever doing any other configuration of the device to handle missing
> power. These are clear signs that the API is being misused - it should only
> be used for supplies that may be physically absent from the system and in
> these cases the hardware usually needs different configuration if the
> supply is missing. Instead use normal regualtor_get(), if the supply is
> not described in DT then the framework will substitute a dummy regulator in
> so no special handling is needed by the consumer driver.
> 
> In the case of the PHY regulator the handling in the driver is a hack to
> deal with integrated PHYs; the supplies are only optional in the sense
> that that there's some confusion in the code about where they're bound to.
>  From a code point of view they function exactly as normal supplies so can
> be treated as such. It'd probably be better to model this by instantiating
> a PHY object for integrated PHYs.

Applied, thanks.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] ata: libahci_platform: Fix regulator_get_optional() misuse
  2019-10-16 10:51 Mark Brown
@ 2019-10-17 12:07 ` Hans de Goede
  2019-10-25 20:22 ` Jens Axboe
  1 sibling, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2019-10-17 12:07 UTC (permalink / raw)
  To: Mark Brown, Jens Axboe; +Cc: linux-ide

Hi,

On 16-10-2019 12:51, Mark Brown wrote:
> This driver is using regulator_get_optional() to handle all the supplies
> that it handles, and only ever enables and disables all supplies en masse
> without ever doing any other configuration of the device to handle missing
> power. These are clear signs that the API is being misused - it should only
> be used for supplies that may be physically absent from the system and in
> these cases the hardware usually needs different configuration if the
> supply is missing. Instead use normal regualtor_get(), if the supply is
> not described in DT then the framework will substitute a dummy regulator in
> so no special handling is needed by the consumer driver.
> 
> In the case of the PHY regulator the handling in the driver is a hack to
> deal with integrated PHYs; the supplies are only optional in the sense
> that that there's some confusion in the code about where they're bound to.
>  From a code point of view they function exactly as normal supplies so can
> be treated as such. It'd probably be better to model this by instantiating
> a PHY object for integrated PHYs.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>   drivers/ata/libahci_platform.c | 38 +++++++++++++---------------------
>   1 file changed, 14 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index e742780950de..8befce036af8 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -153,17 +153,13 @@ int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
>   {
>   	int rc, i;
>   
> -	if (hpriv->ahci_regulator) {
> -		rc = regulator_enable(hpriv->ahci_regulator);
> -		if (rc)
> -			return rc;
> -	}
> +	rc = regulator_enable(hpriv->ahci_regulator);
> +	if (rc)
> +		return rc;
>   
> -	if (hpriv->phy_regulator) {
> -		rc = regulator_enable(hpriv->phy_regulator);
> -		if (rc)
> -			goto disable_ahci_pwrs;
> -	}
> +	rc = regulator_enable(hpriv->phy_regulator);
> +	if (rc)
> +		goto disable_ahci_pwrs;
>   
>   	for (i = 0; i < hpriv->nports; i++) {
>   		if (!hpriv->target_pwrs[i])
> @@ -181,11 +177,9 @@ int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
>   		if (hpriv->target_pwrs[i])
>   			regulator_disable(hpriv->target_pwrs[i]);
>   
> -	if (hpriv->phy_regulator)
> -		regulator_disable(hpriv->phy_regulator);
> +	regulator_disable(hpriv->phy_regulator);
>   disable_ahci_pwrs:
> -	if (hpriv->ahci_regulator)
> -		regulator_disable(hpriv->ahci_regulator);
> +	regulator_disable(hpriv->ahci_regulator);
>   	return rc;
>   }
>   EXPORT_SYMBOL_GPL(ahci_platform_enable_regulators);
> @@ -207,10 +201,8 @@ void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv)
>   		regulator_disable(hpriv->target_pwrs[i]);
>   	}
>   
> -	if (hpriv->ahci_regulator)
> -		regulator_disable(hpriv->ahci_regulator);
> -	if (hpriv->phy_regulator)
> -		regulator_disable(hpriv->phy_regulator);
> +	regulator_disable(hpriv->ahci_regulator);
> +	regulator_disable(hpriv->phy_regulator);
>   }
>   EXPORT_SYMBOL_GPL(ahci_platform_disable_regulators);
>   /**
> @@ -359,7 +351,7 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
>   	struct regulator *target_pwr;
>   	int rc = 0;
>   
> -	target_pwr = regulator_get_optional(dev, "target");
> +	target_pwr = regulator_get(dev, "target");
>   
>   	if (!IS_ERR(target_pwr))
>   		hpriv->target_pwrs[port] = target_pwr;
> @@ -436,16 +428,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>   		hpriv->clks[i] = clk;
>   	}
>   
> -	hpriv->ahci_regulator = devm_regulator_get_optional(dev, "ahci");
> +	hpriv->ahci_regulator = devm_regulator_get(dev, "ahci");
>   	if (IS_ERR(hpriv->ahci_regulator)) {
>   		rc = PTR_ERR(hpriv->ahci_regulator);
> -		if (rc == -EPROBE_DEFER)
> +		if (rc != 0)
>   			goto err_out;
> -		rc = 0;
> -		hpriv->ahci_regulator = NULL;
>   	}
>   
> -	hpriv->phy_regulator = devm_regulator_get_optional(dev, "phy");
> +	hpriv->phy_regulator = devm_regulator_get(dev, "phy");
>   	if (IS_ERR(hpriv->phy_regulator)) {
>   		rc = PTR_ERR(hpriv->phy_regulator);
>   		if (rc == -EPROBE_DEFER)
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread
* [PATCH] ata: libahci_platform: Fix regulator_get_optional() misuse
@ 2019-10-16 10:51 Mark Brown
  2019-10-17 12:07 ` Hans de Goede
  2019-10-25 20:22 ` Jens Axboe
  0 siblings, 2 replies; 4+ messages in thread
From: Mark Brown @ 2019-10-16 10:51 UTC (permalink / raw)
  To: Hans de Goede, Jens Axboe; +Cc: linux-ide, Mark Brown

This driver is using regulator_get_optional() to handle all the supplies
that it handles, and only ever enables and disables all supplies en masse
without ever doing any other configuration of the device to handle missing
power. These are clear signs that the API is being misused - it should only
be used for supplies that may be physically absent from the system and in
these cases the hardware usually needs different configuration if the
supply is missing. Instead use normal regualtor_get(), if the supply is
not described in DT then the framework will substitute a dummy regulator in
so no special handling is needed by the consumer driver.

In the case of the PHY regulator the handling in the driver is a hack to
deal with integrated PHYs; the supplies are only optional in the sense
that that there's some confusion in the code about where they're bound to.
From a code point of view they function exactly as normal supplies so can
be treated as such. It'd probably be better to model this by instantiating
a PHY object for integrated PHYs.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/ata/libahci_platform.c | 38 +++++++++++++---------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index e742780950de..8befce036af8 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -153,17 +153,13 @@ int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
 {
 	int rc, i;
 
-	if (hpriv->ahci_regulator) {
-		rc = regulator_enable(hpriv->ahci_regulator);
-		if (rc)
-			return rc;
-	}
+	rc = regulator_enable(hpriv->ahci_regulator);
+	if (rc)
+		return rc;
 
-	if (hpriv->phy_regulator) {
-		rc = regulator_enable(hpriv->phy_regulator);
-		if (rc)
-			goto disable_ahci_pwrs;
-	}
+	rc = regulator_enable(hpriv->phy_regulator);
+	if (rc)
+		goto disable_ahci_pwrs;
 
 	for (i = 0; i < hpriv->nports; i++) {
 		if (!hpriv->target_pwrs[i])
@@ -181,11 +177,9 @@ int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
 		if (hpriv->target_pwrs[i])
 			regulator_disable(hpriv->target_pwrs[i]);
 
-	if (hpriv->phy_regulator)
-		regulator_disable(hpriv->phy_regulator);
+	regulator_disable(hpriv->phy_regulator);
 disable_ahci_pwrs:
-	if (hpriv->ahci_regulator)
-		regulator_disable(hpriv->ahci_regulator);
+	regulator_disable(hpriv->ahci_regulator);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(ahci_platform_enable_regulators);
@@ -207,10 +201,8 @@ void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv)
 		regulator_disable(hpriv->target_pwrs[i]);
 	}
 
-	if (hpriv->ahci_regulator)
-		regulator_disable(hpriv->ahci_regulator);
-	if (hpriv->phy_regulator)
-		regulator_disable(hpriv->phy_regulator);
+	regulator_disable(hpriv->ahci_regulator);
+	regulator_disable(hpriv->phy_regulator);
 }
 EXPORT_SYMBOL_GPL(ahci_platform_disable_regulators);
 /**
@@ -359,7 +351,7 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
 	struct regulator *target_pwr;
 	int rc = 0;
 
-	target_pwr = regulator_get_optional(dev, "target");
+	target_pwr = regulator_get(dev, "target");
 
 	if (!IS_ERR(target_pwr))
 		hpriv->target_pwrs[port] = target_pwr;
@@ -436,16 +428,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		hpriv->clks[i] = clk;
 	}
 
-	hpriv->ahci_regulator = devm_regulator_get_optional(dev, "ahci");
+	hpriv->ahci_regulator = devm_regulator_get(dev, "ahci");
 	if (IS_ERR(hpriv->ahci_regulator)) {
 		rc = PTR_ERR(hpriv->ahci_regulator);
-		if (rc == -EPROBE_DEFER)
+		if (rc != 0)
 			goto err_out;
-		rc = 0;
-		hpriv->ahci_regulator = NULL;
 	}
 
-	hpriv->phy_regulator = devm_regulator_get_optional(dev, "phy");
+	hpriv->phy_regulator = devm_regulator_get(dev, "phy");
 	if (IS_ERR(hpriv->phy_regulator)) {
 		rc = PTR_ERR(hpriv->phy_regulator);
 		if (rc == -EPROBE_DEFER)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-25 20:22 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-02 17:22 [PATCH] ata: libahci_platform: Fix regulator_get_optional() misuse Mark Brown
2019-10-16 10:51 Mark Brown
2019-10-17 12:07 ` Hans de Goede
2019-10-25 20:22 ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).