All of lore.kernel.org
 help / color / mirror / Atom feed
From: Archit Taneja <architt@codeaurora.org>
To: Mark Brown <broonie@kernel.org>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/msm/dsi: Fix regulator API abuse
Date: Tue, 5 Apr 2016 16:56:02 +0530	[thread overview]
Message-ID: <5703A0CA.8020802@codeaurora.org> (raw)
In-Reply-To: <1459364022-28484-1-git-send-email-broonie@kernel.org>

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

  reply	other threads:[~2016-04-05 11:26 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-30 18:53 [PATCH] drm/msm/dsi: Fix regulator API abuse Mark Brown
2016-04-05 11:26 ` Archit Taneja [this message]
2016-04-05 16:17   ` Mark Brown
2016-04-29  9:49 ` [PATCH 0/3] drm/msm: Remove regulator_set_voltage calls Archit Taneja
2016-04-29  9:49   ` [PATCH 1/3] drm/msm/dsi: Fix regulator API abuse Archit Taneja
2016-04-29  9:49   ` [PATCH 2/3] drm/msm/edp: Drop regulator_set_voltage call Archit Taneja
2016-04-29  9:49   ` [PATCH 3/3] drm/msm/mdp4: Don't manage DSI PLL regulators in MDP driver Archit Taneja

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5703A0CA.8020802@codeaurora.org \
    --to=architt@codeaurora.org \
    --cc=broonie@kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.