Re: [PATCH 3/3] ata: libahci: Allow to use multiple regulators

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

Hi Gregory,

Thanks for working on this. Overall the patch-set / concept looks good,
you can add "Acked-by: Hans de Goede <hdegoede@redhat.com>" to the first
2 patches.

I've some comments on this patch, see below.

On 27-12-14 11:34, Gregory CLEMENT wrote:
> The current implementation of the libahci allows using multiple PHYs
> but not multiple regulators. This patch adds the support of multiple
> regulators. Until now it was mandatory to have a PHY under a subnode,
> now a port subnode can contain either a regulator or a PHY (or both).
>
> There was only one driver which used directly the regulator field of
> the ahci_host_priv structure. To preserve the bisectability the change
> in the ahci_imx driver was done in the same patch.
>
> This patch also depend of a patch of the regulator framework:
> "regulator: core: Add the device tree version to the regulator_get
> family"
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>   drivers/ata/ahci.h             |   2 +-
>   drivers/ata/ahci_imx.c         |  14 +--
>   drivers/ata/libahci_platform.c | 206 ++++++++++++++++++++++++++++-------------
>   include/linux/ahci_platform.h  |   2 +
>   4 files changed, 151 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 40f0e34f17af..275358ae0b3f 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -333,7 +333,7 @@ struct ahci_host_priv {
>   	u32			em_msg_type;	/* EM message type */
>   	bool			got_runtime_pm; /* Did we do pm_runtime_get? */
>   	struct clk		*clks[AHCI_MAX_CLKS]; /* Optional */
> -	struct regulator	*target_pwr;	/* Optional */
> +	struct regulator	**target_pwrs;	/* Optional */
>   	/*
>   	 * If platform uses PHYs. There is a 1:1 relation between the port number and
>   	 * the PHY position in this array.
> diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
> index 35d51c59a370..41632e57d46f 100644
> --- a/drivers/ata/ahci_imx.c
> +++ b/drivers/ata/ahci_imx.c
> @@ -221,11 +221,9 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
>   	if (imxpriv->no_device)
>   		return 0;
>
> -	if (hpriv->target_pwr) {
> -		ret = regulator_enable(hpriv->target_pwr);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = ahci_platform_enable_regulators(hpriv);
> +	if (ret)
> +		return ret;
>
>   	ret = clk_prepare_enable(imxpriv->sata_ref_clk);
>   	if (ret < 0)
> @@ -270,8 +268,7 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
>   disable_clk:
>   	clk_disable_unprepare(imxpriv->sata_ref_clk);
>   disable_regulator:
> -	if (hpriv->target_pwr)
> -		regulator_disable(hpriv->target_pwr);
> +	ahci_platform_disable_regulators(hpriv);
>
>   	return ret;
>   }
> @@ -291,8 +288,7 @@ static void imx_sata_disable(struct ahci_host_priv *hpriv)
>
>   	clk_disable_unprepare(imxpriv->sata_ref_clk);
>
> -	if (hpriv->target_pwr)
> -		regulator_disable(hpriv->target_pwr);
> +	ahci_platform_disable_regulators(hpriv);
>   }
>
>   static void ahci_imx_error_handler(struct ata_port *ap)
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index a147aaadca85..f3bf4311152d 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -138,6 +138,59 @@ void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)
>   EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
>
>   /**
> + * ahci_platform_enable_regulators - Enable regulators
> + * @hpriv: host private area to store config values
> + *
> + * This function enables all the regulators found in
> + * hpriv->target_pwrs, if any.  If a regulator fails to be enabled, it
> + * disables all the regulators already enabled in reverse order and
> + * returns an error.
> + *
> + * RETURNS:
> + * 0 on success otherwise a negative error code
> + */
> +int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
> +{
> +	int rc, i;
> +
> +	for (i = 0; i < hpriv->nports; i++) {
> +		if (!hpriv->target_pwrs[i])
> +			continue;
> +
> +		rc = regulator_enable(hpriv->target_pwrs[i]);
> +		if (rc)
> +			goto disable_target_pwrs;
> +	}
> +
> +	return 0;
> +
> +disable_target_pwrs:
> +	while (--i >= 0)
> +		if (!hpriv->target_pwrs[i])

I'm pretty sure you want:

		if (hpriv->target_pwrs[i])

here, so without the '!' .

> +			regulator_disable(hpriv->target_pwrs[i]);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_enable_regulators);
> +
> +/**
> + * ahci_platform_disable_regulators - Disable regulators
> + * @hpriv: host private area to store config values
> + *
> + * This function disables all regulators found in hpriv->target_pwrs.
> + */
> +void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv)
> +{
> +	int i;
> +
> +	for (i = 0; i < hpriv->nports; i++) {
> +		if (!hpriv->target_pwrs[i])
> +			continue;
> +		regulator_disable(hpriv->target_pwrs[i]);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_disable_regulators);
> +/**
>    * ahci_platform_enable_resources - Enable platform resources
>    * @hpriv: host private area to store config values
>    *
> @@ -157,11 +210,9 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
>   {
>   	int rc;
>
> -	if (hpriv->target_pwr) {
> -		rc = regulator_enable(hpriv->target_pwr);
> -		if (rc)
> -			return rc;
> -	}
> +	rc = ahci_platform_enable_regulators(hpriv);
> +	if (rc)
> +		return rc;
>
>   	rc = ahci_platform_enable_clks(hpriv);
>   	if (rc)
> @@ -177,8 +228,8 @@ disable_clks:
>   	ahci_platform_disable_clks(hpriv);
>
>   disable_regulator:
> -	if (hpriv->target_pwr)
> -		regulator_disable(hpriv->target_pwr);
> +	ahci_platform_disable_regulators(hpriv);
> +
>   	return rc;
>   }
>   EXPORT_SYMBOL_GPL(ahci_platform_enable_resources);
> @@ -199,8 +250,7 @@ void ahci_platform_disable_resources(struct ahci_host_priv *hpriv)
>
>   	ahci_platform_disable_clks(hpriv);
>
> -	if (hpriv->target_pwr)
> -		regulator_disable(hpriv->target_pwr);
> +	ahci_platform_disable_regulators(hpriv);
>   }
>   EXPORT_SYMBOL_GPL(ahci_platform_disable_resources);
>
> @@ -218,6 +268,59 @@ static void ahci_platform_put_resources(struct device *dev, void *res)
>   		clk_put(hpriv->clks[c]);
>   }
>
> +static int ahci_platform_get_phy(struct ahci_host_priv *hpriv, u32 port,
> +				struct device *dev, struct device_node *node)
> +{
> +	int rc;
> +
> +	hpriv->phys[port] = devm_of_phy_get(dev, node, NULL);
> +
> +	if (!IS_ERR(hpriv->phys[port]))
> +		return 0;
> +
> +	rc = PTR_ERR(hpriv->phys[port]);
> +	switch (rc) {
> +	case -ENOSYS:
> +		/* No PHY support. Check if PHY is required. */
> +		if (of_find_property(node, "phys", NULL)) {
> +			dev_err(dev,
> +				"couldn't get PHY in node %s: ENOSYS\n",
> +				node->name);
> +			break;
> +		}
> +	case -ENODEV:
> +		/* continue normally */
> +		hpriv->phys[port] = NULL;
> +		rc = 0;
> +		break;
> +
> +	default:
> +		dev_err(dev,
> +			"couldn't get PHY in node %s: %d\n",
> +			node->name, rc);
> +
> +		break;
> +	}
> +
> +	return rc;
> +}
> +
> +static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
> +				struct device *dev, struct device_node *node)
> +{
> +	struct regulator *target_pwr;
> +	int rc = 0;
> +
> +	target_pwr = devm_of_regulator_get_optional(dev, "target", node);
> +
> +	if (!IS_ERR(target_pwr))
> +		hpriv->target_pwrs[port] = target_pwr;
> +	else
> +		rc = PTR_ERR(target_pwr);
> +
> +	return rc;
> +}
> +
>   /**
>    * ahci_platform_get_resources - Get platform resources
>    * @pdev: platform device to get resources for
> @@ -240,7 +343,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>   	struct ahci_host_priv *hpriv;
>   	struct clk *clk;
>   	struct device_node *child;
> -	int i, enabled_ports = 0, rc = -ENOMEM;
> +	int i, sz, enabled_ports = 0, rc = -ENOMEM;
>   	u32 mask_port_map = 0;
>
>   	if (!devres_open_group(dev, NULL, GFP_KERNEL))
> @@ -261,14 +364,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>   		goto err_out;
>   	}
>
> -	hpriv->target_pwr = devm_regulator_get_optional(dev, "target");
> -	if (IS_ERR(hpriv->target_pwr)) {
> -		rc = PTR_ERR(hpriv->target_pwr);
> -		if (rc == -EPROBE_DEFER)
> -			goto err_out;
> -		hpriv->target_pwr = NULL;
> -	}
> -
>   	for (i = 0; i < AHCI_MAX_CLKS; i++) {
>   		/*
>   		 * For now we must use clk_get(dev, NULL) for the first clock,
> @@ -292,15 +387,24 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>
>   	hpriv->nports = of_get_child_count(dev->of_node);
>
> -	if (hpriv->nports) {
> -		hpriv->phys = devm_kzalloc(dev,
> -					   hpriv->nports * sizeof(*hpriv->phys),
> -					   GFP_KERNEL);
> -		if (!hpriv->phys) {
> -			rc = -ENOMEM;
> -			goto err_out;
> -		}
> +	 /* If no sub-node was found, keep this for device tree compatibility */
> +	if (!hpriv->nports)
> +		hpriv->nports = 1;

So now nports is always >= 1.

> +
> +	sz = hpriv->nports * sizeof(*hpriv->phys);
> +	hpriv->phys = devm_kzalloc(dev, sz, GFP_KERNEL);
> +	if (!hpriv->phys) {
> +		rc = -ENOMEM;
> +		goto err_out;
> +	}
> +	sz = hpriv->nports * sizeof(*hpriv->target_pwrs);
> +	hpriv->target_pwrs = devm_kzalloc(dev, sz, GFP_KERNEL);
> +	if (!hpriv->target_pwrs) {
> +		rc = -ENOMEM;
> +		goto err_out;
> +	}
>
> +	if (hpriv->nports) {

And this is always true,

>   		for_each_child_of_node(dev->of_node, child) {
>   			u32 port;
>
> @@ -319,14 +423,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>
>   			mask_port_map |= BIT(port);
>
> -			hpriv->phys[port] = devm_of_phy_get(dev, child, NULL);
> -			if (IS_ERR(hpriv->phys[port])) {
> -				rc = PTR_ERR(hpriv->phys[port]);
> -				dev_err(dev,
> -					"couldn't get PHY in node %s: %d\n",
> -					child->name, rc);
> +			rc = ahci_platform_get_regulator(hpriv, port,
> +							 dev, child);
> +			if (rc == -EPROBE_DEFER)
> +				goto err_out;
> +
> +			rc = ahci_platform_get_phy(hpriv, port, dev, child);
> +			if (rc)
>   				goto err_out;
> -			}
>
>   			enabled_ports++;
>   		}
> @@ -343,38 +447,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>   		 * If no sub-node was found, keep this for device tree
>   		 * compatibility
>   		 */

And thus the code below (which is in the else) never gets executed, and you've
broken the case where there are no subnodes.

I think it is best to fix this by keeping nports == 0 when there are no subnodes
(because we really do not know nports then, so advertising 1 is sorta wrong anyways),
and use something like this to calculate the array sizes:

sz = (nports ? nports : 1) * sizeof(*hpriv->phys);
...
sz = (nports ? nports : 1) * sizeof(*hpriv->target_pwrs);

> -		struct phy *phy = devm_phy_get(dev, "sata-phy");
> -		if (!IS_ERR(phy)) {
> -			hpriv->phys = devm_kzalloc(dev, sizeof(*hpriv->phys),
> -						   GFP_KERNEL);
> -			if (!hpriv->phys) {
> -				rc = -ENOMEM;
> -				goto err_out;
> -			}
> -
> -			hpriv->phys[0] = phy;
> -			hpriv->nports = 1;
> -		} else {
> -			rc = PTR_ERR(phy);
> -			switch (rc) {
> -				case -ENOSYS:
> -					/* No PHY support. Check if PHY is required. */
> -					if (of_find_property(dev->of_node, "phys", NULL)) {
> -						dev_err(dev, "couldn't get sata-phy: ENOSYS\n");
> -						goto err_out;
> -					}
> -				case -ENODEV:
> -					/* continue normally */
> -					hpriv->phys = NULL;
> -					break;
> -
> -				default:
> -					goto err_out;
> +		rc = ahci_platform_get_phy(hpriv, 0, dev, dev->of_node);
> +		if (rc)
> +			goto err_out;
>
> -			}
> -		}
> +		rc = ahci_platform_get_regulator(hpriv, 0, dev, dev->of_node);
> +		if (rc == -EPROBE_DEFER)
> +			goto err_out;
>   	}
> -
>   	pm_runtime_enable(dev);
>   	pm_runtime_get_sync(dev);
>   	hpriv->got_runtime_pm = true;
> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> index 642d6ae4030c..f65b33809170 100644
> --- a/include/linux/ahci_platform.h
> +++ b/include/linux/ahci_platform.h
> @@ -24,6 +24,8 @@ struct platform_device;
>
>   int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
>   void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
> +int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv);
> +void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv);
>   int ahci_platform_enable_resources(struct ahci_host_priv *hpriv);
>   void ahci_platform_disable_resources(struct ahci_host_priv *hpriv);
>   struct ahci_host_priv *ahci_platform_get_resources(
>

Regards,

Hans