[PATCH v3 0/6] drm/msm/dsi regulator improvements

[PATCH v3 0/6] drm/msm/dsi regulator improvements

[Douglas Anderson]

The main goal of this series is to make a small dent in cleaning up
the way we deal with regulator loads for DSI drivers.

As of v3 of this series, the regulator API improvements needed for the
later patches in the series are merged into mainline. Thus this series
only contains the DSI changes now.

I'd expect:
* The first two patches are bugfixes found while converting the DSI
  driver over. Those could land any time.
* The third patch ("drm/msm/dsi: Don't set a load before disabling a
  regulator") is a patch a sent the other day verbatim, included in
  this series because it's highly related. It could land any
  time.
* The next two patches use the new APIs. Since those APIs are now in
  mainline those could also land any time.
* The last patch is just cleanup I noticed as I was touching the
  function. It's not really related to regulators but it applies atop
  these. In theory it could be rebased to land separately.

Changes in v3:
- ("Improve dsi_phy_driver_probe() probe error handling") new for v3.
- Do all the PHYs too.
- Fix typo in commit message.
- Get rid of error print after devm_regulator_bulk_get_const().
- Just directly call the bulk commands; get rid of the wrapper.
- Update commit message to point at the git hash of the regulator change.

Changes in v2:
- ("Fix number of regulators for SDM660") new for v2.
- ("Fix number of regulators for msm8996_dsi_cfg") new for v2.
- ("Take advantage of devm_regulator_bulk_get_const") new for v2.
- ("Use the new regulator bulk feature to specify the load") new for v2.

Douglas Anderson (6):
  drm/msm/dsi: Fix number of regulators for msm8996_dsi_cfg
  drm/msm/dsi: Fix number of regulators for SDM660
  drm/msm/dsi: Don't set a load before disabling a regulator
  drm/msm/dsi: Use the new regulator bulk feature to specify the load
  drm/msm/dsi: Take advantage of devm_regulator_bulk_get_const()
  drm/msm/dsi: Improve dsi_phy_driver_probe() probe error handling

 drivers/gpu/drm/msm/dsi/dsi.h                 |  13 --
 drivers/gpu/drm/msm/dsi/dsi_cfg.c             | 172 +++++++++---------
 drivers/gpu/drm/msm/dsi/dsi_cfg.h             |   3 +-
 drivers/gpu/drm/msm/dsi/dsi_host.c            |  96 ++--------
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c         | 160 ++++------------
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h         |   5 +-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c    |  20 +-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c    |  32 ++--
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c    |  14 +-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c    |  28 +--
 .../gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c   |  12 +-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c     |  32 ++--
 12 files changed, 197 insertions(+), 390 deletions(-)

-- 
2.37.1.455.g008518b4e5-goog

[PATCH v3 1/6] drm/msm/dsi: Fix number of regulators for msm8996_dsi_cfg

[Douglas Anderson]

3 regulators are specified listed but the number 2 is specified. Fix
it.

Fixes: 3a3ff88a0fc1 ("drm/msm/dsi: Add 8x96 info in dsi_cfg")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

(no changes since v2)

Changes in v2:
- ("Fix number of regulators for msm8996_dsi_cfg") new for v2.

 drivers/gpu/drm/msm/dsi/dsi_cfg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index 2c23324a2296..02000a7b7a18 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -109,7 +109,7 @@ static const char * const dsi_8996_bus_clk_names[] = {
 static const struct msm_dsi_config msm8996_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
 	.reg_cfg = {
-		.num = 2,
+		.num = 3,
 		.regs = {
 			{"vdda", 18160, 1 },	/* 1.25 V */
 			{"vcca", 17000, 32 },	/* 0.925 V */
-- 
2.37.1.455.g008518b4e5-goog

Re: [PATCH v3 1/6] drm/msm/dsi: Fix number of regulators for msm8996_dsi_cfg

[Dmitry Baryshkov]

On 03/08/2022 01:37, Douglas Anderson wrote:
> 3 regulators are specified listed but the number 2 is specified. Fix
> it.
> 
> Fixes: 3a3ff88a0fc1 ("drm/msm/dsi: Add 8x96 info in dsi_cfg")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
> 
> (no changes since v2)
> 
> Changes in v2:
> - ("Fix number of regulators for msm8996_dsi_cfg") new for v2.
> 
>   drivers/gpu/drm/msm/dsi/dsi_cfg.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index 2c23324a2296..02000a7b7a18 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -109,7 +109,7 @@ static const char * const dsi_8996_bus_clk_names[] = {
>   static const struct msm_dsi_config msm8996_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
>   	.reg_cfg = {
> -		.num = 2,
> +		.num = 3,
>   		.regs = {
>   			{"vdda", 18160, 1 },	/* 1.25 V */
>   			{"vcca", 17000, 32 },	/* 0.925 V */


-- 
With best wishes
Dmitry

Re: [PATCH v3 1/6] drm/msm/dsi: Fix number of regulators for msm8996_dsi_cfg

[Abhinav Kumar]

On 8/2/2022 3:37 PM, Douglas Anderson wrote:
> 3 regulators are specified listed but the number 2 is specified. Fix
> it.
> 
> Fixes: 3a3ff88a0fc1 ("drm/msm/dsi: Add 8x96 info in dsi_cfg")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Will queue this for -fixes.

> ---
> 
> (no changes since v2)
> 
> Changes in v2:
> - ("Fix number of regulators for msm8996_dsi_cfg") new for v2.
> 
>   drivers/gpu/drm/msm/dsi/dsi_cfg.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index 2c23324a2296..02000a7b7a18 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -109,7 +109,7 @@ static const char * const dsi_8996_bus_clk_names[] = {
>   static const struct msm_dsi_config msm8996_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
>   	.reg_cfg = {
> -		.num = 2,
> +		.num = 3,
>   		.regs = {
>   			{"vdda", 18160, 1 },	/* 1.25 V */
>   			{"vcca", 17000, 32 },	/* 0.925 V */

[PATCH v3 2/6] drm/msm/dsi: Fix number of regulators for SDM660

[Douglas Anderson]

1 regulators is specified listed but the number 2 is specified. This
presumably means we try to get a regulator with no name. Fix it.

Fixes: 033f47f7f121 ("drm/msm/dsi: Add DSI configuration for SDM660")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

(no changes since v2)

Changes in v2:
- ("Fix number of regulators for SDM660") new for v2.

 drivers/gpu/drm/msm/dsi/dsi_cfg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index 02000a7b7a18..72c018e26f47 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -148,7 +148,7 @@ static const char * const dsi_sdm660_bus_clk_names[] = {
 static const struct msm_dsi_config sdm660_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
 	.reg_cfg = {
-		.num = 2,
+		.num = 1,
 		.regs = {
 			{"vdda", 12560, 4 },	/* 1.2 V */
 		},
-- 
2.37.1.455.g008518b4e5-goog

Re: [PATCH v3 2/6] drm/msm/dsi: Fix number of regulators for SDM660

[Dmitry Baryshkov]

On 03/08/2022 01:37, Douglas Anderson wrote:
> 1 regulators is specified listed but the number 2 is specified. This
> presumably means we try to get a regulator with no name. Fix it.
> 
> Fixes: 033f47f7f121 ("drm/msm/dsi: Add DSI configuration for SDM660")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
> 
> (no changes since v2)
> 
> Changes in v2:
> - ("Fix number of regulators for SDM660") new for v2.
> 
>   drivers/gpu/drm/msm/dsi/dsi_cfg.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index 02000a7b7a18..72c018e26f47 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -148,7 +148,7 @@ static const char * const dsi_sdm660_bus_clk_names[] = {
>   static const struct msm_dsi_config sdm660_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
>   	.reg_cfg = {
> -		.num = 2,
> +		.num = 1,
>   		.regs = {
>   			{"vdda", 12560, 4 },	/* 1.2 V */

We should find some time and switch the reg_cfg.regs to be the external 
array, so that we can use ARRAY_SIZE for the amount of regs. However it 
is definitely not a showstopper for this series.



-- 
With best wishes
Dmitry

Re: [PATCH v3 2/6] drm/msm/dsi: Fix number of regulators for SDM660

[Marijn Suijten]

On 2022-08-02 15:37:34, Douglas Anderson wrote:
> 1 regulators is specified listed but the number 2 is specified. This
> presumably means we try to get a regulator with no name. Fix it.
> 
> Fixes: 033f47f7f121 ("drm/msm/dsi: Add DSI configuration for SDM660")

This should be:

Fixes: 462f7017a691 ("drm/msm/dsi: Fix DSI and DSI PHY regulator config from SDM660")

The original patch included two regulators; that "fix" patch removed
"vdd".

After that:

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> (no changes since v2)
> 
> Changes in v2:
> - ("Fix number of regulators for SDM660") new for v2.
> 
>  drivers/gpu/drm/msm/dsi/dsi_cfg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index 02000a7b7a18..72c018e26f47 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -148,7 +148,7 @@ static const char * const dsi_sdm660_bus_clk_names[] = {
>  static const struct msm_dsi_config sdm660_dsi_cfg = {
>  	.io_offset = DSI_6G_REG_SHIFT,
>  	.reg_cfg = {
> -		.num = 2,
> +		.num = 1,
>  		.regs = {
>  			{"vdda", 12560, 4 },	/* 1.2 V */
>  		},
> -- 
> 2.37.1.455.g008518b4e5-goog
> 

Re: [PATCH v3 2/6] drm/msm/dsi: Fix number of regulators for SDM660

[Abhinav Kumar]

On 8/2/2022 3:37 PM, Douglas Anderson wrote:
> 1 regulators is specified listed but the number 2 is specified. This
> presumably means we try to get a regulator with no name. Fix it.
> 
> Fixes: 033f47f7f121 ("drm/msm/dsi: Add DSI configuration for SDM660")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Agree with Marijn's comment here:

https://patchwork.freedesktop.org/patch/496153/?series=106731&rev=2#comment_894595

Will queue for -fixes with the fixes tag he has given

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> 
> (no changes since v2)
> 
> Changes in v2:
> - ("Fix number of regulators for SDM660") new for v2.
> 
>   drivers/gpu/drm/msm/dsi/dsi_cfg.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index 02000a7b7a18..72c018e26f47 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -148,7 +148,7 @@ static const char * const dsi_sdm660_bus_clk_names[] = {
>   static const struct msm_dsi_config sdm660_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
>   	.reg_cfg = {
> -		.num = 2,
> +		.num = 1,
>   		.regs = {
>   			{"vdda", 12560, 4 },	/* 1.2 V */
>   		},

[PATCH v3 3/6] drm/msm/dsi: Don't set a load before disabling a regulator

[Douglas Anderson]

As of commit 5451781dadf8 ("regulator: core: Only count load for
enabled consumers"), a load isn't counted for a disabled
regulator. That means all the code in the DSI driver to specify and
set loads before disabling a regulator is not actually doing anything
useful. Let's remove it.

It should be noted that all of the loads set that were being specified
were pointless noise anyway. The only use for this number is to pick
between low power and high power modes of regulators. Regulators
appear to do this changeover at loads on the order of 10000 uA. You
would need a lot of clients of the same rail for that 100 uA number to
count for anything.

Note that now that we get rid of the setting of the load at disable
time, we can just set the load once when we first get the regulator
and then forget it.

It should also be noted that the regulator functions
regulator_bulk_enable() and regulator_set_load() already print error
messages when they encounter problems so while moving things around we
get rid of some extra error prints.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- Fix typo in commit message.
- Just directly call the bulk commands; get rid of the wrapper.

 drivers/gpu/drm/msm/dsi/dsi.h                 |  1 -
 drivers/gpu/drm/msm/dsi/dsi_cfg.c             | 52 +++++++-------
 drivers/gpu/drm/msm/dsi/dsi_host.c            | 71 ++++---------------
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c         | 52 ++------------
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c    |  4 +-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c    |  6 +-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c    |  4 +-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c    |  6 +-
 .../gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c   |  2 +-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c     |  6 +-
 10 files changed, 60 insertions(+), 144 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 580a1e6358bf..bb6a5bd05cb1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -37,7 +37,6 @@ enum msm_dsi_phy_usecase {
 struct dsi_reg_entry {
 	char name[32];
 	int enable_load;
-	int disable_load;
 };
 
 struct dsi_reg_config {
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index 72c018e26f47..901d6fd53800 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -14,9 +14,9 @@ static const struct msm_dsi_config apq8064_dsi_cfg = {
 	.reg_cfg = {
 		.num = 3,
 		.regs = {
-			{"vdda", 100000, 100},	/* 1.2 V */
-			{"avdd", 10000, 100},	/* 3.0 V */
-			{"vddio", 100000, 100},	/* 1.8 V */
+			{"vdda", 100000},	/* 1.2 V */
+			{"avdd", 10000},	/* 3.0 V */
+			{"vddio", 100000},	/* 1.8 V */
 		},
 	},
 	.bus_clk_names = dsi_v2_bus_clk_names,
@@ -34,9 +34,9 @@ static const struct msm_dsi_config msm8974_apq8084_dsi_cfg = {
 	.reg_cfg = {
 		.num = 3,
 		.regs = {
-			{"vdd", 150000, 100},	/* 3.0 V */
-			{"vdda", 100000, 100},	/* 1.2 V */
-			{"vddio", 100000, 100},	/* 1.8 V */
+			{"vdd", 150000},	/* 3.0 V */
+			{"vdda", 100000},	/* 1.2 V */
+			{"vddio", 100000},	/* 1.8 V */
 		},
 	},
 	.bus_clk_names = dsi_6g_bus_clk_names,
@@ -54,8 +54,8 @@ static const struct msm_dsi_config msm8916_dsi_cfg = {
 	.reg_cfg = {
 		.num = 2,
 		.regs = {
-			{"vdda", 100000, 100},	/* 1.2 V */
-			{"vddio", 100000, 100},	/* 1.8 V */
+			{"vdda", 100000},	/* 1.2 V */
+			{"vddio", 100000},	/* 1.8 V */
 		},
 	},
 	.bus_clk_names = dsi_8916_bus_clk_names,
@@ -73,8 +73,8 @@ static const struct msm_dsi_config msm8976_dsi_cfg = {
 	.reg_cfg = {
 		.num = 2,
 		.regs = {
-			{"vdda", 100000, 100},	/* 1.2 V */
-			{"vddio", 100000, 100},	/* 1.8 V */
+			{"vdda", 100000},	/* 1.2 V */
+			{"vddio", 100000},	/* 1.8 V */
 		},
 	},
 	.bus_clk_names = dsi_8976_bus_clk_names,
@@ -88,12 +88,12 @@ static const struct msm_dsi_config msm8994_dsi_cfg = {
 	.reg_cfg = {
 		.num = 6,
 		.regs = {
-			{"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},
+			{"vdda", 100000},	/* 1.25 V */
+			{"vddio", 100000},	/* 1.8 V */
+			{"vcca", 10000},	/* 1.0 V */
+			{"vdd", 100000},	/* 1.8 V */
+			{"lab_reg", -1},
+			{"ibb_reg", -1},
 		},
 	},
 	.bus_clk_names = dsi_6g_bus_clk_names,
@@ -111,9 +111,9 @@ static const struct msm_dsi_config msm8996_dsi_cfg = {
 	.reg_cfg = {
 		.num = 3,
 		.regs = {
-			{"vdda", 18160, 1 },	/* 1.25 V */
-			{"vcca", 17000, 32 },	/* 0.925 V */
-			{"vddio", 100000, 100 },/* 1.8 V */
+			{"vdda", 18160},	/* 1.25 V */
+			{"vcca", 17000},	/* 0.925 V */
+			{"vddio", 100000},/* 1.8 V */
 		},
 	},
 	.bus_clk_names = dsi_8996_bus_clk_names,
@@ -131,8 +131,8 @@ static const struct msm_dsi_config msm8998_dsi_cfg = {
 	.reg_cfg = {
 		.num = 2,
 		.regs = {
-			{"vdd", 367000, 16 },	/* 0.9 V */
-			{"vdda", 62800, 2 },	/* 1.2 V */
+			{"vdd", 367000},	/* 0.9 V */
+			{"vdda", 62800},	/* 1.2 V */
 		},
 	},
 	.bus_clk_names = dsi_msm8998_bus_clk_names,
@@ -150,7 +150,7 @@ static const struct msm_dsi_config sdm660_dsi_cfg = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vdda", 12560, 4 },	/* 1.2 V */
+			{"vdda", 12560},	/* 1.2 V */
 		},
 	},
 	.bus_clk_names = dsi_sdm660_bus_clk_names,
@@ -172,7 +172,7 @@ static const struct msm_dsi_config sdm845_dsi_cfg = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vdda", 21800, 4 },	/* 1.2 V */
+			{"vdda", 21800},	/* 1.2 V */
 		},
 	},
 	.bus_clk_names = dsi_sdm845_bus_clk_names,
@@ -186,7 +186,7 @@ static const struct msm_dsi_config sc7180_dsi_cfg = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vdda", 21800, 4 },	/* 1.2 V */
+			{"vdda", 21800},	/* 1.2 V */
 		},
 	},
 	.bus_clk_names = dsi_sc7180_bus_clk_names,
@@ -204,7 +204,7 @@ static const struct msm_dsi_config sc7280_dsi_cfg = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vdda", 8350, 0 },	/* 1.2 V */
+			{"vdda", 8350},	/* 1.2 V */
 		},
 	},
 	.bus_clk_names = dsi_sc7280_bus_clk_names,
@@ -222,7 +222,7 @@ static const struct msm_dsi_config qcm2290_dsi_cfg = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vdda", 21800, 4 },	/* 1.2 V */
+			{"vdda", 21800},	/* 1.2 V */
 		},
 	},
 	.bus_clk_names = dsi_qcm2290_bus_clk_names,
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index a95d5df52653..9df278d39559 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -203,9 +203,6 @@ static inline void dsi_write(struct msm_dsi_host *msm_host, u32 reg, u32 data)
 	msm_writel(data, msm_host->ctrl_base + reg);
 }
 
-static int dsi_host_regulator_enable(struct msm_dsi_host *msm_host);
-static void dsi_host_regulator_disable(struct msm_dsi_host *msm_host);
-
 static const struct msm_dsi_cfg_handler *dsi_get_config(
 						struct msm_dsi_host *msm_host)
 {
@@ -256,56 +253,6 @@ static inline struct msm_dsi_host *to_msm_dsi_host(struct mipi_dsi_host *host)
 	return container_of(host, struct msm_dsi_host, base);
 }
 
-static void dsi_host_regulator_disable(struct msm_dsi_host *msm_host)
-{
-	struct regulator_bulk_data *s = msm_host->supplies;
-	const struct dsi_reg_entry *regs = msm_host->cfg_hnd->cfg->reg_cfg.regs;
-	int num = msm_host->cfg_hnd->cfg->reg_cfg.num;
-	int i;
-
-	DBG("");
-	for (i = num - 1; i >= 0; i--)
-		if (regs[i].disable_load >= 0)
-			regulator_set_load(s[i].consumer,
-					   regs[i].disable_load);
-
-	regulator_bulk_disable(num, s);
-}
-
-static int dsi_host_regulator_enable(struct msm_dsi_host *msm_host)
-{
-	struct regulator_bulk_data *s = msm_host->supplies;
-	const struct dsi_reg_entry *regs = msm_host->cfg_hnd->cfg->reg_cfg.regs;
-	int num = msm_host->cfg_hnd->cfg->reg_cfg.num;
-	int ret, i;
-
-	DBG("");
-	for (i = 0; i < num; i++) {
-		if (regs[i].enable_load >= 0) {
-			ret = regulator_set_load(s[i].consumer,
-						 regs[i].enable_load);
-			if (ret < 0) {
-				pr_err("regulator %d set op mode failed, %d\n",
-					i, ret);
-				goto fail;
-			}
-		}
-	}
-
-	ret = regulator_bulk_enable(num, s);
-	if (ret < 0) {
-		pr_err("regulator enable failed, %d\n", ret);
-		goto fail;
-	}
-
-	return 0;
-
-fail:
-	for (i--; i >= 0; i--)
-		regulator_set_load(s[i].consumer, regs[i].disable_load);
-	return ret;
-}
-
 static int dsi_regulator_init(struct msm_dsi_host *msm_host)
 {
 	struct regulator_bulk_data *s = msm_host->supplies;
@@ -323,6 +270,15 @@ static int dsi_regulator_init(struct msm_dsi_host *msm_host)
 		return ret;
 	}
 
+	for (i = 0; i < num; i++) {
+		if (regs[i].enable_load >= 0) {
+			ret = regulator_set_load(s[i].consumer,
+						 regs[i].enable_load);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -2561,7 +2517,8 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
 
 	msm_dsi_sfpb_config(msm_host, true);
 
-	ret = dsi_host_regulator_enable(msm_host);
+	ret = regulator_bulk_enable(msm_host->cfg_hnd->cfg->reg_cfg.num,
+				    msm_host->supplies);
 	if (ret) {
 		pr_err("%s:Failed to enable vregs.ret=%d\n",
 			__func__, ret);
@@ -2601,7 +2558,8 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
 	cfg_hnd->ops->link_clk_disable(msm_host);
 	pm_runtime_put(&msm_host->pdev->dev);
 fail_disable_reg:
-	dsi_host_regulator_disable(msm_host);
+	regulator_bulk_disable(msm_host->cfg_hnd->cfg->reg_cfg.num,
+			       msm_host->supplies);
 unlock_ret:
 	mutex_unlock(&msm_host->dev_mutex);
 	return ret;
@@ -2628,7 +2586,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host)
 	cfg_hnd->ops->link_clk_disable(msm_host);
 	pm_runtime_put(&msm_host->pdev->dev);
 
-	dsi_host_regulator_disable(msm_host);
+	regulator_bulk_disable(msm_host->cfg_hnd->cfg->reg_cfg.num,
+			       msm_host->supplies);
 
 	msm_dsi_sfpb_config(msm_host, false);
 
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index a39de3bdc7fa..7c105120d73e 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -529,58 +529,16 @@ static int dsi_phy_regulator_init(struct msm_dsi_phy *phy)
 		return ret;
 	}
 
-	return 0;
-}
-
-static void dsi_phy_regulator_disable(struct msm_dsi_phy *phy)
-{
-	struct regulator_bulk_data *s = phy->supplies;
-	const struct dsi_reg_entry *regs = phy->cfg->reg_cfg.regs;
-	int num = phy->cfg->reg_cfg.num;
-	int i;
-
-	DBG("");
-	for (i = num - 1; i >= 0; i--)
-		if (regs[i].disable_load >= 0)
-			regulator_set_load(s[i].consumer, regs[i].disable_load);
-
-	regulator_bulk_disable(num, s);
-}
-
-static int dsi_phy_regulator_enable(struct msm_dsi_phy *phy)
-{
-	struct regulator_bulk_data *s = phy->supplies;
-	const struct dsi_reg_entry *regs = phy->cfg->reg_cfg.regs;
-	struct device *dev = &phy->pdev->dev;
-	int num = phy->cfg->reg_cfg.num;
-	int ret, i;
-
-	DBG("");
 	for (i = 0; i < num; i++) {
 		if (regs[i].enable_load >= 0) {
 			ret = regulator_set_load(s[i].consumer,
 							regs[i].enable_load);
-			if (ret < 0) {
-				DRM_DEV_ERROR(dev,
-					"regulator %d set op mode failed, %d\n",
-					i, ret);
-				goto fail;
-			}
+			if (ret < 0)
+				return ret;
 		}
 	}
 
-	ret = regulator_bulk_enable(num, s);
-	if (ret < 0) {
-		DRM_DEV_ERROR(dev, "regulator enable failed, %d\n", ret);
-		goto fail;
-	}
-
 	return 0;
-
-fail:
-	for (i--; i >= 0; i--)
-		regulator_set_load(s[i].consumer, regs[i].disable_load);
-	return ret;
 }
 
 static int dsi_phy_enable_resource(struct msm_dsi_phy *phy)
@@ -829,7 +787,7 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
 		goto res_en_fail;
 	}
 
-	ret = dsi_phy_regulator_enable(phy);
+	ret = regulator_bulk_enable(phy->cfg->reg_cfg.num, phy->supplies);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "%s: regulator enable failed, %d\n",
 			__func__, ret);
@@ -866,7 +824,7 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
 	if (phy->cfg->ops.disable)
 		phy->cfg->ops.disable(phy);
 phy_en_fail:
-	dsi_phy_regulator_disable(phy);
+	regulator_bulk_disable(phy->cfg->reg_cfg.num, phy->supplies);
 reg_en_fail:
 	dsi_phy_disable_resource(phy);
 res_en_fail:
@@ -880,7 +838,7 @@ void msm_dsi_phy_disable(struct msm_dsi_phy *phy)
 
 	phy->cfg->ops.disable(phy);
 
-	dsi_phy_regulator_disable(phy);
+	regulator_bulk_disable(phy->cfg->reg_cfg.num, phy->supplies);
 	dsi_phy_disable_resource(phy);
 }
 
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
index 08b015ea1b1e..6a10a1448051 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
@@ -1033,7 +1033,7 @@ const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vdds", 36000, 32},
+			{"vdds", 36000},
 		},
 	},
 	.ops = {
@@ -1055,7 +1055,7 @@ const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vdds", 36000, 32},
+			{"vdds", 36000},
 		},
 	},
 	.ops = {
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
index 8199c53567f4..0f3d4c56c333 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
@@ -1029,7 +1029,7 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vcca", 17000, 32},
+			{"vcca", 17000},
 		},
 	},
 	.ops = {
@@ -1050,7 +1050,7 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vcca", 73400, 32},
+			{"vcca", 73400},
 		},
 	},
 	.ops = {
@@ -1071,7 +1071,7 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_8953_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vcca", 17000, 32},
+			{"vcca", 17000},
 		},
 	},
 	.ops = {
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 ee7c418a1c29..b7c621d94981 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
@@ -134,8 +134,8 @@ const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs = {
 	.reg_cfg = {
 		.num = 2,
 		.regs = {
-			{"vddio", 100000, 100},	/* 1.8 V */
-			{"vcca", 10000, 100},	/* 1.0 V */
+			{"vddio", 100000},	/* 1.8 V */
+			{"vcca", 10000},	/* 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 48eab80b548e..6beba387640d 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
@@ -774,7 +774,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vddio", 100000, 100},
+			{"vddio", 100000},
 		},
 	},
 	.ops = {
@@ -795,7 +795,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_famb_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vddio", 100000, 100},
+			{"vddio", 100000},
 		},
 	},
 	.ops = {
@@ -816,7 +816,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_lp_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vddio", 100000, 100},	/* 1.8 V */
+			{"vddio", 100000},	/* 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 fc56cdcc9ad6..2e942b10fffa 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
@@ -653,7 +653,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_8960_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vddio", 100000, 100},	/* 1.8 V */
+			{"vddio", 100000},	/* 1.8 V */
 		},
 	},
 	.ops = {
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
index 66ed1919a1db..9c7c49ce1200 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
@@ -1041,7 +1041,7 @@ const struct msm_dsi_phy_cfg dsi_phy_7nm_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vdds", 36000, 32},
+			{"vdds", 36000},
 		},
 	},
 	.ops = {
@@ -1068,7 +1068,7 @@ const struct msm_dsi_phy_cfg dsi_phy_7nm_8150_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vdds", 36000, 32},
+			{"vdds", 36000},
 		},
 	},
 	.ops = {
@@ -1090,7 +1090,7 @@ const struct msm_dsi_phy_cfg dsi_phy_7nm_7280_cfgs = {
 	.reg_cfg = {
 		.num = 1,
 		.regs = {
-			{"vdds", 37550, 0},
+			{"vdds", 37550},
 		},
 	},
 	.ops = {
-- 
2.37.1.455.g008518b4e5-goog

Re: [PATCH v3 3/6] drm/msm/dsi: Don't set a load before disabling a regulator

[Dmitry Baryshkov]

On 03/08/2022 01:37, Douglas Anderson wrote:
> As of commit 5451781dadf8 ("regulator: core: Only count load for
> enabled consumers"), a load isn't counted for a disabled
> regulator. That means all the code in the DSI driver to specify and
> set loads before disabling a regulator is not actually doing anything
> useful. Let's remove it.
> 
> It should be noted that all of the loads set that were being specified
> were pointless noise anyway. The only use for this number is to pick
> between low power and high power modes of regulators. Regulators
> appear to do this changeover at loads on the order of 10000 uA. You
> would need a lot of clients of the same rail for that 100 uA number to
> count for anything.
> 
> Note that now that we get rid of the setting of the load at disable
> time, we can just set the load once when we first get the regulator
> and then forget it.
> 
> It should also be noted that the regulator functions
> regulator_bulk_enable() and regulator_set_load() already print error
> messages when they encounter problems so while moving things around we
> get rid of some extra error prints.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
> 
> Changes in v3:
> - Fix typo in commit message.
> - Just directly call the bulk commands; get rid of the wrapper.
> 
>   drivers/gpu/drm/msm/dsi/dsi.h                 |  1 -
>   drivers/gpu/drm/msm/dsi/dsi_cfg.c             | 52 +++++++-------
>   drivers/gpu/drm/msm/dsi/dsi_host.c            | 71 ++++---------------
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c         | 52 ++------------
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c    |  4 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c    |  6 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c    |  4 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c    |  6 +-
>   .../gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c   |  2 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c     |  6 +-
>   10 files changed, 60 insertions(+), 144 deletions(-)
> 


-- 
With best wishes
Dmitry

Re: [PATCH v3 3/6] drm/msm/dsi: Don't set a load before disabling a regulator

[Abhinav Kumar]

On 8/2/2022 3:37 PM, Douglas Anderson wrote:
> As of commit 5451781dadf8 ("regulator: core: Only count load for
> enabled consumers"), a load isn't counted for a disabled
> regulator. That means all the code in the DSI driver to specify and
> set loads before disabling a regulator is not actually doing anything
> useful. Let's remove it.
> 
> It should be noted that all of the loads set that were being specified
> were pointless noise anyway. The only use for this number is to pick
> between low power and high power modes of regulators. Regulators
> appear to do this changeover at loads on the order of 10000 uA. You
> would need a lot of clients of the same rail for that 100 uA number to
> count for anything.
> 
> Note that now that we get rid of the setting of the load at disable
> time, we can just set the load once when we first get the regulator
> and then forget it.
> 
> It should also be noted that the regulator functions
> regulator_bulk_enable() and regulator_set_load() already print error
> messages when they encounter problems so while moving things around we
> get rid of some extra error prints.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> 
> Changes in v3:
> - Fix typo in commit message.
> - Just directly call the bulk commands; get rid of the wrapper.
> 
>   drivers/gpu/drm/msm/dsi/dsi.h                 |  1 -
>   drivers/gpu/drm/msm/dsi/dsi_cfg.c             | 52 +++++++-------
>   drivers/gpu/drm/msm/dsi/dsi_host.c            | 71 ++++---------------
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c         | 52 ++------------
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c    |  4 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c    |  6 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c    |  4 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c    |  6 +-
>   .../gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c   |  2 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c     |  6 +-
>   10 files changed, 60 insertions(+), 144 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index 580a1e6358bf..bb6a5bd05cb1 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -37,7 +37,6 @@ enum msm_dsi_phy_usecase {
>   struct dsi_reg_entry {
>   	char name[32];
>   	int enable_load;
> -	int disable_load;
>   };
>   
>   struct dsi_reg_config {
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index 72c018e26f47..901d6fd53800 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -14,9 +14,9 @@ static const struct msm_dsi_config apq8064_dsi_cfg = {
>   	.reg_cfg = {
>   		.num = 3,
>   		.regs = {
> -			{"vdda", 100000, 100},	/* 1.2 V */
> -			{"avdd", 10000, 100},	/* 3.0 V */
> -			{"vddio", 100000, 100},	/* 1.8 V */
> +			{"vdda", 100000},	/* 1.2 V */
> +			{"avdd", 10000},	/* 3.0 V */
> +			{"vddio", 100000},	/* 1.8 V */
>   		},
>   	},
>   	.bus_clk_names = dsi_v2_bus_clk_names,
> @@ -34,9 +34,9 @@ static const struct msm_dsi_config msm8974_apq8084_dsi_cfg = {
>   	.reg_cfg = {
>   		.num = 3,
>   		.regs = {
> -			{"vdd", 150000, 100},	/* 3.0 V */
> -			{"vdda", 100000, 100},	/* 1.2 V */
> -			{"vddio", 100000, 100},	/* 1.8 V */
> +			{"vdd", 150000},	/* 3.0 V */
> +			{"vdda", 100000},	/* 1.2 V */
> +			{"vddio", 100000},	/* 1.8 V */
>   		},
>   	},
>   	.bus_clk_names = dsi_6g_bus_clk_names,
> @@ -54,8 +54,8 @@ static const struct msm_dsi_config msm8916_dsi_cfg = {
>   	.reg_cfg = {
>   		.num = 2,
>   		.regs = {
> -			{"vdda", 100000, 100},	/* 1.2 V */
> -			{"vddio", 100000, 100},	/* 1.8 V */
> +			{"vdda", 100000},	/* 1.2 V */
> +			{"vddio", 100000},	/* 1.8 V */
>   		},
>   	},
>   	.bus_clk_names = dsi_8916_bus_clk_names,
> @@ -73,8 +73,8 @@ static const struct msm_dsi_config msm8976_dsi_cfg = {
>   	.reg_cfg = {
>   		.num = 2,
>   		.regs = {
> -			{"vdda", 100000, 100},	/* 1.2 V */
> -			{"vddio", 100000, 100},	/* 1.8 V */
> +			{"vdda", 100000},	/* 1.2 V */
> +			{"vddio", 100000},	/* 1.8 V */
>   		},
>   	},
>   	.bus_clk_names = dsi_8976_bus_clk_names,
> @@ -88,12 +88,12 @@ static const struct msm_dsi_config msm8994_dsi_cfg = {
>   	.reg_cfg = {
>   		.num = 6,
>   		.regs = {
> -			{"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},
> +			{"vdda", 100000},	/* 1.25 V */
> +			{"vddio", 100000},	/* 1.8 V */
> +			{"vcca", 10000},	/* 1.0 V */
> +			{"vdd", 100000},	/* 1.8 V */
> +			{"lab_reg", -1},
> +			{"ibb_reg", -1},
>   		},
>   	},
>   	.bus_clk_names = dsi_6g_bus_clk_names,
> @@ -111,9 +111,9 @@ static const struct msm_dsi_config msm8996_dsi_cfg = {
>   	.reg_cfg = {
>   		.num = 3,
>   		.regs = {
> -			{"vdda", 18160, 1 },	/* 1.25 V */
> -			{"vcca", 17000, 32 },	/* 0.925 V */
> -			{"vddio", 100000, 100 },/* 1.8 V */
> +			{"vdda", 18160},	/* 1.25 V */
> +			{"vcca", 17000},	/* 0.925 V */
> +			{"vddio", 100000},/* 1.8 V */
>   		},
>   	},
>   	.bus_clk_names = dsi_8996_bus_clk_names,
> @@ -131,8 +131,8 @@ static const struct msm_dsi_config msm8998_dsi_cfg = {
>   	.reg_cfg = {
>   		.num = 2,
>   		.regs = {
> -			{"vdd", 367000, 16 },	/* 0.9 V */
> -			{"vdda", 62800, 2 },	/* 1.2 V */
> +			{"vdd", 367000},	/* 0.9 V */
> +			{"vdda", 62800},	/* 1.2 V */
>   		},
>   	},
>   	.bus_clk_names = dsi_msm8998_bus_clk_names,
> @@ -150,7 +150,7 @@ static const struct msm_dsi_config sdm660_dsi_cfg = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vdda", 12560, 4 },	/* 1.2 V */
> +			{"vdda", 12560},	/* 1.2 V */
>   		},
>   	},
>   	.bus_clk_names = dsi_sdm660_bus_clk_names,
> @@ -172,7 +172,7 @@ static const struct msm_dsi_config sdm845_dsi_cfg = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vdda", 21800, 4 },	/* 1.2 V */
> +			{"vdda", 21800},	/* 1.2 V */
>   		},
>   	},
>   	.bus_clk_names = dsi_sdm845_bus_clk_names,
> @@ -186,7 +186,7 @@ static const struct msm_dsi_config sc7180_dsi_cfg = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vdda", 21800, 4 },	/* 1.2 V */
> +			{"vdda", 21800},	/* 1.2 V */
>   		},
>   	},
>   	.bus_clk_names = dsi_sc7180_bus_clk_names,
> @@ -204,7 +204,7 @@ static const struct msm_dsi_config sc7280_dsi_cfg = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vdda", 8350, 0 },	/* 1.2 V */
> +			{"vdda", 8350},	/* 1.2 V */
>   		},
>   	},
>   	.bus_clk_names = dsi_sc7280_bus_clk_names,
> @@ -222,7 +222,7 @@ static const struct msm_dsi_config qcm2290_dsi_cfg = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vdda", 21800, 4 },	/* 1.2 V */
> +			{"vdda", 21800},	/* 1.2 V */
>   		},
>   	},
>   	.bus_clk_names = dsi_qcm2290_bus_clk_names,
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index a95d5df52653..9df278d39559 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -203,9 +203,6 @@ static inline void dsi_write(struct msm_dsi_host *msm_host, u32 reg, u32 data)
>   	msm_writel(data, msm_host->ctrl_base + reg);
>   }
>   
> -static int dsi_host_regulator_enable(struct msm_dsi_host *msm_host);
> -static void dsi_host_regulator_disable(struct msm_dsi_host *msm_host);
> -
>   static const struct msm_dsi_cfg_handler *dsi_get_config(
>   						struct msm_dsi_host *msm_host)
>   {
> @@ -256,56 +253,6 @@ static inline struct msm_dsi_host *to_msm_dsi_host(struct mipi_dsi_host *host)
>   	return container_of(host, struct msm_dsi_host, base);
>   }
>   
> -static void dsi_host_regulator_disable(struct msm_dsi_host *msm_host)
> -{
> -	struct regulator_bulk_data *s = msm_host->supplies;
> -	const struct dsi_reg_entry *regs = msm_host->cfg_hnd->cfg->reg_cfg.regs;
> -	int num = msm_host->cfg_hnd->cfg->reg_cfg.num;
> -	int i;
> -
> -	DBG("");
> -	for (i = num - 1; i >= 0; i--)
> -		if (regs[i].disable_load >= 0)
> -			regulator_set_load(s[i].consumer,
> -					   regs[i].disable_load);
> -
> -	regulator_bulk_disable(num, s);
> -}
> -
> -static int dsi_host_regulator_enable(struct msm_dsi_host *msm_host)
> -{
> -	struct regulator_bulk_data *s = msm_host->supplies;
> -	const struct dsi_reg_entry *regs = msm_host->cfg_hnd->cfg->reg_cfg.regs;
> -	int num = msm_host->cfg_hnd->cfg->reg_cfg.num;
> -	int ret, i;
> -
> -	DBG("");
> -	for (i = 0; i < num; i++) {
> -		if (regs[i].enable_load >= 0) {
> -			ret = regulator_set_load(s[i].consumer,
> -						 regs[i].enable_load);
> -			if (ret < 0) {
> -				pr_err("regulator %d set op mode failed, %d\n",
> -					i, ret);
> -				goto fail;
> -			}
> -		}
> -	}
> -
> -	ret = regulator_bulk_enable(num, s);
> -	if (ret < 0) {
> -		pr_err("regulator enable failed, %d\n", ret);
> -		goto fail;
> -	}
> -
> -	return 0;
> -
> -fail:
> -	for (i--; i >= 0; i--)
> -		regulator_set_load(s[i].consumer, regs[i].disable_load);
> -	return ret;
> -}
> -
>   static int dsi_regulator_init(struct msm_dsi_host *msm_host)
>   {
>   	struct regulator_bulk_data *s = msm_host->supplies;
> @@ -323,6 +270,15 @@ static int dsi_regulator_init(struct msm_dsi_host *msm_host)
>   		return ret;
>   	}
>   
> +	for (i = 0; i < num; i++) {
> +		if (regs[i].enable_load >= 0) {
> +			ret = regulator_set_load(s[i].consumer,
> +						 regs[i].enable_load);
> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +
>   	return 0;
>   }
>   
> @@ -2561,7 +2517,8 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
>   
>   	msm_dsi_sfpb_config(msm_host, true);
>   
> -	ret = dsi_host_regulator_enable(msm_host);
> +	ret = regulator_bulk_enable(msm_host->cfg_hnd->cfg->reg_cfg.num,
> +				    msm_host->supplies);
>   	if (ret) {
>   		pr_err("%s:Failed to enable vregs.ret=%d\n",
>   			__func__, ret);
> @@ -2601,7 +2558,8 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
>   	cfg_hnd->ops->link_clk_disable(msm_host);
>   	pm_runtime_put(&msm_host->pdev->dev);
>   fail_disable_reg:
> -	dsi_host_regulator_disable(msm_host);
> +	regulator_bulk_disable(msm_host->cfg_hnd->cfg->reg_cfg.num,
> +			       msm_host->supplies);
>   unlock_ret:
>   	mutex_unlock(&msm_host->dev_mutex);
>   	return ret;
> @@ -2628,7 +2586,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host)
>   	cfg_hnd->ops->link_clk_disable(msm_host);
>   	pm_runtime_put(&msm_host->pdev->dev);
>   
> -	dsi_host_regulator_disable(msm_host);
> +	regulator_bulk_disable(msm_host->cfg_hnd->cfg->reg_cfg.num,
> +			       msm_host->supplies);
>   
>   	msm_dsi_sfpb_config(msm_host, false);
>   
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> index a39de3bdc7fa..7c105120d73e 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> @@ -529,58 +529,16 @@ static int dsi_phy_regulator_init(struct msm_dsi_phy *phy)
>   		return ret;
>   	}
>   
> -	return 0;
> -}
> -
> -static void dsi_phy_regulator_disable(struct msm_dsi_phy *phy)
> -{
> -	struct regulator_bulk_data *s = phy->supplies;
> -	const struct dsi_reg_entry *regs = phy->cfg->reg_cfg.regs;
> -	int num = phy->cfg->reg_cfg.num;
> -	int i;
> -
> -	DBG("");
> -	for (i = num - 1; i >= 0; i--)
> -		if (regs[i].disable_load >= 0)
> -			regulator_set_load(s[i].consumer, regs[i].disable_load);
> -
> -	regulator_bulk_disable(num, s);
> -}
> -
> -static int dsi_phy_regulator_enable(struct msm_dsi_phy *phy)
> -{
> -	struct regulator_bulk_data *s = phy->supplies;
> -	const struct dsi_reg_entry *regs = phy->cfg->reg_cfg.regs;
> -	struct device *dev = &phy->pdev->dev;
> -	int num = phy->cfg->reg_cfg.num;
> -	int ret, i;
> -
> -	DBG("");
>   	for (i = 0; i < num; i++) {
>   		if (regs[i].enable_load >= 0) {
>   			ret = regulator_set_load(s[i].consumer,
>   							regs[i].enable_load);
> -			if (ret < 0) {
> -				DRM_DEV_ERROR(dev,
> -					"regulator %d set op mode failed, %d\n",
> -					i, ret);
> -				goto fail;
> -			}
> +			if (ret < 0)
> +				return ret;
>   		}
>   	}
>   
> -	ret = regulator_bulk_enable(num, s);
> -	if (ret < 0) {
> -		DRM_DEV_ERROR(dev, "regulator enable failed, %d\n", ret);
> -		goto fail;
> -	}
> -
>   	return 0;
> -
> -fail:
> -	for (i--; i >= 0; i--)
> -		regulator_set_load(s[i].consumer, regs[i].disable_load);
> -	return ret;
>   }
>   
>   static int dsi_phy_enable_resource(struct msm_dsi_phy *phy)
> @@ -829,7 +787,7 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
>   		goto res_en_fail;
>   	}
>   
> -	ret = dsi_phy_regulator_enable(phy);
> +	ret = regulator_bulk_enable(phy->cfg->reg_cfg.num, phy->supplies);
>   	if (ret) {
>   		DRM_DEV_ERROR(dev, "%s: regulator enable failed, %d\n",
>   			__func__, ret);
> @@ -866,7 +824,7 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
>   	if (phy->cfg->ops.disable)
>   		phy->cfg->ops.disable(phy);
>   phy_en_fail:
> -	dsi_phy_regulator_disable(phy);
> +	regulator_bulk_disable(phy->cfg->reg_cfg.num, phy->supplies);
>   reg_en_fail:
>   	dsi_phy_disable_resource(phy);
>   res_en_fail:
> @@ -880,7 +838,7 @@ void msm_dsi_phy_disable(struct msm_dsi_phy *phy)
>   
>   	phy->cfg->ops.disable(phy);
>   
> -	dsi_phy_regulator_disable(phy);
> +	regulator_bulk_disable(phy->cfg->reg_cfg.num, phy->supplies);
>   	dsi_phy_disable_resource(phy);
>   }
>   
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> index 08b015ea1b1e..6a10a1448051 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> @@ -1033,7 +1033,7 @@ const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vdds", 36000, 32},
> +			{"vdds", 36000},
>   		},
>   	},
>   	.ops = {
> @@ -1055,7 +1055,7 @@ const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vdds", 36000, 32},
> +			{"vdds", 36000},
>   		},
>   	},
>   	.ops = {
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
> index 8199c53567f4..0f3d4c56c333 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
> @@ -1029,7 +1029,7 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_cfgs = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vcca", 17000, 32},
> +			{"vcca", 17000},
>   		},
>   	},
>   	.ops = {
> @@ -1050,7 +1050,7 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vcca", 73400, 32},
> +			{"vcca", 73400},
>   		},
>   	},
>   	.ops = {
> @@ -1071,7 +1071,7 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_8953_cfgs = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vcca", 17000, 32},
> +			{"vcca", 17000},
>   		},
>   	},
>   	.ops = {
> 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 ee7c418a1c29..b7c621d94981 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
> @@ -134,8 +134,8 @@ const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs = {
>   	.reg_cfg = {
>   		.num = 2,
>   		.regs = {
> -			{"vddio", 100000, 100},	/* 1.8 V */
> -			{"vcca", 10000, 100},	/* 1.0 V */
> +			{"vddio", 100000},	/* 1.8 V */
> +			{"vcca", 10000},	/* 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 48eab80b548e..6beba387640d 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
> @@ -774,7 +774,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vddio", 100000, 100},
> +			{"vddio", 100000},
>   		},
>   	},
>   	.ops = {
> @@ -795,7 +795,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_famb_cfgs = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vddio", 100000, 100},
> +			{"vddio", 100000},
>   		},
>   	},
>   	.ops = {
> @@ -816,7 +816,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_lp_cfgs = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vddio", 100000, 100},	/* 1.8 V */
> +			{"vddio", 100000},	/* 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 fc56cdcc9ad6..2e942b10fffa 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
> @@ -653,7 +653,7 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_8960_cfgs = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vddio", 100000, 100},	/* 1.8 V */
> +			{"vddio", 100000},	/* 1.8 V */
>   		},
>   	},
>   	.ops = {
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> index 66ed1919a1db..9c7c49ce1200 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> @@ -1041,7 +1041,7 @@ const struct msm_dsi_phy_cfg dsi_phy_7nm_cfgs = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vdds", 36000, 32},
> +			{"vdds", 36000},
>   		},
>   	},
>   	.ops = {
> @@ -1068,7 +1068,7 @@ const struct msm_dsi_phy_cfg dsi_phy_7nm_8150_cfgs = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vdds", 36000, 32},
> +			{"vdds", 36000},
>   		},
>   	},
>   	.ops = {
> @@ -1090,7 +1090,7 @@ const struct msm_dsi_phy_cfg dsi_phy_7nm_7280_cfgs = {
>   	.reg_cfg = {
>   		.num = 1,
>   		.regs = {
> -			{"vdds", 37550, 0},
> +			{"vdds", 37550},
>   		},
>   	},
>   	.ops = {

[PATCH v3 4/6] drm/msm/dsi: Use the new regulator bulk feature to specify the load

[Douglas Anderson]

As of commit 6eabfc018e8d ("regulator: core: Allow specifying an
initial load w/ the bulk API") we can now specify the initial load in
the bulk data rather than having to manually call regulator_set_load()
on each regulator. Let's use it.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- Update commit message to point at the git hash of the regulator change.

Changes in v2:
- ("Use the new regulator bulk feature to specify the load") new for v2.

 drivers/gpu/drm/msm/dsi/dsi_host.c    | 13 +++----------
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 13 +++----------
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 9df278d39559..a0a1b6d61d05 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -260,8 +260,10 @@ static int dsi_regulator_init(struct msm_dsi_host *msm_host)
 	int num = msm_host->cfg_hnd->cfg->reg_cfg.num;
 	int i, ret;
 
-	for (i = 0; i < num; i++)
+	for (i = 0; i < num; i++) {
 		s[i].supply = regs[i].name;
+		s[i].init_load_uA = regs[i].enable_load;
+	}
 
 	ret = devm_regulator_bulk_get(&msm_host->pdev->dev, num, s);
 	if (ret < 0) {
@@ -270,15 +272,6 @@ static int dsi_regulator_init(struct msm_dsi_host *msm_host)
 		return ret;
 	}
 
-	for (i = 0; i < num; i++) {
-		if (regs[i].enable_load >= 0) {
-			ret = regulator_set_load(s[i].consumer,
-						 regs[i].enable_load);
-			if (ret < 0)
-				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 7c105120d73e..efb6b1726cdb 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -515,8 +515,10 @@ static int dsi_phy_regulator_init(struct msm_dsi_phy *phy)
 	int num = phy->cfg->reg_cfg.num;
 	int i, ret;
 
-	for (i = 0; i < num; i++)
+	for (i = 0; i < num; i++) {
 		s[i].supply = regs[i].name;
+		s[i].init_load_uA = regs[i].enable_load;
+	}
 
 	ret = devm_regulator_bulk_get(dev, num, s);
 	if (ret < 0) {
@@ -529,15 +531,6 @@ static int dsi_phy_regulator_init(struct msm_dsi_phy *phy)
 		return ret;
 	}
 
-	for (i = 0; i < num; i++) {
-		if (regs[i].enable_load >= 0) {
-			ret = regulator_set_load(s[i].consumer,
-							regs[i].enable_load);
-			if (ret < 0)
-				return ret;
-		}
-	}
-
 	return 0;
 }
 
-- 
2.37.1.455.g008518b4e5-goog

Re: [PATCH v3 4/6] drm/msm/dsi: Use the new regulator bulk feature to specify the load

[Dmitry Baryshkov]

On 03/08/2022 01:37, Douglas Anderson wrote:
> As of commit 6eabfc018e8d ("regulator: core: Allow specifying an
> initial load w/ the bulk API") we can now specify the initial load in
> the bulk data rather than having to manually call regulator_set_load()
> on each regulator. Let's use it.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

It might have been better, if the previous patch had only removed the 
load_setting on disable and inlined the dsi_host_regulator_disable(). 
Then this patch would drop the regulator_set_load() from 
dsi_host_regulator_enable() path and inline it. Then it would have been 
more obvious that after these two changes the time when we set loads is 
not changed.

> ---
> 
> Changes in v3:
> - Update commit message to point at the git hash of the regulator change.
> 
> Changes in v2:
> - ("Use the new regulator bulk feature to specify the load") new for v2.
> 
>   drivers/gpu/drm/msm/dsi/dsi_host.c    | 13 +++----------
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 13 +++----------
>   2 files changed, 6 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 9df278d39559..a0a1b6d61d05 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -260,8 +260,10 @@ static int dsi_regulator_init(struct msm_dsi_host *msm_host)
>   	int num = msm_host->cfg_hnd->cfg->reg_cfg.num;
>   	int i, ret;
>   
> -	for (i = 0; i < num; i++)
> +	for (i = 0; i < num; i++) {
>   		s[i].supply = regs[i].name;
> +		s[i].init_load_uA = regs[i].enable_load;
> +	}
>   
>   	ret = devm_regulator_bulk_get(&msm_host->pdev->dev, num, s);
>   	if (ret < 0) {
> @@ -270,15 +272,6 @@ static int dsi_regulator_init(struct msm_dsi_host *msm_host)
>   		return ret;
>   	}
>   
> -	for (i = 0; i < num; i++) {
> -		if (regs[i].enable_load >= 0) {
> -			ret = regulator_set_load(s[i].consumer,
> -						 regs[i].enable_load);
> -			if (ret < 0)
> -				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 7c105120d73e..efb6b1726cdb 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> @@ -515,8 +515,10 @@ static int dsi_phy_regulator_init(struct msm_dsi_phy *phy)
>   	int num = phy->cfg->reg_cfg.num;
>   	int i, ret;
>   
> -	for (i = 0; i < num; i++)
> +	for (i = 0; i < num; i++) {
>   		s[i].supply = regs[i].name;
> +		s[i].init_load_uA = regs[i].enable_load;
> +	}
>   
>   	ret = devm_regulator_bulk_get(dev, num, s);
>   	if (ret < 0) {
> @@ -529,15 +531,6 @@ static int dsi_phy_regulator_init(struct msm_dsi_phy *phy)
>   		return ret;
>   	}
>   
> -	for (i = 0; i < num; i++) {
> -		if (regs[i].enable_load >= 0) {
> -			ret = regulator_set_load(s[i].consumer,
> -							regs[i].enable_load);
> -			if (ret < 0)
> -				return ret;
> -		}
> -	}
> -
>   	return 0;
>   }
>   


-- 
With best wishes
Dmitry

Re: [PATCH v3 4/6] drm/msm/dsi: Use the new regulator bulk feature to specify the load

[Doug Anderson]

Hi,

On Wed, Aug 3, 2022 at 12:12 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 03/08/2022 01:37, Douglas Anderson wrote:
> > As of commit 6eabfc018e8d ("regulator: core: Allow specifying an
> > initial load w/ the bulk API") we can now specify the initial load in
> > the bulk data rather than having to manually call regulator_set_load()
> > on each regulator. Let's use it.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> It might have been better, if the previous patch had only removed the
> load_setting on disable and inlined the dsi_host_regulator_disable().
> Then this patch would drop the regulator_set_load() from
> dsi_host_regulator_enable() path and inline it. Then it would have been
> more obvious that after these two changes the time when we set loads is
> not changed.

Seems like I should post a v4 to update the commit message of the
final patch in the series, but I'm going to leave this the way it is
since the end result is the same. Originally when I wrote the series I
didn't know if the new regulator API changes would be accepted, so the
previous patch did the most cleanup it could do with the old API. ;-)

-Doug

Re: [PATCH v3 4/6] drm/msm/dsi: Use the new regulator bulk feature to specify the load

[Abhinav Kumar]

On 8/2/2022 3:37 PM, Douglas Anderson wrote:
> As of commit 6eabfc018e8d ("regulator: core: Allow specifying an
> initial load w/ the bulk API") we can now specify the initial load in
> the bulk data rather than having to manually call regulator_set_load()
> on each regulator. Let's use it.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> 
> Changes in v3:
> - Update commit message to point at the git hash of the regulator change.
> 
> Changes in v2:
> - ("Use the new regulator bulk feature to specify the load") new for v2.
> 
>   drivers/gpu/drm/msm/dsi/dsi_host.c    | 13 +++----------
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 13 +++----------
>   2 files changed, 6 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 9df278d39559..a0a1b6d61d05 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -260,8 +260,10 @@ static int dsi_regulator_init(struct msm_dsi_host *msm_host)
>   	int num = msm_host->cfg_hnd->cfg->reg_cfg.num;
>   	int i, ret;
>   
> -	for (i = 0; i < num; i++)
> +	for (i = 0; i < num; i++) {
>   		s[i].supply = regs[i].name;
> +		s[i].init_load_uA = regs[i].enable_load;
> +	}
>   
>   	ret = devm_regulator_bulk_get(&msm_host->pdev->dev, num, s);
>   	if (ret < 0) {
> @@ -270,15 +272,6 @@ static int dsi_regulator_init(struct msm_dsi_host *msm_host)
>   		return ret;
>   	}
>   
> -	for (i = 0; i < num; i++) {
> -		if (regs[i].enable_load >= 0) {
> -			ret = regulator_set_load(s[i].consumer,
> -						 regs[i].enable_load);
> -			if (ret < 0)
> -				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 7c105120d73e..efb6b1726cdb 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> @@ -515,8 +515,10 @@ static int dsi_phy_regulator_init(struct msm_dsi_phy *phy)
>   	int num = phy->cfg->reg_cfg.num;
>   	int i, ret;
>   
> -	for (i = 0; i < num; i++)
> +	for (i = 0; i < num; i++) {
>   		s[i].supply = regs[i].name;
> +		s[i].init_load_uA = regs[i].enable_load;
> +	}
>   
>   	ret = devm_regulator_bulk_get(dev, num, s);
>   	if (ret < 0) {
> @@ -529,15 +531,6 @@ static int dsi_phy_regulator_init(struct msm_dsi_phy *phy)
>   		return ret;
>   	}
>   
> -	for (i = 0; i < num; i++) {
> -		if (regs[i].enable_load >= 0) {
> -			ret = regulator_set_load(s[i].consumer,
> -							regs[i].enable_load);
> -			if (ret < 0)
> -				return ret;
> -		}
> -	}
> -
>   	return 0;
>   }
>   

[PATCH v3 5/6] drm/msm/dsi: Take advantage of devm_regulator_bulk_get_const()

[Douglas Anderson]

As of the commit 1de452a0edda ("regulator: core: Allow drivers to
define their init data as const") we no longer need to do copying of
regulator bulk data from initdata to something dynamic. Let's take
advantage of that.

In addition to saving some code, this also moves us to using
ARRAY_SIZE() to specify how many regulators we have which is less
error prone.

This gets rid of some layers of wrappers which makes it obvious that
we can get rid of an extra error print.
devm_regulator_bulk_get_const() prints errors for you so you don't
need an extra layer of printing.

In all cases here I have preserved the old settings without any
investigation about whether the loads being set are sensible. In the
cases of some of the PHYs if several PHYs in the same file used
exactly the same settings I had them point to the same data structure.

NOTE: Though I haven't done the math, this is likely an overall
savings in terms of "static const" data. We previously always
allocated space for 8 supplies. Each of these supplies took up 36
bytes of data (32 for name, 4 for an int).

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- Do all the PHYs too.
- Get rid of error print after devm_regulator_bulk_get_const().
- Just directly call the bulk commands; get rid of the wrapper.
- Update commit message to point at the git hash of the regulator change.

Changes in v2:
- ("Take advantage of devm_regulator_bulk_get_const") new for v2.

 drivers/gpu/drm/msm/dsi/dsi.h                 |  12 --
 drivers/gpu/drm/msm/dsi/dsi_cfg.c             | 172 +++++++++---------
 drivers/gpu/drm/msm/dsi/dsi_cfg.h             |   3 +-
 drivers/gpu/drm/msm/dsi/dsi_host.c            |  42 ++---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c         |  37 +---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h         |   5 +-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c    |  20 +-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c    |  32 ++--
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c    |  14 +-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c    |  28 +--
 .../gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c   |  12 +-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c     |  32 ++--
 12 files changed, 167 insertions(+), 242 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index bb6a5bd05cb1..d661510d570d 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -30,20 +30,8 @@ enum msm_dsi_phy_usecase {
 	MSM_DSI_PHY_SLAVE,
 };
 
-#define DSI_DEV_REGULATOR_MAX	8
 #define DSI_BUS_CLK_MAX		4
 
-/* Regulators for DSI devices */
-struct dsi_reg_entry {
-	char name[32];
-	int enable_load;
-};
-
-struct dsi_reg_config {
-	int num;
-	struct dsi_reg_entry regs[DSI_DEV_REGULATOR_MAX];
-};
-
 struct msm_dsi {
 	struct drm_device *dev;
 	struct platform_device *pdev;
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index 901d6fd53800..7e97c239ed48 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -9,16 +9,16 @@ static const char * const dsi_v2_bus_clk_names[] = {
 	"core_mmss", "iface", "bus",
 };
 
+static const struct regulator_bulk_data apq8064_dsi_regulators[] = {
+	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.2 V */
+	{ .supply = "avdd", .init_load_uA = 10000 },	/* 3.0 V */
+	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
+};
+
 static const struct msm_dsi_config apq8064_dsi_cfg = {
 	.io_offset = 0,
-	.reg_cfg = {
-		.num = 3,
-		.regs = {
-			{"vdda", 100000},	/* 1.2 V */
-			{"avdd", 10000},	/* 3.0 V */
-			{"vddio", 100000},	/* 1.8 V */
-		},
-	},
+	.regulator_data = apq8064_dsi_regulators,
+	.num_regulators = ARRAY_SIZE(apq8064_dsi_regulators),
 	.bus_clk_names = dsi_v2_bus_clk_names,
 	.num_bus_clks = ARRAY_SIZE(dsi_v2_bus_clk_names),
 	.io_start = { 0x4700000, 0x5800000 },
@@ -29,16 +29,16 @@ static const char * const dsi_6g_bus_clk_names[] = {
 	"mdp_core", "iface", "bus", "core_mmss",
 };
 
+static const struct regulator_bulk_data msm8974_apq8084_regulators[] = {
+	{ .supply = "vdd", .init_load_uA = 150000 },	/* 3.0 V */
+	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.2 V */
+	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
+};
+
 static const struct msm_dsi_config msm8974_apq8084_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
-	.reg_cfg = {
-		.num = 3,
-		.regs = {
-			{"vdd", 150000},	/* 3.0 V */
-			{"vdda", 100000},	/* 1.2 V */
-			{"vddio", 100000},	/* 1.8 V */
-		},
-	},
+	.regulator_data = msm8974_apq8084_regulators,
+	.num_regulators = ARRAY_SIZE(msm8974_apq8084_regulators),
 	.bus_clk_names = dsi_6g_bus_clk_names,
 	.num_bus_clks = ARRAY_SIZE(dsi_6g_bus_clk_names),
 	.io_start = { 0xfd922800, 0xfd922b00 },
@@ -49,15 +49,15 @@ static const char * const dsi_8916_bus_clk_names[] = {
 	"mdp_core", "iface", "bus",
 };
 
+static const struct regulator_bulk_data msm8916_dsi_regulators[] = {
+	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.2 V */
+	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
+};
+
 static const struct msm_dsi_config msm8916_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
-	.reg_cfg = {
-		.num = 2,
-		.regs = {
-			{"vdda", 100000},	/* 1.2 V */
-			{"vddio", 100000},	/* 1.8 V */
-		},
-	},
+	.regulator_data = msm8916_dsi_regulators,
+	.num_regulators = ARRAY_SIZE(msm8916_dsi_regulators),
 	.bus_clk_names = dsi_8916_bus_clk_names,
 	.num_bus_clks = ARRAY_SIZE(dsi_8916_bus_clk_names),
 	.io_start = { 0x1a98000 },
@@ -68,34 +68,34 @@ static const char * const dsi_8976_bus_clk_names[] = {
 	"mdp_core", "iface", "bus",
 };
 
+static const struct regulator_bulk_data msm8976_dsi_regulators[] = {
+	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.2 V */
+	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
+};
+
 static const struct msm_dsi_config msm8976_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
-	.reg_cfg = {
-		.num = 2,
-		.regs = {
-			{"vdda", 100000},	/* 1.2 V */
-			{"vddio", 100000},	/* 1.8 V */
-		},
-	},
+	.regulator_data = msm8976_dsi_regulators,
+	.num_regulators = ARRAY_SIZE(msm8976_dsi_regulators),
 	.bus_clk_names = dsi_8976_bus_clk_names,
 	.num_bus_clks = ARRAY_SIZE(dsi_8976_bus_clk_names),
 	.io_start = { 0x1a94000, 0x1a96000 },
 	.num_dsi = 2,
 };
 
+static const struct regulator_bulk_data msm8994_dsi_regulators[] = {
+	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.25 V */
+	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
+	{ .supply = "vcca", .init_load_uA = 10000 },	/* 1.0 V */
+	{ .supply = "vdd", .init_load_uA = 100000 },	/* 1.8 V */
+	{ .supply = "lab_reg", .init_load_uA = -1 },
+	{ .supply = "ibb_reg", .init_load_uA = -1 },
+};
+
 static const struct msm_dsi_config msm8994_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
-	.reg_cfg = {
-		.num = 6,
-		.regs = {
-			{"vdda", 100000},	/* 1.25 V */
-			{"vddio", 100000},	/* 1.8 V */
-			{"vcca", 10000},	/* 1.0 V */
-			{"vdd", 100000},	/* 1.8 V */
-			{"lab_reg", -1},
-			{"ibb_reg", -1},
-		},
-	},
+	.regulator_data = msm8994_dsi_regulators,
+	.num_regulators = ARRAY_SIZE(msm8994_dsi_regulators),
 	.bus_clk_names = dsi_6g_bus_clk_names,
 	.num_bus_clks = ARRAY_SIZE(dsi_6g_bus_clk_names),
 	.io_start = { 0xfd998000, 0xfd9a0000 },
@@ -106,16 +106,16 @@ static const char * const dsi_8996_bus_clk_names[] = {
 	"mdp_core", "iface", "bus", "core_mmss",
 };
 
+static const struct regulator_bulk_data msm8996_dsi_regulators[] = {
+	{ .supply = "vdda", .init_load_uA = 18160 },	/* 1.25 V */
+	{ .supply = "vcca", .init_load_uA = 17000 },	/* 0.925 V */
+	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
+};
+
 static const struct msm_dsi_config msm8996_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
-	.reg_cfg = {
-		.num = 3,
-		.regs = {
-			{"vdda", 18160},	/* 1.25 V */
-			{"vcca", 17000},	/* 0.925 V */
-			{"vddio", 100000},/* 1.8 V */
-		},
-	},
+	.regulator_data = msm8996_dsi_regulators,
+	.num_regulators = ARRAY_SIZE(msm8996_dsi_regulators),
 	.bus_clk_names = dsi_8996_bus_clk_names,
 	.num_bus_clks = ARRAY_SIZE(dsi_8996_bus_clk_names),
 	.io_start = { 0x994000, 0x996000 },
@@ -126,15 +126,15 @@ static const char * const dsi_msm8998_bus_clk_names[] = {
 	"iface", "bus", "core",
 };
 
+static const struct regulator_bulk_data msm8998_dsi_regulators[] = {
+	{ .supply = "vdd", .init_load_uA = 367000 },	/* 0.9 V */
+	{ .supply = "vdda", .init_load_uA = 62800 },	/* 1.2 V */
+};
+
 static const struct msm_dsi_config msm8998_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
-	.reg_cfg = {
-		.num = 2,
-		.regs = {
-			{"vdd", 367000},	/* 0.9 V */
-			{"vdda", 62800},	/* 1.2 V */
-		},
-	},
+	.regulator_data = msm8998_dsi_regulators,
+	.num_regulators = ARRAY_SIZE(msm8998_dsi_regulators),
 	.bus_clk_names = dsi_msm8998_bus_clk_names,
 	.num_bus_clks = ARRAY_SIZE(dsi_msm8998_bus_clk_names),
 	.io_start = { 0xc994000, 0xc996000 },
@@ -145,14 +145,14 @@ static const char * const dsi_sdm660_bus_clk_names[] = {
 	"iface", "bus", "core", "core_mmss",
 };
 
+static const struct regulator_bulk_data sdm660_dsi_regulators[] = {
+	{ .supply = "vdda", .init_load_uA = 12560 },	/* 1.2 V */
+};
+
 static const struct msm_dsi_config sdm660_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vdda", 12560},	/* 1.2 V */
-		},
-	},
+	.regulator_data = sdm660_dsi_regulators,
+	.num_regulators = ARRAY_SIZE(sdm660_dsi_regulators),
 	.bus_clk_names = dsi_sdm660_bus_clk_names,
 	.num_bus_clks = ARRAY_SIZE(dsi_sdm660_bus_clk_names),
 	.io_start = { 0xc994000, 0xc996000 },
@@ -167,28 +167,28 @@ static const char * const dsi_sc7180_bus_clk_names[] = {
 	"iface", "bus",
 };
 
+static const struct regulator_bulk_data sdm845_dsi_regulators[] = {
+	{ .supply = "vdda", .init_load_uA = 21800 },	/* 1.2 V */
+};
+
 static const struct msm_dsi_config sdm845_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vdda", 21800},	/* 1.2 V */
-		},
-	},
+	.regulator_data = sdm845_dsi_regulators,
+	.num_regulators = ARRAY_SIZE(sdm845_dsi_regulators),
 	.bus_clk_names = dsi_sdm845_bus_clk_names,
 	.num_bus_clks = ARRAY_SIZE(dsi_sdm845_bus_clk_names),
 	.io_start = { 0xae94000, 0xae96000 },
 	.num_dsi = 2,
 };
 
+static const struct regulator_bulk_data sc7180_dsi_regulators[] = {
+	{ .supply = "vdda", .init_load_uA = 21800 },	/* 1.2 V */
+};
+
 static const struct msm_dsi_config sc7180_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vdda", 21800},	/* 1.2 V */
-		},
-	},
+	.regulator_data = sc7180_dsi_regulators,
+	.num_regulators = ARRAY_SIZE(sc7180_dsi_regulators),
 	.bus_clk_names = dsi_sc7180_bus_clk_names,
 	.num_bus_clks = ARRAY_SIZE(dsi_sc7180_bus_clk_names),
 	.io_start = { 0xae94000 },
@@ -199,14 +199,14 @@ static const char * const dsi_sc7280_bus_clk_names[] = {
 	"iface", "bus",
 };
 
+static const struct regulator_bulk_data sc7280_dsi_regulators[] = {
+	{ .supply = "vdda", .init_load_uA = 8350 },	/* 1.2 V */
+};
+
 static const struct msm_dsi_config sc7280_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vdda", 8350},	/* 1.2 V */
-		},
-	},
+	.regulator_data = sc7280_dsi_regulators,
+	.num_regulators = ARRAY_SIZE(sc7280_dsi_regulators),
 	.bus_clk_names = dsi_sc7280_bus_clk_names,
 	.num_bus_clks = ARRAY_SIZE(dsi_sc7280_bus_clk_names),
 	.io_start = { 0xae94000 },
@@ -217,14 +217,14 @@ static const char * const dsi_qcm2290_bus_clk_names[] = {
 	"iface", "bus",
 };
 
+static const struct regulator_bulk_data qcm2290_dsi_cfg_regulators[] = {
+	{ .supply = "vdda", .init_load_uA = 21800 },	/* 1.2 V */
+};
+
 static const struct msm_dsi_config qcm2290_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vdda", 21800},	/* 1.2 V */
-		},
-	},
+	.regulator_data = qcm2290_dsi_cfg_regulators,
+	.num_regulators = ARRAY_SIZE(qcm2290_dsi_cfg_regulators),
 	.bus_clk_names = dsi_qcm2290_bus_clk_names,
 	.num_bus_clks = ARRAY_SIZE(dsi_qcm2290_bus_clk_names),
 	.io_start = { 0x5e94000 },
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
index fe54a999968b..8f04e685a74e 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
@@ -32,7 +32,8 @@
 
 struct msm_dsi_config {
 	u32 io_offset;
-	struct dsi_reg_config reg_cfg;
+	const struct regulator_bulk_data *regulator_data;
+	int num_regulators;
 	const char * const *bus_clk_names;
 	const int num_bus_clks;
 	const resource_size_t io_start[DSI_MAX];
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index a0a1b6d61d05..3364b2affac5 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -106,7 +106,7 @@ struct msm_dsi_host {
 
 	void __iomem *ctrl_base;
 	phys_addr_t ctrl_size;
-	struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];
+	struct regulator_bulk_data *supplies;
 
 	int num_bus_clks;
 	struct clk_bulk_data bus_clks[DSI_BUS_CLK_MAX];
@@ -253,28 +253,6 @@ static inline struct msm_dsi_host *to_msm_dsi_host(struct mipi_dsi_host *host)
 	return container_of(host, struct msm_dsi_host, base);
 }
 
-static int dsi_regulator_init(struct msm_dsi_host *msm_host)
-{
-	struct regulator_bulk_data *s = msm_host->supplies;
-	const struct dsi_reg_entry *regs = msm_host->cfg_hnd->cfg->reg_cfg.regs;
-	int num = msm_host->cfg_hnd->cfg->reg_cfg.num;
-	int i, ret;
-
-	for (i = 0; i < num; i++) {
-		s[i].supply = regs[i].name;
-		s[i].init_load_uA = regs[i].enable_load;
-	}
-
-	ret = devm_regulator_bulk_get(&msm_host->pdev->dev, num, s);
-	if (ret < 0) {
-		pr_err("%s: failed to init regulator, ret=%d\n",
-						__func__, ret);
-		return ret;
-	}
-
-	return 0;
-}
-
 int dsi_clk_init_v2(struct msm_dsi_host *msm_host)
 {
 	struct platform_device *pdev = msm_host->pdev;
@@ -1982,6 +1960,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
 {
 	struct msm_dsi_host *msm_host = NULL;
 	struct platform_device *pdev = msm_dsi->pdev;
+	const struct msm_dsi_config *cfg;
 	int ret;
 
 	msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL);
@@ -2014,6 +1993,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
 		pr_err("%s: get config failed\n", __func__);
 		goto fail;
 	}
+	cfg = msm_host->cfg_hnd->cfg;
 
 	msm_host->id = dsi_host_get_id(msm_host);
 	if (msm_host->id < 0) {
@@ -2023,13 +2003,13 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
 	}
 
 	/* fixup base address by io offset */
-	msm_host->ctrl_base += msm_host->cfg_hnd->cfg->io_offset;
+	msm_host->ctrl_base += cfg->io_offset;
 
-	ret = dsi_regulator_init(msm_host);
-	if (ret) {
-		pr_err("%s: regulator init failed\n", __func__);
+	ret = devm_regulator_bulk_get_const(&pdev->dev, cfg->num_regulators,
+					    cfg->regulator_data,
+					    &msm_host->supplies);
+	if (ret)
 		goto fail;
-	}
 
 	ret = dsi_clk_init(msm_host);
 	if (ret) {
@@ -2510,7 +2490,7 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
 
 	msm_dsi_sfpb_config(msm_host, true);
 
-	ret = regulator_bulk_enable(msm_host->cfg_hnd->cfg->reg_cfg.num,
+	ret = regulator_bulk_enable(msm_host->cfg_hnd->cfg->num_regulators,
 				    msm_host->supplies);
 	if (ret) {
 		pr_err("%s:Failed to enable vregs.ret=%d\n",
@@ -2551,7 +2531,7 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
 	cfg_hnd->ops->link_clk_disable(msm_host);
 	pm_runtime_put(&msm_host->pdev->dev);
 fail_disable_reg:
-	regulator_bulk_disable(msm_host->cfg_hnd->cfg->reg_cfg.num,
+	regulator_bulk_disable(msm_host->cfg_hnd->cfg->num_regulators,
 			       msm_host->supplies);
 unlock_ret:
 	mutex_unlock(&msm_host->dev_mutex);
@@ -2579,7 +2559,7 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host)
 	cfg_hnd->ops->link_clk_disable(msm_host);
 	pm_runtime_put(&msm_host->pdev->dev);
 
-	regulator_bulk_disable(msm_host->cfg_hnd->cfg->reg_cfg.num,
+	regulator_bulk_disable(msm_host->cfg_hnd->cfg->num_regulators,
 			       msm_host->supplies);
 
 	msm_dsi_sfpb_config(msm_host, false);
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index efb6b1726cdb..0a00f9b73fc5 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -507,33 +507,6 @@ int msm_dsi_cphy_timing_calc_v4(struct msm_dsi_dphy_timing *timing,
 	return 0;
 }
 
-static int dsi_phy_regulator_init(struct msm_dsi_phy *phy)
-{
-	struct regulator_bulk_data *s = phy->supplies;
-	const struct dsi_reg_entry *regs = phy->cfg->reg_cfg.regs;
-	struct device *dev = &phy->pdev->dev;
-	int num = phy->cfg->reg_cfg.num;
-	int i, ret;
-
-	for (i = 0; i < num; i++) {
-		s[i].supply = regs[i].name;
-		s[i].init_load_uA = regs[i].enable_load;
-	}
-
-	ret = devm_regulator_bulk_get(dev, num, s);
-	if (ret < 0) {
-		if (ret != -EPROBE_DEFER) {
-			DRM_DEV_ERROR(dev,
-				      "%s: failed to init regulator, ret=%d\n",
-				      __func__, ret);
-		}
-
-		return ret;
-	}
-
-	return 0;
-}
-
 static int dsi_phy_enable_resource(struct msm_dsi_phy *phy)
 {
 	struct device *dev = &phy->pdev->dev;
@@ -698,7 +671,9 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
 			goto fail;
 	}
 
-	ret = dsi_phy_regulator_init(phy);
+	ret = devm_regulator_bulk_get_const(dev, phy->cfg->num_regulators,
+					    phy->cfg->regulator_data,
+					    &phy->supplies);
 	if (ret)
 		goto fail;
 
@@ -780,7 +755,7 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
 		goto res_en_fail;
 	}
 
-	ret = regulator_bulk_enable(phy->cfg->reg_cfg.num, phy->supplies);
+	ret = regulator_bulk_enable(phy->cfg->num_regulators, phy->supplies);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "%s: regulator enable failed, %d\n",
 			__func__, ret);
@@ -817,7 +792,7 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
 	if (phy->cfg->ops.disable)
 		phy->cfg->ops.disable(phy);
 phy_en_fail:
-	regulator_bulk_disable(phy->cfg->reg_cfg.num, phy->supplies);
+	regulator_bulk_disable(phy->cfg->num_regulators, phy->supplies);
 reg_en_fail:
 	dsi_phy_disable_resource(phy);
 res_en_fail:
@@ -831,7 +806,7 @@ void msm_dsi_phy_disable(struct msm_dsi_phy *phy)
 
 	phy->cfg->ops.disable(phy);
 
-	regulator_bulk_disable(phy->cfg->reg_cfg.num, phy->supplies);
+	regulator_bulk_disable(phy->cfg->num_regulators, phy->supplies);
 	dsi_phy_disable_resource(phy);
 }
 
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
index dc91b43d5a38..60a99c6525b2 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -29,7 +29,8 @@ struct msm_dsi_phy_ops {
 };
 
 struct msm_dsi_phy_cfg {
-	struct dsi_reg_config reg_cfg;
+	const struct regulator_bulk_data *regulator_data;
+	int num_regulators;
 	struct msm_dsi_phy_ops ops;
 
 	unsigned long	min_pll_rate;
@@ -98,7 +99,7 @@ struct msm_dsi_phy {
 	int id;
 
 	struct clk *ahb_clk;
-	struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];
+	struct regulator_bulk_data *supplies;
 
 	struct msm_dsi_dphy_timing timing;
 	const struct msm_dsi_phy_cfg *cfg;
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
index 6a10a1448051..e34a2274db87 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
@@ -1028,14 +1028,14 @@ static int dsi_10nm_phy_parse_dt(struct msm_dsi_phy *phy)
 	return 0;
 }
 
+static const struct regulator_bulk_data dsi_phy_10nm_regulators[] = {
+	{ .supply = "vdds", .init_load_uA = 36000 },
+};
+
 const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs = {
 	.has_phy_lane = true,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vdds", 36000},
-		},
-	},
+	.regulator_data = dsi_phy_10nm_regulators,
+	.num_regulators = ARRAY_SIZE(dsi_phy_10nm_regulators),
 	.ops = {
 		.enable = dsi_10nm_phy_enable,
 		.disable = dsi_10nm_phy_disable,
@@ -1052,12 +1052,8 @@ const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs = {
 
 const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs = {
 	.has_phy_lane = true,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vdds", 36000},
-		},
-	},
+	.regulator_data = dsi_phy_10nm_regulators,
+	.num_regulators = ARRAY_SIZE(dsi_phy_10nm_regulators),
 	.ops = {
 		.enable = dsi_10nm_phy_enable,
 		.disable = dsi_10nm_phy_disable,
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
index 0f3d4c56c333..be1500368d95 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
@@ -1024,14 +1024,18 @@ static void dsi_14nm_phy_disable(struct msm_dsi_phy *phy)
 	wmb();
 }
 
+static const struct regulator_bulk_data dsi_phy_14nm_17mA_regulators[] = {
+	{ .supply = "vcca", .init_load_uA = 17000 },
+};
+
+static const struct regulator_bulk_data dsi_phy_14nm_73p4mA_regulators[] = {
+	{ .supply = "vcca", .init_load_uA = 73400 },
+};
+
 const struct msm_dsi_phy_cfg dsi_phy_14nm_cfgs = {
 	.has_phy_lane = true,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vcca", 17000},
-		},
-	},
+	.regulator_data = dsi_phy_14nm_17mA_regulators,
+	.num_regulators = ARRAY_SIZE(dsi_phy_14nm_17mA_regulators),
 	.ops = {
 		.enable = dsi_14nm_phy_enable,
 		.disable = dsi_14nm_phy_disable,
@@ -1047,12 +1051,8 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_cfgs = {
 
 const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs = {
 	.has_phy_lane = true,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vcca", 73400},
-		},
-	},
+	.regulator_data = dsi_phy_14nm_73p4mA_regulators,
+	.num_regulators = ARRAY_SIZE(dsi_phy_14nm_73p4mA_regulators),
 	.ops = {
 		.enable = dsi_14nm_phy_enable,
 		.disable = dsi_14nm_phy_disable,
@@ -1068,12 +1068,8 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs = {
 
 const struct msm_dsi_phy_cfg dsi_phy_14nm_8953_cfgs = {
 	.has_phy_lane = true,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vcca", 17000},
-		},
-	},
+	.regulator_data = dsi_phy_14nm_17mA_regulators,
+	.num_regulators = ARRAY_SIZE(dsi_phy_14nm_17mA_regulators),
 	.ops = {
 		.enable = dsi_14nm_phy_enable,
 		.disable = dsi_14nm_phy_disable,
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 b7c621d94981..c9752b991744 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
@@ -129,15 +129,15 @@ static void dsi_20nm_phy_disable(struct msm_dsi_phy *phy)
 	dsi_20nm_phy_regulator_ctrl(phy, false);
 }
 
+static const struct regulator_bulk_data dsi_phy_20nm_regulators[] = {
+	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
+	{ .supply = "vcca", .init_load_uA = 10000 },	/* 1.0 V */
+};
+
 const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs = {
 	.has_phy_regulator = true,
-	.reg_cfg = {
-		.num = 2,
-		.regs = {
-			{"vddio", 100000},	/* 1.8 V */
-			{"vcca", 10000},	/* 1.0 V */
-		},
-	},
+	.regulator_data = dsi_phy_20nm_regulators,
+	.num_regulators = ARRAY_SIZE(dsi_phy_20nm_regulators),
 	.ops = {
 		.enable = dsi_20nm_phy_enable,
 		.disable = dsi_20nm_phy_disable,
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 6beba387640d..578c671a3bb1 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
@@ -769,14 +769,14 @@ static void dsi_28nm_phy_disable(struct msm_dsi_phy *phy)
 	wmb();
 }
 
+static const struct regulator_bulk_data dsi_phy_28nm_regulators[] = {
+	{ .supply = "vddio", .init_load_uA = 100000 },
+};
+
 const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs = {
 	.has_phy_regulator = true,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vddio", 100000},
-		},
-	},
+	.regulator_data = dsi_phy_28nm_regulators,
+	.num_regulators = ARRAY_SIZE(dsi_phy_28nm_regulators),
 	.ops = {
 		.enable = dsi_28nm_phy_enable,
 		.disable = dsi_28nm_phy_disable,
@@ -792,12 +792,8 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs = {
 
 const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_famb_cfgs = {
 	.has_phy_regulator = true,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vddio", 100000},
-		},
-	},
+	.regulator_data = dsi_phy_28nm_regulators,
+	.num_regulators = ARRAY_SIZE(dsi_phy_28nm_regulators),
 	.ops = {
 		.enable = dsi_28nm_phy_enable,
 		.disable = dsi_28nm_phy_disable,
@@ -813,12 +809,8 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_famb_cfgs = {
 
 const struct msm_dsi_phy_cfg dsi_phy_28nm_lp_cfgs = {
 	.has_phy_regulator = true,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vddio", 100000},	/* 1.8 V */
-		},
-	},
+	.regulator_data = dsi_phy_28nm_regulators,
+	.num_regulators = ARRAY_SIZE(dsi_phy_28nm_regulators),
 	.ops = {
 		.enable = dsi_28nm_phy_enable,
 		.disable = dsi_28nm_phy_disable,
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 2e942b10fffa..fba1ba88de01 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
@@ -648,14 +648,14 @@ static void dsi_28nm_phy_disable(struct msm_dsi_phy *phy)
 	wmb();
 }
 
+static const struct regulator_bulk_data dsi_phy_28nm_8960_regulators[] = {
+	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
+};
+
 const struct msm_dsi_phy_cfg dsi_phy_28nm_8960_cfgs = {
 	.has_phy_regulator = true,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vddio", 100000},	/* 1.8 V */
-		},
-	},
+	.regulator_data = dsi_phy_28nm_8960_regulators,
+	.num_regulators = ARRAY_SIZE(dsi_phy_28nm_8960_regulators),
 	.ops = {
 		.enable = dsi_28nm_phy_enable,
 		.disable = dsi_28nm_phy_disable,
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
index 9c7c49ce1200..cef801c58cdf 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
@@ -1036,14 +1036,18 @@ static void dsi_7nm_phy_disable(struct msm_dsi_phy *phy)
 	DBG("DSI%d PHY disabled", phy->id);
 }
 
+static const struct regulator_bulk_data dsi_phy_7nm_36mA_regulators[] = {
+	{ .supply = "vdds", .init_load_uA = 36000 },
+};
+
+static const struct regulator_bulk_data dsi_phy_7nm_37750uA_regulators[] = {
+	{ .supply = "vdds", .init_load_uA = 37550 },
+};
+
 const struct msm_dsi_phy_cfg dsi_phy_7nm_cfgs = {
 	.has_phy_lane = true,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vdds", 36000},
-		},
-	},
+	.regulator_data = dsi_phy_7nm_36mA_regulators,
+	.num_regulators = ARRAY_SIZE(dsi_phy_7nm_36mA_regulators),
 	.ops = {
 		.enable = dsi_7nm_phy_enable,
 		.disable = dsi_7nm_phy_disable,
@@ -1065,12 +1069,8 @@ const struct msm_dsi_phy_cfg dsi_phy_7nm_cfgs = {
 
 const struct msm_dsi_phy_cfg dsi_phy_7nm_8150_cfgs = {
 	.has_phy_lane = true,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vdds", 36000},
-		},
-	},
+	.regulator_data = dsi_phy_7nm_36mA_regulators,
+	.num_regulators = ARRAY_SIZE(dsi_phy_7nm_36mA_regulators),
 	.ops = {
 		.enable = dsi_7nm_phy_enable,
 		.disable = dsi_7nm_phy_disable,
@@ -1087,12 +1087,8 @@ const struct msm_dsi_phy_cfg dsi_phy_7nm_8150_cfgs = {
 
 const struct msm_dsi_phy_cfg dsi_phy_7nm_7280_cfgs = {
 	.has_phy_lane = true,
-	.reg_cfg = {
-		.num = 1,
-		.regs = {
-			{"vdds", 37550},
-		},
-	},
+	.regulator_data = dsi_phy_7nm_37750uA_regulators,
+	.num_regulators = ARRAY_SIZE(dsi_phy_7nm_37750uA_regulators),
 	.ops = {
 		.enable = dsi_7nm_phy_enable,
 		.disable = dsi_7nm_phy_disable,
-- 
2.37.1.455.g008518b4e5-goog

Re: [PATCH v3 5/6] drm/msm/dsi: Take advantage of devm_regulator_bulk_get_const()

[Dmitry Baryshkov]

On 03/08/2022 01:37, Douglas Anderson wrote:
> As of the commit 1de452a0edda ("regulator: core: Allow drivers to
> define their init data as const") we no longer need to do copying of
> regulator bulk data from initdata to something dynamic. Let's take
> advantage of that.
> 
> In addition to saving some code, this also moves us to using
> ARRAY_SIZE() to specify how many regulators we have which is less
> error prone.
> 
> This gets rid of some layers of wrappers which makes it obvious that
> we can get rid of an extra error print.
> devm_regulator_bulk_get_const() prints errors for you so you don't
> need an extra layer of printing.
> 
> In all cases here I have preserved the old settings without any
> investigation about whether the loads being set are sensible. In the
> cases of some of the PHYs if several PHYs in the same file used
> exactly the same settings I had them point to the same data structure.
> 
> NOTE: Though I haven't done the math, this is likely an overall
> savings in terms of "static const" data. We previously always
> allocated space for 8 supplies. Each of these supplies took up 36
> bytes of data (32 for name, 4 for an int).
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Ah, so to array conversion is already done. That's great.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


> ---
> 
> Changes in v3:
> - Do all the PHYs too.

It would have been easier if DSI and DSI PHY were split to separate patches.

> - Get rid of error print after devm_regulator_bulk_get_const().
> - Just directly call the bulk commands; get rid of the wrapper.
> - Update commit message to point at the git hash of the regulator change.
> 
> Changes in v2:
> - ("Take advantage of devm_regulator_bulk_get_const") new for v2.
> 
>   drivers/gpu/drm/msm/dsi/dsi.h                 |  12 --
>   drivers/gpu/drm/msm/dsi/dsi_cfg.c             | 172 +++++++++---------
>   drivers/gpu/drm/msm/dsi/dsi_cfg.h             |   3 +-
>   drivers/gpu/drm/msm/dsi/dsi_host.c            |  42 ++---
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c         |  37 +---
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.h         |   5 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c    |  20 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c    |  32 ++--
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c    |  14 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c    |  28 +--
>   .../gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c   |  12 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c     |  32 ++--
>   12 files changed, 167 insertions(+), 242 deletions(-)

-- 
With best wishes
Dmitry

Re: [PATCH v3 5/6] drm/msm/dsi: Take advantage of devm_regulator_bulk_get_const()

[Doug Anderson]

Hi,

On Wed, Aug 3, 2022 at 12:19 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 03/08/2022 01:37, Douglas Anderson wrote:
> > As of the commit 1de452a0edda ("regulator: core: Allow drivers to
> > define their init data as const") we no longer need to do copying of
> > regulator bulk data from initdata to something dynamic. Let's take
> > advantage of that.
> >
> > In addition to saving some code, this also moves us to using
> > ARRAY_SIZE() to specify how many regulators we have which is less
> > error prone.
> >
> > This gets rid of some layers of wrappers which makes it obvious that
> > we can get rid of an extra error print.
> > devm_regulator_bulk_get_const() prints errors for you so you don't
> > need an extra layer of printing.
> >
> > In all cases here I have preserved the old settings without any
> > investigation about whether the loads being set are sensible. In the
> > cases of some of the PHYs if several PHYs in the same file used
> > exactly the same settings I had them point to the same data structure.
> >
> > NOTE: Though I haven't done the math, this is likely an overall
> > savings in terms of "static const" data. We previously always
> > allocated space for 8 supplies. Each of these supplies took up 36
> > bytes of data (32 for name, 4 for an int).
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> Ah, so to array conversion is already done. That's great.
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
>
> > ---
> >
> > Changes in v3:
> > - Do all the PHYs too.
>
> It would have been easier if DSI and DSI PHY were split to separate patches.

One of the earlier patches in the series (where we remove the
"disable" load) was harder to split since the DSI and DSI PHY code was
sharing a single data structure. Once I had one patch touching both at
the same time I figured I'd keep them all like that. If you need me to
rework them to be separate patches to make it easier to land then
please yell. Otherwise I'll assume it's OK.

-Doug

Re: [PATCH v3 5/6] drm/msm/dsi: Take advantage of devm_regulator_bulk_get_const()

[Abhinav Kumar]

On 8/2/2022 3:37 PM, Douglas Anderson wrote:
> As of the commit 1de452a0edda ("regulator: core: Allow drivers to
> define their init data as const") we no longer need to do copying of
> regulator bulk data from initdata to something dynamic. Let's take
> advantage of that.
> 
> In addition to saving some code, this also moves us to using
> ARRAY_SIZE() to specify how many regulators we have which is less
> error prone.
> 
> This gets rid of some layers of wrappers which makes it obvious that
> we can get rid of an extra error print.
> devm_regulator_bulk_get_const() prints errors for you so you don't
> need an extra layer of printing.
> 
> In all cases here I have preserved the old settings without any
> investigation about whether the loads being set are sensible. In the
> cases of some of the PHYs if several PHYs in the same file used
> exactly the same settings I had them point to the same data structure.
> 
> NOTE: Though I haven't done the math, this is likely an overall
> savings in terms of "static const" data. We previously always
> allocated space for 8 supplies. Each of these supplies took up 36
> bytes of data (32 for name, 4 for an int).
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

LGTM, I am okay with DSI and DSI PHY being in one patch,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>


> ---
> 
> Changes in v3:
> - Do all the PHYs too.
> - Get rid of error print after devm_regulator_bulk_get_const().
> - Just directly call the bulk commands; get rid of the wrapper.
> - Update commit message to point at the git hash of the regulator change.
> 
> Changes in v2:
> - ("Take advantage of devm_regulator_bulk_get_const") new for v2.
> 
>   drivers/gpu/drm/msm/dsi/dsi.h                 |  12 --
>   drivers/gpu/drm/msm/dsi/dsi_cfg.c             | 172 +++++++++---------
>   drivers/gpu/drm/msm/dsi/dsi_cfg.h             |   3 +-
>   drivers/gpu/drm/msm/dsi/dsi_host.c            |  42 ++---
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c         |  37 +---
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.h         |   5 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c    |  20 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c    |  32 ++--
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c    |  14 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c    |  28 +--
>   .../gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c   |  12 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c     |  32 ++--
>   12 files changed, 167 insertions(+), 242 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index bb6a5bd05cb1..d661510d570d 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -30,20 +30,8 @@ enum msm_dsi_phy_usecase {
>   	MSM_DSI_PHY_SLAVE,
>   };
>   
> -#define DSI_DEV_REGULATOR_MAX	8
>   #define DSI_BUS_CLK_MAX		4
>   
> -/* Regulators for DSI devices */
> -struct dsi_reg_entry {
> -	char name[32];
> -	int enable_load;
> -};
> -
> -struct dsi_reg_config {
> -	int num;
> -	struct dsi_reg_entry regs[DSI_DEV_REGULATOR_MAX];
> -};
> -
>   struct msm_dsi {
>   	struct drm_device *dev;
>   	struct platform_device *pdev;
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index 901d6fd53800..7e97c239ed48 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -9,16 +9,16 @@ static const char * const dsi_v2_bus_clk_names[] = {
>   	"core_mmss", "iface", "bus",
>   };
>   
> +static const struct regulator_bulk_data apq8064_dsi_regulators[] = {
> +	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.2 V */
> +	{ .supply = "avdd", .init_load_uA = 10000 },	/* 3.0 V */
> +	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
> +};
> +
>   static const struct msm_dsi_config apq8064_dsi_cfg = {
>   	.io_offset = 0,
> -	.reg_cfg = {
> -		.num = 3,
> -		.regs = {
> -			{"vdda", 100000},	/* 1.2 V */
> -			{"avdd", 10000},	/* 3.0 V */
> -			{"vddio", 100000},	/* 1.8 V */
> -		},
> -	},
> +	.regulator_data = apq8064_dsi_regulators,
> +	.num_regulators = ARRAY_SIZE(apq8064_dsi_regulators),
>   	.bus_clk_names = dsi_v2_bus_clk_names,
>   	.num_bus_clks = ARRAY_SIZE(dsi_v2_bus_clk_names),
>   	.io_start = { 0x4700000, 0x5800000 },
> @@ -29,16 +29,16 @@ static const char * const dsi_6g_bus_clk_names[] = {
>   	"mdp_core", "iface", "bus", "core_mmss",
>   };
>   
> +static const struct regulator_bulk_data msm8974_apq8084_regulators[] = {
> +	{ .supply = "vdd", .init_load_uA = 150000 },	/* 3.0 V */
> +	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.2 V */
> +	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
> +};
> +
>   static const struct msm_dsi_config msm8974_apq8084_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
> -	.reg_cfg = {
> -		.num = 3,
> -		.regs = {
> -			{"vdd", 150000},	/* 3.0 V */
> -			{"vdda", 100000},	/* 1.2 V */
> -			{"vddio", 100000},	/* 1.8 V */
> -		},
> -	},
> +	.regulator_data = msm8974_apq8084_regulators,
> +	.num_regulators = ARRAY_SIZE(msm8974_apq8084_regulators),
>   	.bus_clk_names = dsi_6g_bus_clk_names,
>   	.num_bus_clks = ARRAY_SIZE(dsi_6g_bus_clk_names),
>   	.io_start = { 0xfd922800, 0xfd922b00 },
> @@ -49,15 +49,15 @@ static const char * const dsi_8916_bus_clk_names[] = {
>   	"mdp_core", "iface", "bus",
>   };
>   
> +static const struct regulator_bulk_data msm8916_dsi_regulators[] = {
> +	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.2 V */
> +	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
> +};
> +
>   static const struct msm_dsi_config msm8916_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
> -	.reg_cfg = {
> -		.num = 2,
> -		.regs = {
> -			{"vdda", 100000},	/* 1.2 V */
> -			{"vddio", 100000},	/* 1.8 V */
> -		},
> -	},
> +	.regulator_data = msm8916_dsi_regulators,
> +	.num_regulators = ARRAY_SIZE(msm8916_dsi_regulators),
>   	.bus_clk_names = dsi_8916_bus_clk_names,
>   	.num_bus_clks = ARRAY_SIZE(dsi_8916_bus_clk_names),
>   	.io_start = { 0x1a98000 },
> @@ -68,34 +68,34 @@ static const char * const dsi_8976_bus_clk_names[] = {
>   	"mdp_core", "iface", "bus",
>   };
>   
> +static const struct regulator_bulk_data msm8976_dsi_regulators[] = {
> +	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.2 V */
> +	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
> +};
> +
>   static const struct msm_dsi_config msm8976_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
> -	.reg_cfg = {
> -		.num = 2,
> -		.regs = {
> -			{"vdda", 100000},	/* 1.2 V */
> -			{"vddio", 100000},	/* 1.8 V */
> -		},
> -	},
> +	.regulator_data = msm8976_dsi_regulators,
> +	.num_regulators = ARRAY_SIZE(msm8976_dsi_regulators),
>   	.bus_clk_names = dsi_8976_bus_clk_names,
>   	.num_bus_clks = ARRAY_SIZE(dsi_8976_bus_clk_names),
>   	.io_start = { 0x1a94000, 0x1a96000 },
>   	.num_dsi = 2,
>   };
>   
> +static const struct regulator_bulk_data msm8994_dsi_regulators[] = {
> +	{ .supply = "vdda", .init_load_uA = 100000 },	/* 1.25 V */
> +	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
> +	{ .supply = "vcca", .init_load_uA = 10000 },	/* 1.0 V */
> +	{ .supply = "vdd", .init_load_uA = 100000 },	/* 1.8 V */
> +	{ .supply = "lab_reg", .init_load_uA = -1 },
> +	{ .supply = "ibb_reg", .init_load_uA = -1 },
> +};
> +
>   static const struct msm_dsi_config msm8994_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
> -	.reg_cfg = {
> -		.num = 6,
> -		.regs = {
> -			{"vdda", 100000},	/* 1.25 V */
> -			{"vddio", 100000},	/* 1.8 V */
> -			{"vcca", 10000},	/* 1.0 V */
> -			{"vdd", 100000},	/* 1.8 V */
> -			{"lab_reg", -1},
> -			{"ibb_reg", -1},
> -		},
> -	},
> +	.regulator_data = msm8994_dsi_regulators,
> +	.num_regulators = ARRAY_SIZE(msm8994_dsi_regulators),
>   	.bus_clk_names = dsi_6g_bus_clk_names,
>   	.num_bus_clks = ARRAY_SIZE(dsi_6g_bus_clk_names),
>   	.io_start = { 0xfd998000, 0xfd9a0000 },
> @@ -106,16 +106,16 @@ static const char * const dsi_8996_bus_clk_names[] = {
>   	"mdp_core", "iface", "bus", "core_mmss",
>   };
>   
> +static const struct regulator_bulk_data msm8996_dsi_regulators[] = {
> +	{ .supply = "vdda", .init_load_uA = 18160 },	/* 1.25 V */
> +	{ .supply = "vcca", .init_load_uA = 17000 },	/* 0.925 V */
> +	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
> +};
> +
>   static const struct msm_dsi_config msm8996_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
> -	.reg_cfg = {
> -		.num = 3,
> -		.regs = {
> -			{"vdda", 18160},	/* 1.25 V */
> -			{"vcca", 17000},	/* 0.925 V */
> -			{"vddio", 100000},/* 1.8 V */
> -		},
> -	},
> +	.regulator_data = msm8996_dsi_regulators,
> +	.num_regulators = ARRAY_SIZE(msm8996_dsi_regulators),
>   	.bus_clk_names = dsi_8996_bus_clk_names,
>   	.num_bus_clks = ARRAY_SIZE(dsi_8996_bus_clk_names),
>   	.io_start = { 0x994000, 0x996000 },
> @@ -126,15 +126,15 @@ static const char * const dsi_msm8998_bus_clk_names[] = {
>   	"iface", "bus", "core",
>   };
>   
> +static const struct regulator_bulk_data msm8998_dsi_regulators[] = {
> +	{ .supply = "vdd", .init_load_uA = 367000 },	/* 0.9 V */
> +	{ .supply = "vdda", .init_load_uA = 62800 },	/* 1.2 V */
> +};
> +
>   static const struct msm_dsi_config msm8998_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
> -	.reg_cfg = {
> -		.num = 2,
> -		.regs = {
> -			{"vdd", 367000},	/* 0.9 V */
> -			{"vdda", 62800},	/* 1.2 V */
> -		},
> -	},
> +	.regulator_data = msm8998_dsi_regulators,
> +	.num_regulators = ARRAY_SIZE(msm8998_dsi_regulators),
>   	.bus_clk_names = dsi_msm8998_bus_clk_names,
>   	.num_bus_clks = ARRAY_SIZE(dsi_msm8998_bus_clk_names),
>   	.io_start = { 0xc994000, 0xc996000 },
> @@ -145,14 +145,14 @@ static const char * const dsi_sdm660_bus_clk_names[] = {
>   	"iface", "bus", "core", "core_mmss",
>   };
>   
> +static const struct regulator_bulk_data sdm660_dsi_regulators[] = {
> +	{ .supply = "vdda", .init_load_uA = 12560 },	/* 1.2 V */
> +};
> +
>   static const struct msm_dsi_config sdm660_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vdda", 12560},	/* 1.2 V */
> -		},
> -	},
> +	.regulator_data = sdm660_dsi_regulators,
> +	.num_regulators = ARRAY_SIZE(sdm660_dsi_regulators),
>   	.bus_clk_names = dsi_sdm660_bus_clk_names,
>   	.num_bus_clks = ARRAY_SIZE(dsi_sdm660_bus_clk_names),
>   	.io_start = { 0xc994000, 0xc996000 },
> @@ -167,28 +167,28 @@ static const char * const dsi_sc7180_bus_clk_names[] = {
>   	"iface", "bus",
>   };
>   
> +static const struct regulator_bulk_data sdm845_dsi_regulators[] = {
> +	{ .supply = "vdda", .init_load_uA = 21800 },	/* 1.2 V */
> +};
> +
>   static const struct msm_dsi_config sdm845_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vdda", 21800},	/* 1.2 V */
> -		},
> -	},
> +	.regulator_data = sdm845_dsi_regulators,
> +	.num_regulators = ARRAY_SIZE(sdm845_dsi_regulators),
>   	.bus_clk_names = dsi_sdm845_bus_clk_names,
>   	.num_bus_clks = ARRAY_SIZE(dsi_sdm845_bus_clk_names),
>   	.io_start = { 0xae94000, 0xae96000 },
>   	.num_dsi = 2,
>   };
>   
> +static const struct regulator_bulk_data sc7180_dsi_regulators[] = {
> +	{ .supply = "vdda", .init_load_uA = 21800 },	/* 1.2 V */
> +};
> +
>   static const struct msm_dsi_config sc7180_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vdda", 21800},	/* 1.2 V */
> -		},
> -	},
> +	.regulator_data = sc7180_dsi_regulators,
> +	.num_regulators = ARRAY_SIZE(sc7180_dsi_regulators),
>   	.bus_clk_names = dsi_sc7180_bus_clk_names,
>   	.num_bus_clks = ARRAY_SIZE(dsi_sc7180_bus_clk_names),
>   	.io_start = { 0xae94000 },
> @@ -199,14 +199,14 @@ static const char * const dsi_sc7280_bus_clk_names[] = {
>   	"iface", "bus",
>   };
>   
> +static const struct regulator_bulk_data sc7280_dsi_regulators[] = {
> +	{ .supply = "vdda", .init_load_uA = 8350 },	/* 1.2 V */
> +};
> +
>   static const struct msm_dsi_config sc7280_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vdda", 8350},	/* 1.2 V */
> -		},
> -	},
> +	.regulator_data = sc7280_dsi_regulators,
> +	.num_regulators = ARRAY_SIZE(sc7280_dsi_regulators),
>   	.bus_clk_names = dsi_sc7280_bus_clk_names,
>   	.num_bus_clks = ARRAY_SIZE(dsi_sc7280_bus_clk_names),
>   	.io_start = { 0xae94000 },
> @@ -217,14 +217,14 @@ static const char * const dsi_qcm2290_bus_clk_names[] = {
>   	"iface", "bus",
>   };
>   
> +static const struct regulator_bulk_data qcm2290_dsi_cfg_regulators[] = {
> +	{ .supply = "vdda", .init_load_uA = 21800 },	/* 1.2 V */
> +};
> +
>   static const struct msm_dsi_config qcm2290_dsi_cfg = {
>   	.io_offset = DSI_6G_REG_SHIFT,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vdda", 21800},	/* 1.2 V */
> -		},
> -	},
> +	.regulator_data = qcm2290_dsi_cfg_regulators,
> +	.num_regulators = ARRAY_SIZE(qcm2290_dsi_cfg_regulators),
>   	.bus_clk_names = dsi_qcm2290_bus_clk_names,
>   	.num_bus_clks = ARRAY_SIZE(dsi_qcm2290_bus_clk_names),
>   	.io_start = { 0x5e94000 },
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> index fe54a999968b..8f04e685a74e 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> @@ -32,7 +32,8 @@
>   
>   struct msm_dsi_config {
>   	u32 io_offset;
> -	struct dsi_reg_config reg_cfg;
> +	const struct regulator_bulk_data *regulator_data;
> +	int num_regulators;
>   	const char * const *bus_clk_names;
>   	const int num_bus_clks;
>   	const resource_size_t io_start[DSI_MAX];
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index a0a1b6d61d05..3364b2affac5 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -106,7 +106,7 @@ struct msm_dsi_host {
>   
>   	void __iomem *ctrl_base;
>   	phys_addr_t ctrl_size;
> -	struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];
> +	struct regulator_bulk_data *supplies;
>   
>   	int num_bus_clks;
>   	struct clk_bulk_data bus_clks[DSI_BUS_CLK_MAX];
> @@ -253,28 +253,6 @@ static inline struct msm_dsi_host *to_msm_dsi_host(struct mipi_dsi_host *host)
>   	return container_of(host, struct msm_dsi_host, base);
>   }
>   
> -static int dsi_regulator_init(struct msm_dsi_host *msm_host)
> -{
> -	struct regulator_bulk_data *s = msm_host->supplies;
> -	const struct dsi_reg_entry *regs = msm_host->cfg_hnd->cfg->reg_cfg.regs;
> -	int num = msm_host->cfg_hnd->cfg->reg_cfg.num;
> -	int i, ret;
> -
> -	for (i = 0; i < num; i++) {
> -		s[i].supply = regs[i].name;
> -		s[i].init_load_uA = regs[i].enable_load;
> -	}
> -
> -	ret = devm_regulator_bulk_get(&msm_host->pdev->dev, num, s);
> -	if (ret < 0) {
> -		pr_err("%s: failed to init regulator, ret=%d\n",
> -						__func__, ret);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>   int dsi_clk_init_v2(struct msm_dsi_host *msm_host)
>   {
>   	struct platform_device *pdev = msm_host->pdev;
> @@ -1982,6 +1960,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
>   {
>   	struct msm_dsi_host *msm_host = NULL;
>   	struct platform_device *pdev = msm_dsi->pdev;
> +	const struct msm_dsi_config *cfg;
>   	int ret;
>   
>   	msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL);
> @@ -2014,6 +1993,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
>   		pr_err("%s: get config failed\n", __func__);
>   		goto fail;
>   	}
> +	cfg = msm_host->cfg_hnd->cfg;
>   
>   	msm_host->id = dsi_host_get_id(msm_host);
>   	if (msm_host->id < 0) {
> @@ -2023,13 +2003,13 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
>   	}
>   
>   	/* fixup base address by io offset */
> -	msm_host->ctrl_base += msm_host->cfg_hnd->cfg->io_offset;
> +	msm_host->ctrl_base += cfg->io_offset;
>   
> -	ret = dsi_regulator_init(msm_host);
> -	if (ret) {
> -		pr_err("%s: regulator init failed\n", __func__);
> +	ret = devm_regulator_bulk_get_const(&pdev->dev, cfg->num_regulators,
> +					    cfg->regulator_data,
> +					    &msm_host->supplies);
> +	if (ret)
>   		goto fail;
> -	}
>   
>   	ret = dsi_clk_init(msm_host);
>   	if (ret) {
> @@ -2510,7 +2490,7 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
>   
>   	msm_dsi_sfpb_config(msm_host, true);
>   
> -	ret = regulator_bulk_enable(msm_host->cfg_hnd->cfg->reg_cfg.num,
> +	ret = regulator_bulk_enable(msm_host->cfg_hnd->cfg->num_regulators,
>   				    msm_host->supplies);
>   	if (ret) {
>   		pr_err("%s:Failed to enable vregs.ret=%d\n",
> @@ -2551,7 +2531,7 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
>   	cfg_hnd->ops->link_clk_disable(msm_host);
>   	pm_runtime_put(&msm_host->pdev->dev);
>   fail_disable_reg:
> -	regulator_bulk_disable(msm_host->cfg_hnd->cfg->reg_cfg.num,
> +	regulator_bulk_disable(msm_host->cfg_hnd->cfg->num_regulators,
>   			       msm_host->supplies);
>   unlock_ret:
>   	mutex_unlock(&msm_host->dev_mutex);
> @@ -2579,7 +2559,7 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host)
>   	cfg_hnd->ops->link_clk_disable(msm_host);
>   	pm_runtime_put(&msm_host->pdev->dev);
>   
> -	regulator_bulk_disable(msm_host->cfg_hnd->cfg->reg_cfg.num,
> +	regulator_bulk_disable(msm_host->cfg_hnd->cfg->num_regulators,
>   			       msm_host->supplies);
>   
>   	msm_dsi_sfpb_config(msm_host, false);
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> index efb6b1726cdb..0a00f9b73fc5 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> @@ -507,33 +507,6 @@ int msm_dsi_cphy_timing_calc_v4(struct msm_dsi_dphy_timing *timing,
>   	return 0;
>   }
>   
> -static int dsi_phy_regulator_init(struct msm_dsi_phy *phy)
> -{
> -	struct regulator_bulk_data *s = phy->supplies;
> -	const struct dsi_reg_entry *regs = phy->cfg->reg_cfg.regs;
> -	struct device *dev = &phy->pdev->dev;
> -	int num = phy->cfg->reg_cfg.num;
> -	int i, ret;
> -
> -	for (i = 0; i < num; i++) {
> -		s[i].supply = regs[i].name;
> -		s[i].init_load_uA = regs[i].enable_load;
> -	}
> -
> -	ret = devm_regulator_bulk_get(dev, num, s);
> -	if (ret < 0) {
> -		if (ret != -EPROBE_DEFER) {
> -			DRM_DEV_ERROR(dev,
> -				      "%s: failed to init regulator, ret=%d\n",
> -				      __func__, ret);
> -		}
> -
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>   static int dsi_phy_enable_resource(struct msm_dsi_phy *phy)
>   {
>   	struct device *dev = &phy->pdev->dev;
> @@ -698,7 +671,9 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
>   			goto fail;
>   	}
>   
> -	ret = dsi_phy_regulator_init(phy);
> +	ret = devm_regulator_bulk_get_const(dev, phy->cfg->num_regulators,
> +					    phy->cfg->regulator_data,
> +					    &phy->supplies);
>   	if (ret)
>   		goto fail;
>   
> @@ -780,7 +755,7 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
>   		goto res_en_fail;
>   	}
>   
> -	ret = regulator_bulk_enable(phy->cfg->reg_cfg.num, phy->supplies);
> +	ret = regulator_bulk_enable(phy->cfg->num_regulators, phy->supplies);
>   	if (ret) {
>   		DRM_DEV_ERROR(dev, "%s: regulator enable failed, %d\n",
>   			__func__, ret);
> @@ -817,7 +792,7 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
>   	if (phy->cfg->ops.disable)
>   		phy->cfg->ops.disable(phy);
>   phy_en_fail:
> -	regulator_bulk_disable(phy->cfg->reg_cfg.num, phy->supplies);
> +	regulator_bulk_disable(phy->cfg->num_regulators, phy->supplies);
>   reg_en_fail:
>   	dsi_phy_disable_resource(phy);
>   res_en_fail:
> @@ -831,7 +806,7 @@ void msm_dsi_phy_disable(struct msm_dsi_phy *phy)
>   
>   	phy->cfg->ops.disable(phy);
>   
> -	regulator_bulk_disable(phy->cfg->reg_cfg.num, phy->supplies);
> +	regulator_bulk_disable(phy->cfg->num_regulators, phy->supplies);
>   	dsi_phy_disable_resource(phy);
>   }
>   
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> index dc91b43d5a38..60a99c6525b2 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> @@ -29,7 +29,8 @@ struct msm_dsi_phy_ops {
>   };
>   
>   struct msm_dsi_phy_cfg {
> -	struct dsi_reg_config reg_cfg;
> +	const struct regulator_bulk_data *regulator_data;
> +	int num_regulators;
>   	struct msm_dsi_phy_ops ops;
>   
>   	unsigned long	min_pll_rate;
> @@ -98,7 +99,7 @@ struct msm_dsi_phy {
>   	int id;
>   
>   	struct clk *ahb_clk;
> -	struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];
> +	struct regulator_bulk_data *supplies;
>   
>   	struct msm_dsi_dphy_timing timing;
>   	const struct msm_dsi_phy_cfg *cfg;
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> index 6a10a1448051..e34a2274db87 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> @@ -1028,14 +1028,14 @@ static int dsi_10nm_phy_parse_dt(struct msm_dsi_phy *phy)
>   	return 0;
>   }
>   
> +static const struct regulator_bulk_data dsi_phy_10nm_regulators[] = {
> +	{ .supply = "vdds", .init_load_uA = 36000 },
> +};
> +
>   const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs = {
>   	.has_phy_lane = true,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vdds", 36000},
> -		},
> -	},
> +	.regulator_data = dsi_phy_10nm_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_phy_10nm_regulators),
>   	.ops = {
>   		.enable = dsi_10nm_phy_enable,
>   		.disable = dsi_10nm_phy_disable,
> @@ -1052,12 +1052,8 @@ const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs = {
>   
>   const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs = {
>   	.has_phy_lane = true,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vdds", 36000},
> -		},
> -	},
> +	.regulator_data = dsi_phy_10nm_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_phy_10nm_regulators),
>   	.ops = {
>   		.enable = dsi_10nm_phy_enable,
>   		.disable = dsi_10nm_phy_disable,
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
> index 0f3d4c56c333..be1500368d95 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
> @@ -1024,14 +1024,18 @@ static void dsi_14nm_phy_disable(struct msm_dsi_phy *phy)
>   	wmb();
>   }
>   
> +static const struct regulator_bulk_data dsi_phy_14nm_17mA_regulators[] = {
> +	{ .supply = "vcca", .init_load_uA = 17000 },
> +};
> +
> +static const struct regulator_bulk_data dsi_phy_14nm_73p4mA_regulators[] = {
> +	{ .supply = "vcca", .init_load_uA = 73400 },
> +};
> +
>   const struct msm_dsi_phy_cfg dsi_phy_14nm_cfgs = {
>   	.has_phy_lane = true,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vcca", 17000},
> -		},
> -	},
> +	.regulator_data = dsi_phy_14nm_17mA_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_phy_14nm_17mA_regulators),
>   	.ops = {
>   		.enable = dsi_14nm_phy_enable,
>   		.disable = dsi_14nm_phy_disable,
> @@ -1047,12 +1051,8 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_cfgs = {
>   
>   const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs = {
>   	.has_phy_lane = true,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vcca", 73400},
> -		},
> -	},
> +	.regulator_data = dsi_phy_14nm_73p4mA_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_phy_14nm_73p4mA_regulators),
>   	.ops = {
>   		.enable = dsi_14nm_phy_enable,
>   		.disable = dsi_14nm_phy_disable,
> @@ -1068,12 +1068,8 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs = {
>   
>   const struct msm_dsi_phy_cfg dsi_phy_14nm_8953_cfgs = {
>   	.has_phy_lane = true,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vcca", 17000},
> -		},
> -	},
> +	.regulator_data = dsi_phy_14nm_17mA_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_phy_14nm_17mA_regulators),
>   	.ops = {
>   		.enable = dsi_14nm_phy_enable,
>   		.disable = dsi_14nm_phy_disable,
> 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 b7c621d94981..c9752b991744 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c
> @@ -129,15 +129,15 @@ static void dsi_20nm_phy_disable(struct msm_dsi_phy *phy)
>   	dsi_20nm_phy_regulator_ctrl(phy, false);
>   }
>   
> +static const struct regulator_bulk_data dsi_phy_20nm_regulators[] = {
> +	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
> +	{ .supply = "vcca", .init_load_uA = 10000 },	/* 1.0 V */
> +};
> +
>   const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs = {
>   	.has_phy_regulator = true,
> -	.reg_cfg = {
> -		.num = 2,
> -		.regs = {
> -			{"vddio", 100000},	/* 1.8 V */
> -			{"vcca", 10000},	/* 1.0 V */
> -		},
> -	},
> +	.regulator_data = dsi_phy_20nm_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_phy_20nm_regulators),
>   	.ops = {
>   		.enable = dsi_20nm_phy_enable,
>   		.disable = dsi_20nm_phy_disable,
> 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 6beba387640d..578c671a3bb1 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
> @@ -769,14 +769,14 @@ static void dsi_28nm_phy_disable(struct msm_dsi_phy *phy)
>   	wmb();
>   }
>   
> +static const struct regulator_bulk_data dsi_phy_28nm_regulators[] = {
> +	{ .supply = "vddio", .init_load_uA = 100000 },
> +};
> +
>   const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs = {
>   	.has_phy_regulator = true,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vddio", 100000},
> -		},
> -	},
> +	.regulator_data = dsi_phy_28nm_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_phy_28nm_regulators),
>   	.ops = {
>   		.enable = dsi_28nm_phy_enable,
>   		.disable = dsi_28nm_phy_disable,
> @@ -792,12 +792,8 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs = {
>   
>   const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_famb_cfgs = {
>   	.has_phy_regulator = true,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vddio", 100000},
> -		},
> -	},
> +	.regulator_data = dsi_phy_28nm_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_phy_28nm_regulators),
>   	.ops = {
>   		.enable = dsi_28nm_phy_enable,
>   		.disable = dsi_28nm_phy_disable,
> @@ -813,12 +809,8 @@ const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_famb_cfgs = {
>   
>   const struct msm_dsi_phy_cfg dsi_phy_28nm_lp_cfgs = {
>   	.has_phy_regulator = true,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vddio", 100000},	/* 1.8 V */
> -		},
> -	},
> +	.regulator_data = dsi_phy_28nm_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_phy_28nm_regulators),
>   	.ops = {
>   		.enable = dsi_28nm_phy_enable,
>   		.disable = dsi_28nm_phy_disable,
> 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 2e942b10fffa..fba1ba88de01 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
> @@ -648,14 +648,14 @@ static void dsi_28nm_phy_disable(struct msm_dsi_phy *phy)
>   	wmb();
>   }
>   
> +static const struct regulator_bulk_data dsi_phy_28nm_8960_regulators[] = {
> +	{ .supply = "vddio", .init_load_uA = 100000 },	/* 1.8 V */
> +};
> +
>   const struct msm_dsi_phy_cfg dsi_phy_28nm_8960_cfgs = {
>   	.has_phy_regulator = true,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vddio", 100000},	/* 1.8 V */
> -		},
> -	},
> +	.regulator_data = dsi_phy_28nm_8960_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_phy_28nm_8960_regulators),
>   	.ops = {
>   		.enable = dsi_28nm_phy_enable,
>   		.disable = dsi_28nm_phy_disable,
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> index 9c7c49ce1200..cef801c58cdf 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> @@ -1036,14 +1036,18 @@ static void dsi_7nm_phy_disable(struct msm_dsi_phy *phy)
>   	DBG("DSI%d PHY disabled", phy->id);
>   }
>   
> +static const struct regulator_bulk_data dsi_phy_7nm_36mA_regulators[] = {
> +	{ .supply = "vdds", .init_load_uA = 36000 },
> +};
> +
> +static const struct regulator_bulk_data dsi_phy_7nm_37750uA_regulators[] = {
> +	{ .supply = "vdds", .init_load_uA = 37550 },
> +};
> +
>   const struct msm_dsi_phy_cfg dsi_phy_7nm_cfgs = {
>   	.has_phy_lane = true,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vdds", 36000},
> -		},
> -	},
> +	.regulator_data = dsi_phy_7nm_36mA_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_phy_7nm_36mA_regulators),
>   	.ops = {
>   		.enable = dsi_7nm_phy_enable,
>   		.disable = dsi_7nm_phy_disable,
> @@ -1065,12 +1069,8 @@ const struct msm_dsi_phy_cfg dsi_phy_7nm_cfgs = {
>   
>   const struct msm_dsi_phy_cfg dsi_phy_7nm_8150_cfgs = {
>   	.has_phy_lane = true,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vdds", 36000},
> -		},
> -	},
> +	.regulator_data = dsi_phy_7nm_36mA_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_phy_7nm_36mA_regulators),
>   	.ops = {
>   		.enable = dsi_7nm_phy_enable,
>   		.disable = dsi_7nm_phy_disable,
> @@ -1087,12 +1087,8 @@ const struct msm_dsi_phy_cfg dsi_phy_7nm_8150_cfgs = {
>   
>   const struct msm_dsi_phy_cfg dsi_phy_7nm_7280_cfgs = {
>   	.has_phy_lane = true,
> -	.reg_cfg = {
> -		.num = 1,
> -		.regs = {
> -			{"vdds", 37550},
> -		},
> -	},
> +	.regulator_data = dsi_phy_7nm_37750uA_regulators,
> +	.num_regulators = ARRAY_SIZE(dsi_phy_7nm_37750uA_regulators),
>   	.ops = {
>   		.enable = dsi_7nm_phy_enable,
>   		.disable = dsi_7nm_phy_disable,

[PATCH v3 6/6] drm/msm/dsi: Improve dsi_phy_driver_probe() probe error handling

[Douglas Anderson]

The dsi_phy_driver_probe() function has a "goto fail" for no
reason. Change it to just always return directly when it sees an
error. Make this simpler by leveraging dev_err_probe() which is
designed to make code like this shorter / simpler.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- ("Improve dsi_phy_driver_probe() probe error handling") new for v3.

 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 74 ++++++++++-----------------
 1 file changed, 27 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 0a00f9b73fc5..57cd525de7a1 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -621,12 +621,9 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
 	phy->pdev = pdev;
 
 	phy->id = dsi_phy_get_id(phy);
-	if (phy->id < 0) {
-		ret = phy->id;
-		DRM_DEV_ERROR(dev, "%s: couldn't identify PHY index, %d\n",
-			__func__, ret);
-		goto fail;
-	}
+	if (phy->id < 0)
+		return dev_err_probe(dev, phy->id,
+				     "Couldn't identify PHY index\n");
 
 	phy->regulator_ldo_mode = of_property_read_bool(dev->of_node,
 				"qcom,dsi-phy-regulator-ldo-mode");
@@ -634,88 +631,71 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
 		phy->cphy_mode = (phy_type == PHY_TYPE_CPHY);
 
 	phy->base = msm_ioremap_size(pdev, "dsi_phy", &phy->base_size);
-	if (IS_ERR(phy->base)) {
-		DRM_DEV_ERROR(dev, "%s: failed to map phy base\n", __func__);
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (IS_ERR(phy->base))
+		return dev_err_probe(dev, PTR_ERR(phy->base),
+				     "Failed to map phy base\n");
 
 	phy->pll_base = msm_ioremap_size(pdev, "dsi_pll", &phy->pll_size);
-	if (IS_ERR(phy->pll_base)) {
-		DRM_DEV_ERROR(&pdev->dev, "%s: failed to map pll base\n", __func__);
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (IS_ERR(phy->pll_base))
+		return dev_err_probe(dev, PTR_ERR(phy->pll_base),
+				     "Failed to map pll base\n");
 
 	if (phy->cfg->has_phy_lane) {
 		phy->lane_base = msm_ioremap_size(pdev, "dsi_phy_lane", &phy->lane_size);
-		if (IS_ERR(phy->lane_base)) {
-			DRM_DEV_ERROR(&pdev->dev, "%s: failed to map phy lane base\n", __func__);
-			ret = -ENOMEM;
-			goto fail;
-		}
+		if (IS_ERR(phy->lane_base))
+			return dev_err_probe(dev, PTR_ERR(phy->lane_base),
+					     "Failed to map phy lane base\n");
 	}
 
 	if (phy->cfg->has_phy_regulator) {
 		phy->reg_base = msm_ioremap_size(pdev, "dsi_phy_regulator", &phy->reg_size);
-		if (IS_ERR(phy->reg_base)) {
-			DRM_DEV_ERROR(&pdev->dev, "%s: failed to map phy regulator base\n", __func__);
-			ret = -ENOMEM;
-			goto fail;
-		}
+		if (IS_ERR(phy->reg_base))
+			return dev_err_probe(dev, PTR_ERR(phy->reg_base),
+					     "Failed to map phy regulator base\n");
 	}
 
 	if (phy->cfg->ops.parse_dt_properties) {
 		ret = phy->cfg->ops.parse_dt_properties(phy);
 		if (ret)
-			goto fail;
+			return ret;
 	}
 
 	ret = devm_regulator_bulk_get_const(dev, phy->cfg->num_regulators,
 					    phy->cfg->regulator_data,
 					    &phy->supplies);
 	if (ret)
-		goto fail;
+		return ret;
 
 	phy->ahb_clk = msm_clk_get(pdev, "iface");
-	if (IS_ERR(phy->ahb_clk)) {
-		DRM_DEV_ERROR(dev, "%s: Unable to get ahb clk\n", __func__);
-		ret = PTR_ERR(phy->ahb_clk);
-		goto fail;
-	}
+	if (IS_ERR(phy->ahb_clk))
+		return dev_err_probe(dev, PTR_ERR(phy->ahb_clk),
+				     "Unable to get ahb clk\n");
 
 	/* PLL init will call into clk_register which requires
 	 * register access, so we need to enable power and ahb clock.
 	 */
 	ret = dsi_phy_enable_resource(phy);
 	if (ret)
-		goto fail;
+		return ret;
 
 	if (phy->cfg->ops.pll_init) {
 		ret = phy->cfg->ops.pll_init(phy);
-		if (ret) {
-			DRM_DEV_INFO(dev,
-				"%s: pll init failed: %d, need separate pll clk driver\n",
-				__func__, ret);
-			goto fail;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "PLL init failed; need separate clk driver\n");
 	}
 
 	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
 				     phy->provided_clocks);
-	if (ret) {
-		DRM_DEV_ERROR(dev, "%s: failed to register clk provider: %d\n", __func__, ret);
-		goto fail;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to register clk provider\n");
 
 	dsi_phy_disable_resource(phy);
 
 	platform_set_drvdata(pdev, phy);
 
 	return 0;
-
-fail:
-	return ret;
 }
 
 static struct platform_driver dsi_phy_platform_driver = {
-- 
2.37.1.455.g008518b4e5-goog

Re: [PATCH v3 6/6] drm/msm/dsi: Improve dsi_phy_driver_probe() probe error handling

[Dmitry Baryshkov]

On 03/08/2022 01:37, Douglas Anderson wrote:
> The dsi_phy_driver_probe() function has a "goto fail" for no
> reason. Change it to just always return directly when it sees an
> error. Make this simpler by leveraging dev_err_probe() which is
> designed to make code like this shorter / simpler.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Minor nit below.

> ---
> 
> Changes in v3:
> - ("Improve dsi_phy_driver_probe() probe error handling") new for v3.
> 
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 74 ++++++++++-----------------
>   1 file changed, 27 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> index 0a00f9b73fc5..57cd525de7a1 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> @@ -621,12 +621,9 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
>   	phy->pdev = pdev;
>   
>   	phy->id = dsi_phy_get_id(phy);
> -	if (phy->id < 0) {
> -		ret = phy->id;
> -		DRM_DEV_ERROR(dev, "%s: couldn't identify PHY index, %d\n",
> -			__func__, ret);
> -		goto fail;
> -	}
> +	if (phy->id < 0)
> +		return dev_err_probe(dev, phy->id,
> +				     "Couldn't identify PHY index\n");
>   
>   	phy->regulator_ldo_mode = of_property_read_bool(dev->of_node,
>   				"qcom,dsi-phy-regulator-ldo-mode");
> @@ -634,88 +631,71 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
>   		phy->cphy_mode = (phy_type == PHY_TYPE_CPHY);
>   
>   	phy->base = msm_ioremap_size(pdev, "dsi_phy", &phy->base_size);
> -	if (IS_ERR(phy->base)) {
> -		DRM_DEV_ERROR(dev, "%s: failed to map phy base\n", __func__);
> -		ret = -ENOMEM;

Here (and in a few cases later) this changes the error code from crafted 
-ENOMEM to the proper one returned by msm_ioremap_size(). This should be 
mentioned in the commit message.

> -		goto fail;
> -	}
> +	if (IS_ERR(phy->base))
> +		return dev_err_probe(dev, PTR_ERR(phy->base),
> +				     "Failed to map phy base\n");
>   
>   	phy->pll_base = msm_ioremap_size(pdev, "dsi_pll", &phy->pll_size);
> -	if (IS_ERR(phy->pll_base)) {
> -		DRM_DEV_ERROR(&pdev->dev, "%s: failed to map pll base\n", __func__);
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> +	if (IS_ERR(phy->pll_base))
> +		return dev_err_probe(dev, PTR_ERR(phy->pll_base),
> +				     "Failed to map pll base\n");
>   
>   	if (phy->cfg->has_phy_lane) {
>   		phy->lane_base = msm_ioremap_size(pdev, "dsi_phy_lane", &phy->lane_size);
> -		if (IS_ERR(phy->lane_base)) {
> -			DRM_DEV_ERROR(&pdev->dev, "%s: failed to map phy lane base\n", __func__);
> -			ret = -ENOMEM;
> -			goto fail;
> -		}
> +		if (IS_ERR(phy->lane_base))
> +			return dev_err_probe(dev, PTR_ERR(phy->lane_base),
> +					     "Failed to map phy lane base\n");
>   	}
>   
>   	if (phy->cfg->has_phy_regulator) {
>   		phy->reg_base = msm_ioremap_size(pdev, "dsi_phy_regulator", &phy->reg_size);
> -		if (IS_ERR(phy->reg_base)) {
> -			DRM_DEV_ERROR(&pdev->dev, "%s: failed to map phy regulator base\n", __func__);
> -			ret = -ENOMEM;
> -			goto fail;
> -		}
> +		if (IS_ERR(phy->reg_base))
> +			return dev_err_probe(dev, PTR_ERR(phy->reg_base),
> +					     "Failed to map phy regulator base\n");
>   	}
>   
>   	if (phy->cfg->ops.parse_dt_properties) {
>   		ret = phy->cfg->ops.parse_dt_properties(phy);
>   		if (ret)
> -			goto fail;
> +			return ret;
>   	}
>   
>   	ret = devm_regulator_bulk_get_const(dev, phy->cfg->num_regulators,
>   					    phy->cfg->regulator_data,
>   					    &phy->supplies);
>   	if (ret)
> -		goto fail;
> +		return ret;
>   
>   	phy->ahb_clk = msm_clk_get(pdev, "iface");
> -	if (IS_ERR(phy->ahb_clk)) {
> -		DRM_DEV_ERROR(dev, "%s: Unable to get ahb clk\n", __func__);
> -		ret = PTR_ERR(phy->ahb_clk);
> -		goto fail;
> -	}
> +	if (IS_ERR(phy->ahb_clk))
> +		return dev_err_probe(dev, PTR_ERR(phy->ahb_clk),
> +				     "Unable to get ahb clk\n");
>   
>   	/* PLL init will call into clk_register which requires
>   	 * register access, so we need to enable power and ahb clock.
>   	 */
>   	ret = dsi_phy_enable_resource(phy);
>   	if (ret)
> -		goto fail;
> +		return ret;
>   
>   	if (phy->cfg->ops.pll_init) {
>   		ret = phy->cfg->ops.pll_init(phy);
> -		if (ret) {
> -			DRM_DEV_INFO(dev,
> -				"%s: pll init failed: %d, need separate pll clk driver\n",
> -				__func__, ret);
> -			goto fail;
> -		}
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "PLL init failed; need separate clk driver\n");
>   	}
>   
>   	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>   				     phy->provided_clocks);
> -	if (ret) {
> -		DRM_DEV_ERROR(dev, "%s: failed to register clk provider: %d\n", __func__, ret);
> -		goto fail;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register clk provider\n");
>   
>   	dsi_phy_disable_resource(phy);
>   
>   	platform_set_drvdata(pdev, phy);
>   
>   	return 0;
> -
> -fail:
> -	return ret;
>   }
>   
>   static struct platform_driver dsi_phy_platform_driver = {


-- 
With best wishes
Dmitry

Re: [PATCH v3 6/6] drm/msm/dsi: Improve dsi_phy_driver_probe() probe error handling

[Doug Anderson]

Hi,

On Wed, Aug 3, 2022 at 12:32 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> > @@ -634,88 +631,71 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
> >               phy->cphy_mode = (phy_type == PHY_TYPE_CPHY);
> >
> >       phy->base = msm_ioremap_size(pdev, "dsi_phy", &phy->base_size);
> > -     if (IS_ERR(phy->base)) {
> > -             DRM_DEV_ERROR(dev, "%s: failed to map phy base\n", __func__);
> > -             ret = -ENOMEM;
>
> Here (and in a few cases later) this changes the error code from crafted
> -ENOMEM to the proper one returned by msm_ioremap_size(). This should be
> mentioned in the commit message.

Good point. Unless something comes up I'll plan to spin a v4 with this
change to the commit message tomorrow.

-Doug

Re: [PATCH v3 6/6] drm/msm/dsi: Improve dsi_phy_driver_probe() probe error handling

[Abhinav Kumar]

On 8/2/2022 3:37 PM, Douglas Anderson wrote:
> The dsi_phy_driver_probe() function has a "goto fail" for no
> reason. Change it to just always return directly when it sees an
> error. Make this simpler by leveraging dev_err_probe() which is
> designed to make code like this shorter / simpler.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Yes, apart from Dmitry's suggestion of updating the commit text about 
the return value change, this is:

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> 
> Changes in v3:
> - ("Improve dsi_phy_driver_probe() probe error handling") new for v3.
> 
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 74 ++++++++++-----------------
>   1 file changed, 27 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> index 0a00f9b73fc5..57cd525de7a1 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> @@ -621,12 +621,9 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
>   	phy->pdev = pdev;
>   
>   	phy->id = dsi_phy_get_id(phy);
> -	if (phy->id < 0) {
> -		ret = phy->id;
> -		DRM_DEV_ERROR(dev, "%s: couldn't identify PHY index, %d\n",
> -			__func__, ret);
> -		goto fail;
> -	}
> +	if (phy->id < 0)
> +		return dev_err_probe(dev, phy->id,
> +				     "Couldn't identify PHY index\n");
>   
>   	phy->regulator_ldo_mode = of_property_read_bool(dev->of_node,
>   				"qcom,dsi-phy-regulator-ldo-mode");
> @@ -634,88 +631,71 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
>   		phy->cphy_mode = (phy_type == PHY_TYPE_CPHY);
>   
>   	phy->base = msm_ioremap_size(pdev, "dsi_phy", &phy->base_size);
> -	if (IS_ERR(phy->base)) {
> -		DRM_DEV_ERROR(dev, "%s: failed to map phy base\n", __func__);
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> +	if (IS_ERR(phy->base))
> +		return dev_err_probe(dev, PTR_ERR(phy->base),
> +				     "Failed to map phy base\n");
>   
>   	phy->pll_base = msm_ioremap_size(pdev, "dsi_pll", &phy->pll_size);
> -	if (IS_ERR(phy->pll_base)) {
> -		DRM_DEV_ERROR(&pdev->dev, "%s: failed to map pll base\n", __func__);
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> +	if (IS_ERR(phy->pll_base))
> +		return dev_err_probe(dev, PTR_ERR(phy->pll_base),
> +				     "Failed to map pll base\n");
>   
>   	if (phy->cfg->has_phy_lane) {
>   		phy->lane_base = msm_ioremap_size(pdev, "dsi_phy_lane", &phy->lane_size);
> -		if (IS_ERR(phy->lane_base)) {
> -			DRM_DEV_ERROR(&pdev->dev, "%s: failed to map phy lane base\n", __func__);
> -			ret = -ENOMEM;
> -			goto fail;
> -		}
> +		if (IS_ERR(phy->lane_base))
> +			return dev_err_probe(dev, PTR_ERR(phy->lane_base),
> +					     "Failed to map phy lane base\n");
>   	}
>   
>   	if (phy->cfg->has_phy_regulator) {
>   		phy->reg_base = msm_ioremap_size(pdev, "dsi_phy_regulator", &phy->reg_size);
> -		if (IS_ERR(phy->reg_base)) {
> -			DRM_DEV_ERROR(&pdev->dev, "%s: failed to map phy regulator base\n", __func__);
> -			ret = -ENOMEM;
> -			goto fail;
> -		}
> +		if (IS_ERR(phy->reg_base))
> +			return dev_err_probe(dev, PTR_ERR(phy->reg_base),
> +					     "Failed to map phy regulator base\n");
>   	}
>   
>   	if (phy->cfg->ops.parse_dt_properties) {
>   		ret = phy->cfg->ops.parse_dt_properties(phy);
>   		if (ret)
> -			goto fail;
> +			return ret;
>   	}
>   
>   	ret = devm_regulator_bulk_get_const(dev, phy->cfg->num_regulators,
>   					    phy->cfg->regulator_data,
>   					    &phy->supplies);
>   	if (ret)
> -		goto fail;
> +		return ret;
>   
>   	phy->ahb_clk = msm_clk_get(pdev, "iface");
> -	if (IS_ERR(phy->ahb_clk)) {
> -		DRM_DEV_ERROR(dev, "%s: Unable to get ahb clk\n", __func__);
> -		ret = PTR_ERR(phy->ahb_clk);
> -		goto fail;
> -	}
> +	if (IS_ERR(phy->ahb_clk))
> +		return dev_err_probe(dev, PTR_ERR(phy->ahb_clk),
> +				     "Unable to get ahb clk\n");
>   
>   	/* PLL init will call into clk_register which requires
>   	 * register access, so we need to enable power and ahb clock.
>   	 */
>   	ret = dsi_phy_enable_resource(phy);
>   	if (ret)
> -		goto fail;
> +		return ret;
>   
>   	if (phy->cfg->ops.pll_init) {
>   		ret = phy->cfg->ops.pll_init(phy);
> -		if (ret) {
> -			DRM_DEV_INFO(dev,
> -				"%s: pll init failed: %d, need separate pll clk driver\n",
> -				__func__, ret);
> -			goto fail;
> -		}
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "PLL init failed; need separate clk driver\n");
>   	}
>   
>   	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>   				     phy->provided_clocks);
> -	if (ret) {
> -		DRM_DEV_ERROR(dev, "%s: failed to register clk provider: %d\n", __func__, ret);
> -		goto fail;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register clk provider\n");
>   
>   	dsi_phy_disable_resource(phy);
>   
>   	platform_set_drvdata(pdev, phy);
>   
>   	return 0;
> -
> -fail:
> -	return ret;
>   }
>   
>   static struct platform_driver dsi_phy_platform_driver = {

Re: [PATCH v3 0/6] drm/msm/dsi regulator improvements

[Dmitry Baryshkov]

On 03/08/2022 01:37, Douglas Anderson wrote:
> The main goal of this series is to make a small dent in cleaning up
> the way we deal with regulator loads for DSI drivers.
> 
> As of v3 of this series, the regulator API improvements needed for the
> later patches in the series are merged into mainline. Thus this series
> only contains the DSI changes now.
> 
> I'd expect:
> * The first two patches are bugfixes found while converting the DSI
>    driver over. Those could land any time.
> * The third patch ("drm/msm/dsi: Don't set a load before disabling a
>    regulator") is a patch a sent the other day verbatim, included in
>    this series because it's highly related. It could land any
>    time.
> * The next two patches use the new APIs. Since those APIs are now in
>    mainline those could also land any time.
> * The last patch is just cleanup I noticed as I was touching the
>    function. It's not really related to regulators but it applies atop
>    these. In theory it could be rebased to land separately.
> 
> Changes in v3:
> - ("Improve dsi_phy_driver_probe() probe error handling") new for v3.
> - Do all the PHYs too.
> - Fix typo in commit message.
> - Get rid of error print after devm_regulator_bulk_get_const().
> - Just directly call the bulk commands; get rid of the wrapper.
> - Update commit message to point at the git hash of the regulator change.
> 
> Changes in v2:
> - ("Fix number of regulators for SDM660") new for v2.
> - ("Fix number of regulators for msm8996_dsi_cfg") new for v2.
> - ("Take advantage of devm_regulator_bulk_get_const") new for v2.
> - ("Use the new regulator bulk feature to specify the load") new for v2.
> 
> Douglas Anderson (6):
>    drm/msm/dsi: Fix number of regulators for msm8996_dsi_cfg
>    drm/msm/dsi: Fix number of regulators for SDM660

These two can be picked into the -fixes branch. The reset looks like 
5.21/6.1 material

>    drm/msm/dsi: Don't set a load before disabling a regulator
>    drm/msm/dsi: Use the new regulator bulk feature to specify the load
>    drm/msm/dsi: Take advantage of devm_regulator_bulk_get_const()
>    drm/msm/dsi: Improve dsi_phy_driver_probe() probe error handling
> 
>   drivers/gpu/drm/msm/dsi/dsi.h                 |  13 --
>   drivers/gpu/drm/msm/dsi/dsi_cfg.c             | 172 +++++++++---------
>   drivers/gpu/drm/msm/dsi/dsi_cfg.h             |   3 +-
>   drivers/gpu/drm/msm/dsi/dsi_host.c            |  96 ++--------
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c         | 160 ++++------------
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.h         |   5 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c    |  20 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c    |  32 ++--
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c    |  14 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c    |  28 +--
>   .../gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c   |  12 +-
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c     |  32 ++--
>   12 files changed, 197 insertions(+), 390 deletions(-)
> 


-- 
With best wishes
Dmitry