* [PATCH 2/4] phy: qcom-ufs: Correct usage of regulator_get()
2017-01-19 10:47 [PATCH 1/4] phy: qcom-ufs: Don't kfree devres resource Bjorn Andersson
@ 2017-01-19 10:47 ` Bjorn Andersson
2017-01-19 19:33 ` vivek.gautam
2017-01-20 22:50 ` Subhash Jadavani
2017-01-19 10:47 ` [PATCH 3/4] phy: qcom-ufs: Remove -always-on property Bjorn Andersson
` (3 subsequent siblings)
4 siblings, 2 replies; 13+ messages in thread
From: Bjorn Andersson @ 2017-01-19 10:47 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: linux-kernel, linux-arm-msm, Subhash Jadavani, Vivek Gautam
When regulator_get() tries to resolve a regulator supply but fail to
find a matching property in DeviceTree it returns a dummy regulator, if
a matching supply is specified but unavailable the regulator core will
return an error.
Based on this we should not ignore errors upon failing to acquire the
optional "vddp-ref-clk" supply.
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
drivers/phy/phy-qcom-ufs.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/drivers/phy/phy-qcom-ufs.c b/drivers/phy/phy-qcom-ufs.c
index 4d7f3c018223..bbd317158084 100644
--- a/drivers/phy/phy-qcom-ufs.c
+++ b/drivers/phy/phy-qcom-ufs.c
@@ -210,8 +210,9 @@ int ufs_qcom_phy_init_clks(struct ufs_qcom_phy *phy_common)
}
EXPORT_SYMBOL_GPL(ufs_qcom_phy_init_clks);
-static int __ufs_qcom_phy_init_vreg(struct device *dev,
- struct ufs_qcom_phy_vreg *vreg, const char *name, bool optional)
+static int ufs_qcom_phy_init_vreg(struct device *dev,
+ struct ufs_qcom_phy_vreg *vreg,
+ const char *name)
{
int err = 0;
@@ -221,9 +222,7 @@ static int __ufs_qcom_phy_init_vreg(struct device *dev,
vreg->reg = devm_regulator_get(dev, name);
if (IS_ERR(vreg->reg)) {
err = PTR_ERR(vreg->reg);
- vreg->reg = NULL;
- if (!optional)
- dev_err(dev, "failed to get %s, %d\n", name, err);
+ dev_err(dev, "failed to get %s, %d\n", name, err);
goto out;
}
@@ -263,12 +262,6 @@ static int __ufs_qcom_phy_init_vreg(struct device *dev,
return err;
}
-static int ufs_qcom_phy_init_vreg(struct device *dev,
- struct ufs_qcom_phy_vreg *vreg, const char *name)
-{
- return __ufs_qcom_phy_init_vreg(dev, vreg, name, false);
-}
-
int ufs_qcom_phy_init_vregulators(struct ufs_qcom_phy *phy_common)
{
int err;
@@ -284,9 +277,9 @@ int ufs_qcom_phy_init_vregulators(struct ufs_qcom_phy *phy_common)
if (err)
goto out;
- /* vddp-ref-clk-* properties are optional */
- __ufs_qcom_phy_init_vreg(phy_common->dev, &phy_common->vddp_ref_clk,
- "vddp-ref-clk", true);
+ err = ufs_qcom_phy_init_vreg(phy_common->dev, &phy_common->vddp_ref_clk,
+ "vddp-ref-clk");
+
out:
return err;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] phy: qcom-ufs: Correct usage of regulator_get()
2017-01-19 10:47 ` [PATCH 2/4] phy: qcom-ufs: Correct usage of regulator_get() Bjorn Andersson
@ 2017-01-19 19:33 ` vivek.gautam
2017-01-20 22:50 ` Subhash Jadavani
1 sibling, 0 replies; 13+ messages in thread
From: vivek.gautam @ 2017-01-19 19:33 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Kishon Vijay Abraham I, linux-kernel, linux-arm-msm, Subhash Jadavani
On 2017-01-19 16:17, Bjorn Andersson wrote:
> When regulator_get() tries to resolve a regulator supply but fail to
> find a matching property in DeviceTree it returns a dummy regulator, if
> a matching supply is specified but unavailable the regulator core will
> return an error.
>
> Based on this we should not ignore errors upon failing to acquire the
> optional "vddp-ref-clk" supply.
>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
Yea, we don't need additional flag to indicate optional regulator.
Change looks good.
Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Regards
Vivek
> drivers/phy/phy-qcom-ufs.c | 21 +++++++--------------
> 1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/phy/phy-qcom-ufs.c b/drivers/phy/phy-qcom-ufs.c
> index 4d7f3c018223..bbd317158084 100644
> --- a/drivers/phy/phy-qcom-ufs.c
> +++ b/drivers/phy/phy-qcom-ufs.c
> @@ -210,8 +210,9 @@ int ufs_qcom_phy_init_clks(struct ufs_qcom_phy
> *phy_common)
> }
> EXPORT_SYMBOL_GPL(ufs_qcom_phy_init_clks);
>
> -static int __ufs_qcom_phy_init_vreg(struct device *dev,
> - struct ufs_qcom_phy_vreg *vreg, const char *name, bool optional)
> +static int ufs_qcom_phy_init_vreg(struct device *dev,
> + struct ufs_qcom_phy_vreg *vreg,
> + const char *name)
> {
> int err = 0;
>
> @@ -221,9 +222,7 @@ static int __ufs_qcom_phy_init_vreg(struct device
> *dev,
> vreg->reg = devm_regulator_get(dev, name);
> if (IS_ERR(vreg->reg)) {
> err = PTR_ERR(vreg->reg);
> - vreg->reg = NULL;
> - if (!optional)
> - dev_err(dev, "failed to get %s, %d\n", name, err);
> + dev_err(dev, "failed to get %s, %d\n", name, err);
> goto out;
> }
>
> @@ -263,12 +262,6 @@ static int __ufs_qcom_phy_init_vreg(struct device
> *dev,
> return err;
> }
>
> -static int ufs_qcom_phy_init_vreg(struct device *dev,
> - struct ufs_qcom_phy_vreg *vreg, const char *name)
> -{
> - return __ufs_qcom_phy_init_vreg(dev, vreg, name, false);
> -}
> -
> int ufs_qcom_phy_init_vregulators(struct ufs_qcom_phy *phy_common)
> {
> int err;
> @@ -284,9 +277,9 @@ int ufs_qcom_phy_init_vregulators(struct
> ufs_qcom_phy *phy_common)
> if (err)
> goto out;
>
> - /* vddp-ref-clk-* properties are optional */
> - __ufs_qcom_phy_init_vreg(phy_common->dev, &phy_common->vddp_ref_clk,
> - "vddp-ref-clk", true);
> + err = ufs_qcom_phy_init_vreg(phy_common->dev,
> &phy_common->vddp_ref_clk,
> + "vddp-ref-clk");
> +
> out:
> return err;
> }
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] phy: qcom-ufs: Correct usage of regulator_get()
2017-01-19 10:47 ` [PATCH 2/4] phy: qcom-ufs: Correct usage of regulator_get() Bjorn Andersson
2017-01-19 19:33 ` vivek.gautam
@ 2017-01-20 22:50 ` Subhash Jadavani
1 sibling, 0 replies; 13+ messages in thread
From: Subhash Jadavani @ 2017-01-20 22:50 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Kishon Vijay Abraham I, linux-kernel, linux-arm-msm, Vivek Gautam
On 2017-01-19 02:47, Bjorn Andersson wrote:
> When regulator_get() tries to resolve a regulator supply but fail to
> find a matching property in DeviceTree it returns a dummy regulator, if
> a matching supply is specified but unavailable the regulator core will
> return an error.
>
> Based on this we should not ignore errors upon failing to acquire the
> optional "vddp-ref-clk" supply.
>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/phy/phy-qcom-ufs.c | 21 +++++++--------------
> 1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/phy/phy-qcom-ufs.c b/drivers/phy/phy-qcom-ufs.c
> index 4d7f3c018223..bbd317158084 100644
> --- a/drivers/phy/phy-qcom-ufs.c
> +++ b/drivers/phy/phy-qcom-ufs.c
> @@ -210,8 +210,9 @@ int ufs_qcom_phy_init_clks(struct ufs_qcom_phy
> *phy_common)
> }
> EXPORT_SYMBOL_GPL(ufs_qcom_phy_init_clks);
>
> -static int __ufs_qcom_phy_init_vreg(struct device *dev,
> - struct ufs_qcom_phy_vreg *vreg, const char *name, bool optional)
> +static int ufs_qcom_phy_init_vreg(struct device *dev,
> + struct ufs_qcom_phy_vreg *vreg,
> + const char *name)
> {
> int err = 0;
>
> @@ -221,9 +222,7 @@ static int __ufs_qcom_phy_init_vreg(struct device
> *dev,
> vreg->reg = devm_regulator_get(dev, name);
> if (IS_ERR(vreg->reg)) {
> err = PTR_ERR(vreg->reg);
> - vreg->reg = NULL;
> - if (!optional)
> - dev_err(dev, "failed to get %s, %d\n", name, err);
> + dev_err(dev, "failed to get %s, %d\n", name, err);
> goto out;
> }
>
> @@ -263,12 +262,6 @@ static int __ufs_qcom_phy_init_vreg(struct device
> *dev,
> return err;
> }
>
> -static int ufs_qcom_phy_init_vreg(struct device *dev,
> - struct ufs_qcom_phy_vreg *vreg, const char *name)
> -{
> - return __ufs_qcom_phy_init_vreg(dev, vreg, name, false);
> -}
> -
> int ufs_qcom_phy_init_vregulators(struct ufs_qcom_phy *phy_common)
> {
> int err;
> @@ -284,9 +277,9 @@ int ufs_qcom_phy_init_vregulators(struct
> ufs_qcom_phy *phy_common)
> if (err)
> goto out;
>
> - /* vddp-ref-clk-* properties are optional */
> - __ufs_qcom_phy_init_vreg(phy_common->dev, &phy_common->vddp_ref_clk,
> - "vddp-ref-clk", true);
> + err = ufs_qcom_phy_init_vreg(phy_common->dev,
> &phy_common->vddp_ref_clk,
> + "vddp-ref-clk");
> +
> out:
> return err;
> }
Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/4] phy: qcom-ufs: Remove -always-on property
2017-01-19 10:47 [PATCH 1/4] phy: qcom-ufs: Don't kfree devres resource Bjorn Andersson
2017-01-19 10:47 ` [PATCH 2/4] phy: qcom-ufs: Correct usage of regulator_get() Bjorn Andersson
@ 2017-01-19 10:47 ` Bjorn Andersson
2017-01-19 19:39 ` Vivek Gautam
` (2 more replies)
2017-01-19 10:47 ` [PATCH 4/4] phy: qcom-ufs: Suppress extraneous logging Bjorn Andersson
` (2 subsequent siblings)
4 siblings, 3 replies; 13+ messages in thread
From: Bjorn Andersson @ 2017-01-19 10:47 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: linux-kernel, linux-arm-msm, Rob Herring, Mark Rutland,
devicetree, Subhash Jadavani, Vivek Gautam
The fact that a regulator is always-on is a property of the regulator,
not a specific consumer. Implementing this in the driver leads to a
system behaviour that is dependent on if the Qualcomm UFS PHY was ever
(partially) probed.
If the specific regulator should be always on in a particular device,
mark it so by specifying "regulator-always-on" in the regulator node.
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
Documentation/devicetree/bindings/ufs/ufs-qcom.txt | 1 -
drivers/phy/phy-qcom-ufs-i.h | 1 -
drivers/phy/phy-qcom-ufs.c | 5 +----
3 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
index b6b5130e5f65..1f69ee1a61ea 100644
--- a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
+++ b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
@@ -29,7 +29,6 @@ Optional properties:
- vdda-pll-max-microamp : specifies max. load that can be drawn from pll supply
- vddp-ref-clk-supply : phandle to UFS device ref_clk pad power supply
- vddp-ref-clk-max-microamp : specifies max. load that can be drawn from this supply
-- vddp-ref-clk-always-on : specifies if this supply needs to be kept always on
Example:
diff --git a/drivers/phy/phy-qcom-ufs-i.h b/drivers/phy/phy-qcom-ufs-i.h
index d505d98cf5f8..13b02b7de30b 100644
--- a/drivers/phy/phy-qcom-ufs-i.h
+++ b/drivers/phy/phy-qcom-ufs-i.h
@@ -77,7 +77,6 @@ struct ufs_qcom_phy_vreg {
int min_uV;
int max_uV;
bool enabled;
- bool is_always_on;
};
struct ufs_qcom_phy {
diff --git a/drivers/phy/phy-qcom-ufs.c b/drivers/phy/phy-qcom-ufs.c
index bbd317158084..c145fa6e824c 100644
--- a/drivers/phy/phy-qcom-ufs.c
+++ b/drivers/phy/phy-qcom-ufs.c
@@ -242,9 +242,6 @@ static int ufs_qcom_phy_init_vreg(struct device *dev,
}
err = 0;
}
- snprintf(prop_name, MAX_PROP_NAME, "%s-always-on", name);
- vreg->is_always_on = of_property_read_bool(dev->of_node,
- prop_name);
}
if (!strcmp(name, "vdda-pll")) {
@@ -402,7 +399,7 @@ static int ufs_qcom_phy_disable_vreg(struct device *dev,
{
int ret = 0;
- if (!vreg || !vreg->enabled || vreg->is_always_on)
+ if (!vreg || !vreg->enabled)
goto out;
ret = regulator_disable(vreg->reg);
--
2.11.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] phy: qcom-ufs: Remove -always-on property
2017-01-19 10:47 ` [PATCH 3/4] phy: qcom-ufs: Remove -always-on property Bjorn Andersson
@ 2017-01-19 19:39 ` Vivek Gautam
2017-01-20 22:53 ` Subhash Jadavani
2017-01-21 20:43 ` Rob Herring
2 siblings, 0 replies; 13+ messages in thread
From: Vivek Gautam @ 2017-01-19 19:39 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Kishon Vijay Abraham I, linux-kernel, linux-arm-msm, Rob Herring,
Mark Rutland, devicetree, Subhash Jadavani
On Thu, Jan 19, 2017 at 4:17 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> The fact that a regulator is always-on is a property of the regulator,
> not a specific consumer. Implementing this in the driver leads to a
> system behaviour that is dependent on if the Qualcomm UFS PHY was ever
> (partially) probed.
>
> If the specific regulator should be always on in a particular device,
> mark it so by specifying "regulator-always-on" in the regulator node.
>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
Looks good.
Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Regards
Vivek
> Documentation/devicetree/bindings/ufs/ufs-qcom.txt | 1 -
> drivers/phy/phy-qcom-ufs-i.h | 1 -
> drivers/phy/phy-qcom-ufs.c | 5 +----
> 3 files changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> index b6b5130e5f65..1f69ee1a61ea 100644
> --- a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> @@ -29,7 +29,6 @@ Optional properties:
> - vdda-pll-max-microamp : specifies max. load that can be drawn from pll supply
> - vddp-ref-clk-supply : phandle to UFS device ref_clk pad power supply
> - vddp-ref-clk-max-microamp : specifies max. load that can be drawn from this supply
> -- vddp-ref-clk-always-on : specifies if this supply needs to be kept always on
>
> Example:
>
> diff --git a/drivers/phy/phy-qcom-ufs-i.h b/drivers/phy/phy-qcom-ufs-i.h
> index d505d98cf5f8..13b02b7de30b 100644
> --- a/drivers/phy/phy-qcom-ufs-i.h
> +++ b/drivers/phy/phy-qcom-ufs-i.h
> @@ -77,7 +77,6 @@ struct ufs_qcom_phy_vreg {
> int min_uV;
> int max_uV;
> bool enabled;
> - bool is_always_on;
> };
>
> struct ufs_qcom_phy {
> diff --git a/drivers/phy/phy-qcom-ufs.c b/drivers/phy/phy-qcom-ufs.c
> index bbd317158084..c145fa6e824c 100644
> --- a/drivers/phy/phy-qcom-ufs.c
> +++ b/drivers/phy/phy-qcom-ufs.c
> @@ -242,9 +242,6 @@ static int ufs_qcom_phy_init_vreg(struct device *dev,
> }
> err = 0;
> }
> - snprintf(prop_name, MAX_PROP_NAME, "%s-always-on", name);
> - vreg->is_always_on = of_property_read_bool(dev->of_node,
> - prop_name);
> }
>
> if (!strcmp(name, "vdda-pll")) {
> @@ -402,7 +399,7 @@ static int ufs_qcom_phy_disable_vreg(struct device *dev,
> {
> int ret = 0;
>
> - if (!vreg || !vreg->enabled || vreg->is_always_on)
> + if (!vreg || !vreg->enabled)
> goto out;
>
> ret = regulator_disable(vreg->reg);
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] phy: qcom-ufs: Remove -always-on property
2017-01-19 10:47 ` [PATCH 3/4] phy: qcom-ufs: Remove -always-on property Bjorn Andersson
2017-01-19 19:39 ` Vivek Gautam
@ 2017-01-20 22:53 ` Subhash Jadavani
2017-01-21 20:43 ` Rob Herring
2 siblings, 0 replies; 13+ messages in thread
From: Subhash Jadavani @ 2017-01-20 22:53 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Kishon Vijay Abraham I, linux-kernel, linux-arm-msm, Rob Herring,
Mark Rutland, devicetree, Vivek Gautam
On 2017-01-19 02:47, Bjorn Andersson wrote:
> The fact that a regulator is always-on is a property of the regulator,
> not a specific consumer. Implementing this in the driver leads to a
> system behaviour that is dependent on if the Qualcomm UFS PHY was ever
> (partially) probed.
>
> If the specific regulator should be always on in a particular device,
> mark it so by specifying "regulator-always-on" in the regulator node.
>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> Documentation/devicetree/bindings/ufs/ufs-qcom.txt | 1 -
> drivers/phy/phy-qcom-ufs-i.h | 1 -
> drivers/phy/phy-qcom-ufs.c | 5 +----
> 3 files changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> index b6b5130e5f65..1f69ee1a61ea 100644
> --- a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> @@ -29,7 +29,6 @@ Optional properties:
> - vdda-pll-max-microamp : specifies max. load that can be drawn from
> pll supply
> - vddp-ref-clk-supply : phandle to UFS device ref_clk pad power
> supply
> - vddp-ref-clk-max-microamp : specifies max. load that can be drawn
> from this supply
> -- vddp-ref-clk-always-on : specifies if this supply needs to be kept
> always on
>
> Example:
>
> diff --git a/drivers/phy/phy-qcom-ufs-i.h
> b/drivers/phy/phy-qcom-ufs-i.h
> index d505d98cf5f8..13b02b7de30b 100644
> --- a/drivers/phy/phy-qcom-ufs-i.h
> +++ b/drivers/phy/phy-qcom-ufs-i.h
> @@ -77,7 +77,6 @@ struct ufs_qcom_phy_vreg {
> int min_uV;
> int max_uV;
> bool enabled;
> - bool is_always_on;
> };
>
> struct ufs_qcom_phy {
> diff --git a/drivers/phy/phy-qcom-ufs.c b/drivers/phy/phy-qcom-ufs.c
> index bbd317158084..c145fa6e824c 100644
> --- a/drivers/phy/phy-qcom-ufs.c
> +++ b/drivers/phy/phy-qcom-ufs.c
> @@ -242,9 +242,6 @@ static int ufs_qcom_phy_init_vreg(struct device
> *dev,
> }
> err = 0;
> }
> - snprintf(prop_name, MAX_PROP_NAME, "%s-always-on", name);
> - vreg->is_always_on = of_property_read_bool(dev->of_node,
> - prop_name);
> }
>
> if (!strcmp(name, "vdda-pll")) {
> @@ -402,7 +399,7 @@ static int ufs_qcom_phy_disable_vreg(struct device
> *dev,
> {
> int ret = 0;
>
> - if (!vreg || !vreg->enabled || vreg->is_always_on)
> + if (!vreg || !vreg->enabled)
> goto out;
>
> ret = regulator_disable(vreg->reg);
Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] phy: qcom-ufs: Remove -always-on property
2017-01-19 10:47 ` [PATCH 3/4] phy: qcom-ufs: Remove -always-on property Bjorn Andersson
2017-01-19 19:39 ` Vivek Gautam
2017-01-20 22:53 ` Subhash Jadavani
@ 2017-01-21 20:43 ` Rob Herring
2 siblings, 0 replies; 13+ messages in thread
From: Rob Herring @ 2017-01-21 20:43 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Kishon Vijay Abraham I, linux-kernel, linux-arm-msm,
Mark Rutland, devicetree, Subhash Jadavani, Vivek Gautam
On Thu, Jan 19, 2017 at 02:47:38AM -0800, Bjorn Andersson wrote:
> The fact that a regulator is always-on is a property of the regulator,
> not a specific consumer. Implementing this in the driver leads to a
> system behaviour that is dependent on if the Qualcomm UFS PHY was ever
> (partially) probed.
>
> If the specific regulator should be always on in a particular device,
> mark it so by specifying "regulator-always-on" in the regulator node.
>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> Documentation/devicetree/bindings/ufs/ufs-qcom.txt | 1 -
> drivers/phy/phy-qcom-ufs-i.h | 1 -
> drivers/phy/phy-qcom-ufs.c | 5 +----
> 3 files changed, 1 insertion(+), 6 deletions(-)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] phy: qcom-ufs: Suppress extraneous logging
2017-01-19 10:47 [PATCH 1/4] phy: qcom-ufs: Don't kfree devres resource Bjorn Andersson
2017-01-19 10:47 ` [PATCH 2/4] phy: qcom-ufs: Correct usage of regulator_get() Bjorn Andersson
2017-01-19 10:47 ` [PATCH 3/4] phy: qcom-ufs: Remove -always-on property Bjorn Andersson
@ 2017-01-19 10:47 ` Bjorn Andersson
2017-01-19 20:07 ` Vivek Gautam
2017-01-20 22:54 ` Subhash Jadavani
2017-01-19 20:09 ` [PATCH 1/4] phy: qcom-ufs: Don't kfree devres resource Vivek Gautam
2017-01-20 20:10 ` Subhash Jadavani
4 siblings, 2 replies; 13+ messages in thread
From: Bjorn Andersson @ 2017-01-19 10:47 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: linux-kernel, linux-arm-msm, Subhash Jadavani, Vivek Gautam
The error paths of the common qcom-ufs functions for registering the
phy, acquiring clocks and acquiring regulators all print specific error
messages before returning an error, so there is no value in printing yet
another - more generic - message when this occur.
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
drivers/phy/phy-qcom-ufs-qmp-14nm.c | 15 +++------------
drivers/phy/phy-qcom-ufs-qmp-20nm.c | 12 ++----------
2 files changed, 5 insertions(+), 22 deletions(-)
diff --git a/drivers/phy/phy-qcom-ufs-qmp-14nm.c b/drivers/phy/phy-qcom-ufs-qmp-14nm.c
index c71c84734916..12a1b498dc4b 100644
--- a/drivers/phy/phy-qcom-ufs-qmp-14nm.c
+++ b/drivers/phy/phy-qcom-ufs-qmp-14nm.c
@@ -132,27 +132,18 @@ static int ufs_qcom_phy_qmp_14nm_probe(struct platform_device *pdev)
&ufs_qcom_phy_qmp_14nm_phy_ops, &phy_14nm_ops);
if (!generic_phy) {
- dev_err(dev, "%s: ufs_qcom_phy_generic_probe() failed\n",
- __func__);
err = -EIO;
goto out;
}
err = ufs_qcom_phy_init_clks(phy_common);
- if (err) {
- dev_err(phy_common->dev,
- "%s: ufs_qcom_phy_init_clks() failed %d\n",
- __func__, err);
+ if (err)
goto out;
- }
err = ufs_qcom_phy_init_vregulators(phy_common);
- if (err) {
- dev_err(phy_common->dev,
- "%s: ufs_qcom_phy_init_vregulators() failed %d\n",
- __func__, err);
+ if (err)
goto out;
- }
+
phy_common->vdda_phy.max_uV = UFS_PHY_VDDA_PHY_UV;
phy_common->vdda_phy.min_uV = UFS_PHY_VDDA_PHY_UV;
diff --git a/drivers/phy/phy-qcom-ufs-qmp-20nm.c b/drivers/phy/phy-qcom-ufs-qmp-20nm.c
index 1a26a64e06d3..4f68acb58b73 100644
--- a/drivers/phy/phy-qcom-ufs-qmp-20nm.c
+++ b/drivers/phy/phy-qcom-ufs-qmp-20nm.c
@@ -190,25 +190,17 @@ static int ufs_qcom_phy_qmp_20nm_probe(struct platform_device *pdev)
&ufs_qcom_phy_qmp_20nm_phy_ops, &phy_20nm_ops);
if (!generic_phy) {
- dev_err(dev, "%s: ufs_qcom_phy_generic_probe() failed\n",
- __func__);
err = -EIO;
goto out;
}
err = ufs_qcom_phy_init_clks(phy_common);
- if (err) {
- dev_err(phy_common->dev, "%s: ufs_qcom_phy_init_clks() failed %d\n",
- __func__, err);
+ if (err)
goto out;
- }
err = ufs_qcom_phy_init_vregulators(phy_common);
- if (err) {
- dev_err(phy_common->dev, "%s: ufs_qcom_phy_init_vregulators() failed %d\n",
- __func__, err);
+ if (err)
goto out;
- }
ufs_qcom_phy_qmp_20nm_advertise_quirks(phy_common);
--
2.11.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] phy: qcom-ufs: Suppress extraneous logging
2017-01-19 10:47 ` [PATCH 4/4] phy: qcom-ufs: Suppress extraneous logging Bjorn Andersson
@ 2017-01-19 20:07 ` Vivek Gautam
2017-01-20 22:54 ` Subhash Jadavani
1 sibling, 0 replies; 13+ messages in thread
From: Vivek Gautam @ 2017-01-19 20:07 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Kishon Vijay Abraham I, linux-kernel, linux-arm-msm, Subhash Jadavani
On Thu, Jan 19, 2017 at 4:17 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> The error paths of the common qcom-ufs functions for registering the
> phy, acquiring clocks and acquiring regulators all print specific error
> messages before returning an error, so there is no value in printing yet
> another - more generic - message when this occur.
>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Regards
Vivek
> drivers/phy/phy-qcom-ufs-qmp-14nm.c | 15 +++------------
> drivers/phy/phy-qcom-ufs-qmp-20nm.c | 12 ++----------
> 2 files changed, 5 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/phy/phy-qcom-ufs-qmp-14nm.c b/drivers/phy/phy-qcom-ufs-qmp-14nm.c
> index c71c84734916..12a1b498dc4b 100644
> --- a/drivers/phy/phy-qcom-ufs-qmp-14nm.c
> +++ b/drivers/phy/phy-qcom-ufs-qmp-14nm.c
> @@ -132,27 +132,18 @@ static int ufs_qcom_phy_qmp_14nm_probe(struct platform_device *pdev)
> &ufs_qcom_phy_qmp_14nm_phy_ops, &phy_14nm_ops);
>
> if (!generic_phy) {
> - dev_err(dev, "%s: ufs_qcom_phy_generic_probe() failed\n",
> - __func__);
> err = -EIO;
> goto out;
> }
>
> err = ufs_qcom_phy_init_clks(phy_common);
> - if (err) {
> - dev_err(phy_common->dev,
> - "%s: ufs_qcom_phy_init_clks() failed %d\n",
> - __func__, err);
> + if (err)
> goto out;
> - }
>
> err = ufs_qcom_phy_init_vregulators(phy_common);
> - if (err) {
> - dev_err(phy_common->dev,
> - "%s: ufs_qcom_phy_init_vregulators() failed %d\n",
> - __func__, err);
> + if (err)
> goto out;
> - }
> +
> phy_common->vdda_phy.max_uV = UFS_PHY_VDDA_PHY_UV;
> phy_common->vdda_phy.min_uV = UFS_PHY_VDDA_PHY_UV;
>
> diff --git a/drivers/phy/phy-qcom-ufs-qmp-20nm.c b/drivers/phy/phy-qcom-ufs-qmp-20nm.c
> index 1a26a64e06d3..4f68acb58b73 100644
> --- a/drivers/phy/phy-qcom-ufs-qmp-20nm.c
> +++ b/drivers/phy/phy-qcom-ufs-qmp-20nm.c
> @@ -190,25 +190,17 @@ static int ufs_qcom_phy_qmp_20nm_probe(struct platform_device *pdev)
> &ufs_qcom_phy_qmp_20nm_phy_ops, &phy_20nm_ops);
>
> if (!generic_phy) {
> - dev_err(dev, "%s: ufs_qcom_phy_generic_probe() failed\n",
> - __func__);
> err = -EIO;
> goto out;
> }
>
> err = ufs_qcom_phy_init_clks(phy_common);
> - if (err) {
> - dev_err(phy_common->dev, "%s: ufs_qcom_phy_init_clks() failed %d\n",
> - __func__, err);
> + if (err)
> goto out;
> - }
>
> err = ufs_qcom_phy_init_vregulators(phy_common);
> - if (err) {
> - dev_err(phy_common->dev, "%s: ufs_qcom_phy_init_vregulators() failed %d\n",
> - __func__, err);
> + if (err)
> goto out;
> - }
>
> ufs_qcom_phy_qmp_20nm_advertise_quirks(phy_common);
>
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] phy: qcom-ufs: Suppress extraneous logging
2017-01-19 10:47 ` [PATCH 4/4] phy: qcom-ufs: Suppress extraneous logging Bjorn Andersson
2017-01-19 20:07 ` Vivek Gautam
@ 2017-01-20 22:54 ` Subhash Jadavani
1 sibling, 0 replies; 13+ messages in thread
From: Subhash Jadavani @ 2017-01-20 22:54 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Kishon Vijay Abraham I, linux-kernel, linux-arm-msm, Vivek Gautam
On 2017-01-19 02:47, Bjorn Andersson wrote:
> The error paths of the common qcom-ufs functions for registering the
> phy, acquiring clocks and acquiring regulators all print specific error
> messages before returning an error, so there is no value in printing
> yet
> another - more generic - message when this occur.
>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/phy/phy-qcom-ufs-qmp-14nm.c | 15 +++------------
> drivers/phy/phy-qcom-ufs-qmp-20nm.c | 12 ++----------
> 2 files changed, 5 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/phy/phy-qcom-ufs-qmp-14nm.c
> b/drivers/phy/phy-qcom-ufs-qmp-14nm.c
> index c71c84734916..12a1b498dc4b 100644
> --- a/drivers/phy/phy-qcom-ufs-qmp-14nm.c
> +++ b/drivers/phy/phy-qcom-ufs-qmp-14nm.c
> @@ -132,27 +132,18 @@ static int ufs_qcom_phy_qmp_14nm_probe(struct
> platform_device *pdev)
> &ufs_qcom_phy_qmp_14nm_phy_ops, &phy_14nm_ops);
>
> if (!generic_phy) {
> - dev_err(dev, "%s: ufs_qcom_phy_generic_probe() failed\n",
> - __func__);
> err = -EIO;
> goto out;
> }
>
> err = ufs_qcom_phy_init_clks(phy_common);
> - if (err) {
> - dev_err(phy_common->dev,
> - "%s: ufs_qcom_phy_init_clks() failed %d\n",
> - __func__, err);
> + if (err)
> goto out;
> - }
>
> err = ufs_qcom_phy_init_vregulators(phy_common);
> - if (err) {
> - dev_err(phy_common->dev,
> - "%s: ufs_qcom_phy_init_vregulators() failed %d\n",
> - __func__, err);
> + if (err)
> goto out;
> - }
> +
> phy_common->vdda_phy.max_uV = UFS_PHY_VDDA_PHY_UV;
> phy_common->vdda_phy.min_uV = UFS_PHY_VDDA_PHY_UV;
>
> diff --git a/drivers/phy/phy-qcom-ufs-qmp-20nm.c
> b/drivers/phy/phy-qcom-ufs-qmp-20nm.c
> index 1a26a64e06d3..4f68acb58b73 100644
> --- a/drivers/phy/phy-qcom-ufs-qmp-20nm.c
> +++ b/drivers/phy/phy-qcom-ufs-qmp-20nm.c
> @@ -190,25 +190,17 @@ static int ufs_qcom_phy_qmp_20nm_probe(struct
> platform_device *pdev)
> &ufs_qcom_phy_qmp_20nm_phy_ops, &phy_20nm_ops);
>
> if (!generic_phy) {
> - dev_err(dev, "%s: ufs_qcom_phy_generic_probe() failed\n",
> - __func__);
> err = -EIO;
> goto out;
> }
>
> err = ufs_qcom_phy_init_clks(phy_common);
> - if (err) {
> - dev_err(phy_common->dev, "%s: ufs_qcom_phy_init_clks() failed %d\n",
> - __func__, err);
> + if (err)
> goto out;
> - }
>
> err = ufs_qcom_phy_init_vregulators(phy_common);
> - if (err) {
> - dev_err(phy_common->dev, "%s: ufs_qcom_phy_init_vregulators() failed
> %d\n",
> - __func__, err);
> + if (err)
> goto out;
> - }
>
> ufs_qcom_phy_qmp_20nm_advertise_quirks(phy_common);
Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] phy: qcom-ufs: Don't kfree devres resource
2017-01-19 10:47 [PATCH 1/4] phy: qcom-ufs: Don't kfree devres resource Bjorn Andersson
` (2 preceding siblings ...)
2017-01-19 10:47 ` [PATCH 4/4] phy: qcom-ufs: Suppress extraneous logging Bjorn Andersson
@ 2017-01-19 20:09 ` Vivek Gautam
2017-01-20 20:10 ` Subhash Jadavani
4 siblings, 0 replies; 13+ messages in thread
From: Vivek Gautam @ 2017-01-19 20:09 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Kishon Vijay Abraham I, linux-kernel, linux-arm-msm, Subhash Jadavani
On Thu, Jan 19, 2017 at 4:17 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> Upon failing to acquire regulator supplies the qcom-ufs driver calls
> kfree() on the devm allocated memory used to store the name of the
> regulator, leading to devres corruption.
>
> Rather than switching to using the appropriate free function the patch
> acknowledge the fact that "name" is always a constant string and we
> don't actually need to create a local copy of it, but rather just
> reference the constant string.
>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
This patch fixes:
add78fc05702 phy: qcom-ufs: Use devm sibling of kstrdup for regulator names
Regards
Vivek
> drivers/phy/phy-qcom-ufs.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/phy/phy-qcom-ufs.c b/drivers/phy/phy-qcom-ufs.c
> index c69568b8543d..4d7f3c018223 100644
> --- a/drivers/phy/phy-qcom-ufs.c
> +++ b/drivers/phy/phy-qcom-ufs.c
> @@ -217,12 +217,7 @@ static int __ufs_qcom_phy_init_vreg(struct device *dev,
>
> char prop_name[MAX_PROP_NAME];
>
> - vreg->name = devm_kstrdup(dev, name, GFP_KERNEL);
> - if (!vreg->name) {
> - err = -ENOMEM;
> - goto out;
> - }
> -
> + vreg->name = name;
> vreg->reg = devm_regulator_get(dev, name);
> if (IS_ERR(vreg->reg)) {
> err = PTR_ERR(vreg->reg);
> @@ -265,8 +260,6 @@ static int __ufs_qcom_phy_init_vreg(struct device *dev,
> }
>
> out:
> - if (err)
> - kfree(vreg->name);
> return err;
> }
>
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] phy: qcom-ufs: Don't kfree devres resource
2017-01-19 10:47 [PATCH 1/4] phy: qcom-ufs: Don't kfree devres resource Bjorn Andersson
` (3 preceding siblings ...)
2017-01-19 20:09 ` [PATCH 1/4] phy: qcom-ufs: Don't kfree devres resource Vivek Gautam
@ 2017-01-20 20:10 ` Subhash Jadavani
4 siblings, 0 replies; 13+ messages in thread
From: Subhash Jadavani @ 2017-01-20 20:10 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Kishon Vijay Abraham I, linux-kernel, linux-arm-msm, Vivek Gautam
On 2017-01-19 02:47, Bjorn Andersson wrote:
> Upon failing to acquire regulator supplies the qcom-ufs driver calls
> kfree() on the devm allocated memory used to store the name of the
> regulator, leading to devres corruption.
>
> Rather than switching to using the appropriate free function the patch
> acknowledge the fact that "name" is always a constant string and we
> don't actually need to create a local copy of it, but rather just
> reference the constant string.
>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/phy/phy-qcom-ufs.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/phy/phy-qcom-ufs.c b/drivers/phy/phy-qcom-ufs.c
> index c69568b8543d..4d7f3c018223 100644
> --- a/drivers/phy/phy-qcom-ufs.c
> +++ b/drivers/phy/phy-qcom-ufs.c
> @@ -217,12 +217,7 @@ static int __ufs_qcom_phy_init_vreg(struct device
> *dev,
>
> char prop_name[MAX_PROP_NAME];
>
> - vreg->name = devm_kstrdup(dev, name, GFP_KERNEL);
> - if (!vreg->name) {
> - err = -ENOMEM;
> - goto out;
> - }
> -
> + vreg->name = name;
> vreg->reg = devm_regulator_get(dev, name);
> if (IS_ERR(vreg->reg)) {
> err = PTR_ERR(vreg->reg);
> @@ -265,8 +260,6 @@ static int __ufs_qcom_phy_init_vreg(struct device
> *dev,
> }
>
> out:
> - if (err)
> - kfree(vreg->name);
> return err;
> }
Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 13+ messages in thread