Re: [PATCH] ata: libahci_platform: Fix regulator_get_optional() misuse

[Hans de Goede <hdegoede@redhat.com>]

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)
>