| Message ID | 1419676483-10346-4-git-send-email-gregory.clement@free-electrons.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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 -- 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
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 -- 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
Hi Hans, On 27/12/2014 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. Thanks for your review. About the second patch as the change in the regulator framework didn't have been accepted, I had to change the binding. I will send a the new binding in the next version. > > I've some comments on this patch, see below. > [...] >> +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]); yes you're right. >> + >> + 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), You are definitely right > 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); I will do it. Thanks again for your review, Gregory
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]) + 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; + + 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) { 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 */ - 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(
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(-)