[PATCH] drm/msm/dsi: Fix regulator API abuse

[PATCH] drm/msm/dsi: Fix regulator API abuse

[Mark Brown]

The voltage changing code in this driver is broken and should be
removed.  The driver sets a single, exact voltage on probe.  Unless
there is a very good reason for this (which should be documented in
comments) constraints like this need to be set via the machine
constraints, voltage setting in a driver is expected to be used in cases
where the voltage varies at runtime.

In addition client drivers should almost never be calling
regulator_can_set_voltage(), if the device needs to set a voltage it
needs to set the voltage and the regulator core will handle the case
where the regulator is fixed voltage.  If the driver simply skips
setting the voltage if it doesn't have permission then it shouild just
not bother in the first place.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 4282ec6bbaaf..a3e47ad83eb3 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -325,18 +325,6 @@ static int dsi_regulator_init(struct msm_dsi_host *msm_host)
 		return ret;
 	}
 
-	for (i = 0; i < num; i++) {
-		if (regulator_can_change_voltage(s[i].consumer)) {
-			ret = regulator_set_voltage(s[i].consumer,
-				regs[i].min_voltage, regs[i].max_voltage);
-			if (ret < 0) {
-				pr_err("regulator %d set voltage failed, %d\n",
-					i, ret);
-				return ret;
-			}
-		}
-	}
-
 	return 0;
 }
 
-- 
2.8.0.rc3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/msm/dsi: Fix regulator API abuse

[Archit Taneja]

Hi,

On 03/31/2016 12:23 AM, Mark Brown wrote:
> The voltage changing code in this driver is broken and should be
> removed.  The driver sets a single, exact voltage on probe.  Unless
> there is a very good reason for this (which should be documented in
> comments) constraints like this need to be set via the machine
> constraints, voltage setting in a driver is expected to be used in cases
> where the voltage varies at runtime.
>
> In addition client drivers should almost never be calling
> regulator_can_set_voltage(), if the device needs to set a voltage it
> needs to set the voltage and the regulator core will handle the case
> where the regulator is fixed voltage.  If the driver simply skips
> setting the voltage if it doesn't have permission then it shouild just

s/shouild/should

Could you also pull in the diff I've added below? It removes another
set_voltage call from the hdmi driver and the struct members used to
manage the voltage range.

> not bother in the first place.
>

+John, you'll need to update your regulator nodes with the min/max
voltage values in your N7 dtsi whenever you plan to rebase next.

Thanks,
Archit

> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 12 ------------
>   1 file changed, 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 4282ec6bbaaf..a3e47ad83eb3 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -325,18 +325,6 @@ static int dsi_regulator_init(struct msm_dsi_host *msm_host)
>   		return ret;
>   	}
>
> -	for (i = 0; i < num; i++) {
> -		if (regulator_can_change_voltage(s[i].consumer)) {
> -			ret = regulator_set_voltage(s[i].consumer,
> -				regs[i].min_voltage, regs[i].max_voltage);
> -			if (ret < 0) {
> -				pr_err("regulator %d set voltage failed, %d\n",
> -					i, ret);
> -				return ret;
> -			}
> -		}
> -	}
> -
>   	return 0;
>   }
>
>

