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

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

Hi,

On 27-12-14 13:58, Hans de Goede wrote:
> 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),

Erm, scrap that we were already setting nports = 1 later on in the no child nodes
case to make ahci_platform_[en|dis]able_phys do the right thing.


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

Still using the above and setting nports = 1 in the "else" case is
an option, but I think that adding a local child_nodes var and doing:

hpriv->nports = child_nodes = of_get_child_count(dev->of_node);

And changing the "if (hpriv->nports)" into "if (child_nodes)",
is a better solution.

Regards,

Hans


>
>> -        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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html