[PATCH 1/4] phy: qcom-ufs: Don't kfree devres resource

[PATCH 1/4] phy: qcom-ufs: Don't kfree devres resource

[Bjorn Andersson]

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;
 }
 
-- 
2.11.0

[PATCH 2/4] phy: qcom-ufs: Correct usage of regulator_get()

[Bjorn Andersson]

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

Re: [PATCH 2/4] phy: qcom-ufs: Correct usage of regulator_get()

[vivek.gautam]

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

Re: [PATCH 2/4] phy: qcom-ufs: Correct usage of regulator_get()

[Subhash Jadavani]

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

[PATCH 3/4] phy: qcom-ufs: Remove -always-on property

[Bjorn Andersson]

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

Re: [PATCH 3/4] phy: qcom-ufs: Remove -always-on property

[Vivek Gautam]

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

Re: [PATCH 3/4] phy: qcom-ufs: Remove -always-on property

[Subhash Jadavani]

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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Cc: Vivek Gautam <vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] phy: qcom-ufs: Remove -always-on property

[Subhash Jadavani]

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

Re: [PATCH 3/4] phy: qcom-ufs: Remove -always-on property

[Rob Herring]

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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Cc: Vivek Gautam <vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] phy: qcom-ufs: Remove -always-on property

[Rob Herring]

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>

[PATCH 4/4] phy: qcom-ufs: Suppress extraneous logging

[Bjorn Andersson]

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

Re: [PATCH 4/4] phy: qcom-ufs: Suppress extraneous logging

[Vivek Gautam]

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

Re: [PATCH 4/4] phy: qcom-ufs: Suppress extraneous logging

[Subhash Jadavani]

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

Re: [PATCH 1/4] phy: qcom-ufs: Don't kfree devres resource

[Vivek Gautam]

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

Re: [PATCH 1/4] phy: qcom-ufs: Don't kfree devres resource

[Subhash Jadavani]

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