---8<---

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 749fbb2..03f115f 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -41,8 +41,6 @@ enum msm_dsi_phy_type {
  /* Regulators for DSI devices */
  struct dsi_reg_entry {
  	char name[32];
-	int min_voltage;
-	int max_voltage;
  	int enable_load;
  	int disable_load;
  };
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c 
b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index e58e9b9..cc802bb 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -22,9 +22,9 @@ static const struct msm_dsi_config apq8064_dsi_cfg = {
  	.reg_cfg = {
  		.num = 3,
  		.regs = {
-			{"vdda", 1200000, 1200000, 100000, 100},
-			{"avdd", 3000000, 3000000, 110000, 100},
-			{"vddio", 1800000, 1800000, 100000, 100},
+			{"vdda", 100000, 100},
+			{"avdd", 10000, 100},
+			{"vddio", 100000, 100},
  		},
  	},
  	.bus_clk_names = dsi_v2_bus_clk_names,
@@ -40,10 +40,10 @@ static const struct msm_dsi_config 
msm8974_apq8084_dsi_cfg = {
  	.reg_cfg = {
  		.num = 4,
  		.regs = {
-			{"gdsc", -1, -1, -1, -1},
-			{"vdd", 3000000, 3000000, 150000, 100},
-			{"vdda", 1200000, 1200000, 100000, 100},
-			{"vddio", 1800000, 1800000, 100000, 100},
+			{"gdsc", -1, -1},
+			{"vdd", 150000, 100},
+			{"vdda", 100000, 100},
+			{"vddio", 100000, 100},
  		},
  	},
  	.bus_clk_names = dsi_6g_bus_clk_names,
@@ -59,9 +59,9 @@ static const struct msm_dsi_config msm8916_dsi_cfg = {
  	.reg_cfg = {
  		.num = 3,
  		.regs = {
-			{"gdsc", -1, -1, -1, -1},
-			{"vdda", 1200000, 1200000, 100000, 100},
-			{"vddio", 1800000, 1800000, 100000, 100},
+			{"gdsc", -1, -1},
+			{"vdda", 100000, 100},
+			{"vddio", 100000, 100},
  		},
  	},
  	.bus_clk_names = dsi_8916_bus_clk_names,
@@ -73,13 +73,13 @@ static const struct msm_dsi_config msm8994_dsi_cfg = {
  	.reg_cfg = {
  		.num = 7,
  		.regs = {
-			{"gdsc", -1, -1, -1, -1},
-			{"vdda", 1250000, 1250000, 100000, 100},
-			{"vddio", 1800000, 1800000, 100000, 100},
-			{"vcca", 1000000, 1000000, 10000, 100},
-			{"vdd", 1800000, 1800000, 100000, 100},
-			{"lab_reg", -1, -1, -1, -1},
-			{"ibb_reg", -1, -1, -1, -1},
+			{"gdsc", -1, -1},
+			{"vdda", 100000, 100},
+			{"vddio", 100000, 100},
+			{"vcca", 10000, 100},
+			{"vdd", 100000, 100},
+			{"lab_reg", -1, -1},
+			{"ibb_reg", -1, -1},
  		},
  	},
  	.bus_clk_names = dsi_6g_bus_clk_names,
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 91a95fb..e2f42d8 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -177,19 +177,6 @@ static int dsi_phy_regulator_init(struct 
msm_dsi_phy *phy)
  		return ret;
  	}

-	for (i = 0; i < num; i++) {
-		if (regulator_can_change_voltage(s[i].consumer)) {
-			ret = regulator_set_voltage(s[i].consumer,
-				regs[i].min_voltage, regs[i].max_voltage);
-			if (ret < 0) {
-				dev_err(dev,
-					"regulator %d set voltage failed, %d\n",
-					i, ret);
-				return ret;
-			}
-		}
-	}
-
  	return 0;
  }

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
index 2e9ba11..3c93cfc 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
@@ -138,8 +138,8 @@ const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs = {
  	.reg_cfg = {
  		.num = 2,
  		.regs = {
-			{"vddio", 1800000, 1800000, 100000, 100},
-			{"vcca", 1000000, 1000000, 10000, 100},
+			{"vddio", 100000, 100},
+			{"vcca", 10000, 100},
  		},
  	},
  	.ops = {
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
index edf7411..397f09a 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
@@ -138,7 +138,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs = {
  	.reg_cfg = {
  		.num = 1,
  		.regs = {
-			{"vddio", 1800000, 1800000, 100000, 100},
+			{"vddio", 100000, 100},
  		},
  	},
  	.ops = {
@@ -153,7 +153,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_lp_cfgs = {
  	.reg_cfg = {
  		.num = 1,
  		.regs = {
-			{"vddio", 1800000, 1800000, 100000, 100},
+			{"vddio", 100000, 100},
  		},
  	},
  	.ops = {
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
index 197b039..4ed7b57 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
@@ -185,7 +185,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_8960_cfgs = {
  	.reg_cfg = {
  		.num = 1,
  		.regs = {
-			{"vddio", 1800000, 1800000, 100000, 100},
+			{"vddio", 100000, 100},
  		},
  	},
  	.ops = {

--->8---

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/msm/dsi: Fix regulator API abuse

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 317 bytes --]

On Tue, Apr 05, 2016 at 04:56:02PM +0530, Archit Taneja wrote:

> Could you also pull in the diff I've added below? It removes another
> set_voltage call from the hdmi driver and the struct members used to
> manage the voltage range.

Please update and submit this directly, I'm not adding anything by
sending it on.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/3] drm/msm: Remove regulator_set_voltage calls

[Archit Taneja]

The MSM KMS driver calls regulator_set_voltage to set a fixed voltage in a
few places. This isn't correct API usage as explained in the commit message
of Mark Brown's original patch:

http://www.spinics.net/lists/dri-devel/msg103714.html

This patchset drops all the regultor_set_voltage calls. The first patch
adds some more clean up code to Mark's original patch.

Archit Taneja (3):
  drm/msm/dsi: Fix regulator API abuse
  drm/msm/edp: Drop regulator_set_voltage call
  drm/msm/mdp4: Don't manage DSI PLL regulators in MDP driver

 drivers/gpu/drm/msm/dsi/dsi.h                   |  2 --
 drivers/gpu/drm/msm/dsi/dsi_cfg.c               | 34 ++++++++++++-------------
 drivers/gpu/drm/msm/dsi/dsi_host.c              | 12 ---------
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c           | 13 ----------
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c      |  4 +--
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c      |  4 +--
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c |  2 +-
 drivers/gpu/drm/msm/edp/edp_ctrl.c              | 10 +-------
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c         | 34 -------------------------
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h         |  2 --
 10 files changed, 23 insertions(+), 94 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 1/3] drm/msm/dsi: Fix regulator API abuse

[Archit Taneja]

The voltage changing code in this driver is broken and should be
removed.  The driver sets a single, exact voltage on probe.  Unless
there is a very good reason for this (which should be documented in
comments) constraints like this need to be set via the machine
constraints, voltage setting in a driver is expected to be used in cases
where the voltage varies at runtime.

In addition client drivers should almost never be calling
regulator_can_set_voltage(), if the device needs to set a voltage it
needs to set the voltage and the regulator core will handle the case
where the regulator is fixed voltage.  If the driver simply skips
setting the voltage if it doesn't have permission then it should just
not bother in the first place.

Originally authored by Mark Brown <broonie@kernel.org>

Remove the min/max voltage data entries per SoC managed by the driver.
These aren't needed as we don't try to set voltages any more. Mention in
comments the voltages that each regulator expects.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/dsi/dsi.h                   |  2 --
 drivers/gpu/drm/msm/dsi/dsi_cfg.c               | 34 ++++++++++++-------------
 drivers/gpu/drm/msm/dsi/dsi_host.c              | 12 ---------
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c           | 13 ----------
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c      |  4 +--
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c      |  4 +--
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c |  2 +-
 7 files changed, 22 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 749fbb2..03f115f 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -41,8 +41,6 @@ enum msm_dsi_phy_type {
 /* Regulators for DSI devices */
 struct dsi_reg_entry {
 	char name[32];
-	int min_voltage;
-	int max_voltage;
 	int enable_load;
 	int disable_load;
 };
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index e58e9b9..93c1ee0 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -22,9 +22,9 @@ static const struct msm_dsi_config apq8064_dsi_cfg = {
 	.reg_cfg = {
 		.num = 3,
 		.regs = {
-			{"vdda", 1200000, 1200000, 100000, 100},
-			{"avdd", 3000000, 3000000, 110000, 100},
-			{"vddio", 1800000, 1800000, 100000, 100},
+			{"vdda", 100000, 100},	/* 1.2 V */
+			{"avdd", 10000, 100},	/* 3.0 V */
+			{"vddio", 100000, 100},	/* 1.8 V */
 		},
 	},
 	.bus_clk_names = dsi_v2_bus_clk_names,
@@ -40,10 +40,10 @@ static const struct msm_dsi_config msm8974_apq8084_dsi_cfg = {
 	.reg_cfg = {
 		.num = 4,
 		.regs = {
-			{"gdsc", -1, -1, -1, -1},
-			{"vdd", 3000000, 3000000, 150000, 100},
-			{"vdda", 1200000, 1200000, 100000, 100},
-			{"vddio", 1800000, 1800000, 100000, 100},
+			{"gdsc", -1, -1},
+			{"vdd", 150000, 100},	/* 3.0 V */
+			{"vdda", 100000, 100},	/* 1.2 V */
+			{"vddio", 100000, 100},	/* 1.8 V */
 		},
 	},
 	.bus_clk_names = dsi_6g_bus_clk_names,
@@ -59,9 +59,9 @@ static const struct msm_dsi_config msm8916_dsi_cfg = {
 	.reg_cfg = {
 		.num = 3,
 		.regs = {
-			{"gdsc", -1, -1, -1, -1},
-			{"vdda", 1200000, 1200000, 100000, 100},
-			{"vddio", 1800000, 1800000, 100000, 100},
+			{"gdsc", -1, -1},
+			{"vdda", 100000, 100},	/* 1.2 V */
+			{"vddio", 100000, 100},	/* 1.8 V */
 		},
 	},
 	.bus_clk_names = dsi_8916_bus_clk_names,
@@ -73,13 +73,13 @@ static const struct msm_dsi_config msm8994_dsi_cfg = {
 	.reg_cfg = {
 		.num = 7,
 		.regs = {
-			{"gdsc", -1, -1, -1, -1},
-			{"vdda", 1250000, 1250000, 100000, 100},
-			{"vddio", 1800000, 1800000, 100000, 100},
-			{"vcca", 1000000, 1000000, 10000, 100},
-			{"vdd", 1800000, 1800000, 100000, 100},
-			{"lab_reg", -1, -1, -1, -1},
-			{"ibb_reg", -1, -1, -1, -1},
+			{"gdsc", -1, -1},
+			{"vdda", 100000, 100},	/* 1.25 V */
+			{"vddio", 100000, 100},	/* 1.8 V */
+			{"vcca", 10000, 100},	/* 1.0 V */
+			{"vdd", 100000, 100},	/* 1.8 V */
+			{"lab_reg", -1, -1},
+			{"ibb_reg", -1, -1},
 		},
 	},
 	.bus_clk_names = dsi_6g_bus_clk_names,
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 4282ec6..a3e47ad8 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -325,18 +325,6 @@ static int dsi_regulator_init(struct msm_dsi_host *msm_host)
 		return ret;
 	}
 
-	for (i = 0; i < num; i++) {
-		if (regulator_can_change_voltage(s[i].consumer)) {
-			ret = regulator_set_voltage(s[i].consumer,
-				regs[i].min_voltage, regs[i].max_voltage);
-			if (ret < 0) {
-				pr_err("regulator %d set voltage failed, %d\n",
-					i, ret);
-				return ret;
-			}
-		}
-	}
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 91a95fb..e2f42d8 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -177,19 +177,6 @@ static int dsi_phy_regulator_init(struct msm_dsi_phy *phy)
 		return ret;
 	}
 
-	for (i = 0; i < num; i++) {
-		if (regulator_can_change_voltage(s[i].consumer)) {
-			ret = regulator_set_voltage(s[i].consumer,
-				regs[i].min_voltage, regs[i].max_voltage);
-			if (ret < 0) {
-				dev_err(dev,
-					"regulator %d set voltage failed, %d\n",
-					i, ret);
-				return ret;
-			}
-		}
-	}
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
index 2e9ba11..f4bc11a 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
@@ -138,8 +138,8 @@ const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs = {
 	.reg_cfg = {
 		.num = 2,
 		.regs = {
-			{"vddio", 1800000, 1800000, 100000, 100},
-			{"vcca", 1000000, 1000000, 10000, 100},
+			{"vddio", 100000, 100},	/* 1.8 V */
+			{"vcca", 10000, 100},	/* 1.0 V */
 		},
 	},
 	.ops = {
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
index edf7411..96d1852 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
@@ -138,7 +138,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vddio", 1800000, 1800000, 100000, 100},
+			{"vddio", 100000, 100},
 		},
 	},
 	.ops = {
@@ -153,7 +153,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_lp_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vddio", 1800000, 1800000, 100000, 100},
+			{"vddio", 100000, 100},	/* 1.8 V */
 		},
 	},
 	.ops = {
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
index 197b039..213355a 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
@@ -185,7 +185,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_8960_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vddio", 1800000, 1800000, 100000, 100},
+			{"vddio", 100000, 100},	/* 1.8 V */
 		},
 	},
 	.ops = {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 2/3] drm/msm/edp: Drop regulator_set_voltage call

[Archit Taneja]

The eDP driver tries to set a fixed voltage for one of its regulators(vdda)
before enabling it. This shouldn't be done by the driver, the voltage
constraints should be specified on the regulator via DT and managed by
the regulator core. A driver should call regulator_set_voltage only if
it needs to change the voltage during runtime. Drop the
regulator_set_voltage call. Mention in a comment the voltage that the
regulator expects.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/edp/edp_ctrl.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/edp/edp_ctrl.c b/drivers/gpu/drm/msm/edp/edp_ctrl.c
index 81200e9..b0c02ee 100644
--- a/drivers/gpu/drm/msm/edp/edp_ctrl.c
+++ b/drivers/gpu/drm/msm/edp/edp_ctrl.c
@@ -21,8 +21,6 @@
 #include "edp.h"
 #include "edp.xml.h"
 
-#define VDDA_MIN_UV		1800000	/* uV units */
-#define VDDA_MAX_UV		1800000	/* uV units */
 #define VDDA_UA_ON_LOAD		100000	/* uA units */
 #define VDDA_UA_OFF_LOAD	100	/* uA units */
 
@@ -67,7 +65,7 @@ struct edp_ctrl {
 	void __iomem *base;
 
 	/* regulators */
-	struct regulator *vdda_vreg;
+	struct regulator *vdda_vreg;	/* 1.8 V */
 	struct regulator *lvl_vreg;
 
 	/* clocks */
@@ -326,12 +324,6 @@ static int edp_regulator_enable(struct edp_ctrl *ctrl)
 {
 	int ret;
 
-	ret = regulator_set_voltage(ctrl->vdda_vreg, VDDA_MIN_UV, VDDA_MAX_UV);
-	if (ret) {
-		pr_err("%s:vdda_vreg set_voltage failed, %d\n", __func__, ret);
-		goto vdda_set_fail;
-	}
-
 	ret = regulator_set_load(ctrl->vdda_vreg, VDDA_UA_ON_LOAD);
 	if (ret < 0) {
 		pr_err("%s: vdda_vreg set regulator mode failed.\n", __func__);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/msm/mdp4: Don't manage DSI PLL regulators in MDP driver

[Archit Taneja]

The MDP4 driver tries to request and set voltages for regulators required
by the DSI PLLs.

Firstly, the MDP4 driver shouldn't manage the DSI regulators, this should
be handled in the DSI driver. Secondly, it shouldn't try to set a fixed
voltage for regulators. Voltage constraints should be specified on the
regulator via DT and managed by the regulator core.

Remove all the DSI PLL regulator related code from the MDP4 driver. It's
managed in the DSI driver for MSM8960/APQ8064 already.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 34 ---------------------------------
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h |  2 --
 2 files changed, 36 deletions(-)

diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
index 76e1dfb..67442d5 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
@@ -50,30 +50,6 @@ static int mdp4_hw_init(struct msm_kms *kms)
 
 	mdp4_kms->rev = minor;
 
-	if (mdp4_kms->dsi_pll_vdda) {
-		if ((mdp4_kms->rev == 2) || (mdp4_kms->rev == 4)) {
-			ret = regulator_set_voltage(mdp4_kms->dsi_pll_vdda,
-					1200000, 1200000);
-			if (ret) {
-				dev_err(dev->dev,
-					"failed to set dsi_pll_vdda voltage: %d\n", ret);
-				goto out;
-			}
-		}
-	}
-
-	if (mdp4_kms->dsi_pll_vddio) {
-		if (mdp4_kms->rev == 2) {
-			ret = regulator_set_voltage(mdp4_kms->dsi_pll_vddio,
-					1800000, 1800000);
-			if (ret) {
-				dev_err(dev->dev,
-					"failed to set dsi_pll_vddio voltage: %d\n", ret);
-				goto out;
-			}
-		}
-	}
-
 	if (mdp4_kms->rev > 1) {
 		mdp4_write(mdp4_kms, REG_MDP4_CS_CONTROLLER0, 0x0707ffff);
 		mdp4_write(mdp4_kms, REG_MDP4_CS_CONTROLLER1, 0x03073f3f);
@@ -485,16 +461,6 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
 		goto fail;
 	}
 
-	mdp4_kms->dsi_pll_vdda =
-			devm_regulator_get_optional(&pdev->dev, "dsi_pll_vdda");
-	if (IS_ERR(mdp4_kms->dsi_pll_vdda))
-		mdp4_kms->dsi_pll_vdda = NULL;
-
-	mdp4_kms->dsi_pll_vddio =
-			devm_regulator_get_optional(&pdev->dev, "dsi_pll_vddio");
-	if (IS_ERR(mdp4_kms->dsi_pll_vddio))
-		mdp4_kms->dsi_pll_vddio = NULL;
-
 	/* NOTE: driver for this regulator still missing upstream.. use
 	 * _get_exclusive() and ignore the error if it does not exist
 	 * (and hope that the bootloader left it on for us)
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h
index b282871..c5d045d 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h
@@ -37,8 +37,6 @@ struct mdp4_kms {
 
 	void __iomem *mmio;
 
-	struct regulator *dsi_pll_vdda;
-	struct regulator *dsi_pll_vddio;
 	struct regulator *vdd;
 
 	struct clk *clk;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation