[PATCH 0/3] add power-supply support to dw_hdmi

[PATCH 0/3] add power-supply support to dw_hdmi

[Heiko Stuebner]

At least the Rockchip variant of the dw_hdmi should control its supplying
regulators. A cursory glance at the imx manual didn't any equivalent there,
so I'm not sure if there are similar controllable regulators present.

Patch1 is only a small fix to keep {devm_}regulator_bulk_get quiet in
the deferring case and could go directly to the regulator tree, if ok.

Patch3 is the addition of the supplies to the rk3288 boards and I would
take it into my dts branch if patch2 gets deemed acceptable.


Heiko Stuebner (3):
  regulator: don't emit errors in {devm_}regulator_bulk_get when defering
  drm/bridge: dw-hdmi: support optional supply regulators
  ARM: dts: rockchip: add hdmi analog power supplies to rk3288 boards

 .../devicetree/bindings/drm/bridge/dw_hdmi.txt     |  5 ++++
 arch/arm/boot/dts/rk3288-evb.dtsi                  |  2 ++
 arch/arm/boot/dts/rk3288-firefly.dtsi              |  2 ++
 drivers/gpu/drm/bridge/dw_hdmi.c                   | 27 +++++++++++++++++++++-
 drivers/gpu/drm/imx/dw_hdmi-imx.c                  |  3 ++-
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        | 15 +++++++++++-
 drivers/regulator/core.c                           |  5 ++--
 drivers/regulator/devres.c                         |  5 ++--
 include/drm/bridge/dw_hdmi.h                       |  3 ++-
 9 files changed, 59 insertions(+), 8 deletions(-)

-- 
2.1.4

[PATCH 1/3] regulator: don't emit errors in {devm_}regulator_bulk_get when defering

[Heiko Stuebner]

When {devm_}regulator_get returns -EPROBE_DEFER the driver in question will
try probing again at a later time. So don't spam the log with failure messages
as this is an expected result of probe ordering.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/regulator/core.c   | 5 +++--
 drivers/regulator/devres.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 52af1d7..07ca6cb 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3163,8 +3163,9 @@ int regulator_bulk_get(struct device *dev, int num_consumers,
 						      consumers[i].supply);
 		if (IS_ERR(consumers[i].consumer)) {
 			ret = PTR_ERR(consumers[i].consumer);
-			dev_err(dev, "Failed to get supply '%s': %d\n",
-				consumers[i].supply, ret);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get supply '%s': %d\n",
+					consumers[i].supply, ret);
 			consumers[i].consumer = NULL;
 			goto err;
 		}
diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 6ec1d40..78e460d 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -168,8 +168,9 @@ int devm_regulator_bulk_get(struct device *dev, int num_consumers,
 							   consumers[i].supply);
 		if (IS_ERR(consumers[i].consumer)) {
 			ret = PTR_ERR(consumers[i].consumer);
-			dev_err(dev, "Failed to get supply '%s': %d\n",
-				consumers[i].supply, ret);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get supply '%s': %d\n",
+					consumers[i].supply, ret);
 			consumers[i].consumer = NULL;
 			goto err;
 		}
-- 
2.1.4

Re: [PATCH 1/3] regulator: don't emit errors in {devm_}regulator_bulk_get when defering

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]

On Tue, Mar 10, 2015 at 12:22:06AM +0100, Heiko Stuebner wrote:
> When {devm_}regulator_get returns -EPROBE_DEFER the driver in question will
> try probing again at a later time. So don't spam the log with failure messages
> as this is an expected result of probe ordering.

> -			dev_err(dev, "Failed to get supply '%s': %d\n",
> -				consumers[i].supply, ret);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to get supply '%s': %d\n",
> +					consumers[i].supply, ret);

No, this is not good - you get a nice quiet boot even if the regulator
does not appear which means people have no idea why the driver isn't
loading.  That's not a good user experience, silent error handling is
the main problem I see people running into trying to get their systems
up and running these days.

Really deferred probe is just fundamentally noisy since it's
intentionally tolerating errors like this and of course a lot of the
noise comes from the deferral messages the core prints.

This is also not really connected to the series you're posting it in...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/3] regulator: don't emit errors in {devm_}regulator_bulk_get when defering

[Heiko Stuebner]

Am Dienstag, 10. März 2015, 12:07:50 schrieb Mark Brown:
> On Tue, Mar 10, 2015 at 12:22:06AM +0100, Heiko Stuebner wrote:
> > When {devm_}regulator_get returns -EPROBE_DEFER the driver in question
> > will
> > try probing again at a later time. So don't spam the log with failure
> > messages as this is an expected result of probe ordering.
> > 
> > -			dev_err(dev, "Failed to get supply '%s': %d\n",
> > -				consumers[i].supply, ret);
> > +			if (ret != -EPROBE_DEFER)
> > +				dev_err(dev, "Failed to get supply '%s': %d\n",
> > +					consumers[i].supply, ret);
> 
> No, this is not good - you get a nice quiet boot even if the regulator
> does not appear which means people have no idea why the driver isn't
> loading.  That's not a good user experience, silent error handling is
> the main problem I see people running into trying to get their systems
> up and running these days.
> 
> Really deferred probe is just fundamentally noisy since it's
> intentionally tolerating errors like this and of course a lot of the
> noise comes from the deferral messages the core prints.

ok, I'll drop this one then

[PATCH 2/3] drm/bridge: dw-hdmi: support optional supply regulators

[Heiko Stuebner]

At least the Rockchip variant of the dw_hdmi can have controllable power supplies
providing 1.0 and 1.8V. Therefore add the possibility for the generic bridge
driver to enable supplies provided by the hw-specific drivers.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 .../devicetree/bindings/drm/bridge/dw_hdmi.txt     |  5 ++++
 drivers/gpu/drm/bridge/dw_hdmi.c                   | 27 +++++++++++++++++++++-
 drivers/gpu/drm/imx/dw_hdmi-imx.c                  |  3 ++-
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        | 15 +++++++++++-
 include/drm/bridge/dw_hdmi.h                       |  3 ++-
 5 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
index a905c14..bb74640 100644
--- a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
+++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
@@ -22,6 +22,11 @@ Optional properties
 - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
 - clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"
 
+Supplies:
+rockchip,rk3288-dw-hdmi handles two optional power supplies:
+- avdd1v0-supply: 1.0V power supply
+- avdd1v8-supply: 1.8V power supply
+
 Example:
 	hdmi: hdmi@0120000 {
 		compatible = "fsl,imx6q-hdmi";
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index cd6a706..9f8999d 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -17,6 +17,7 @@
 #include <linux/clk.h>
 #include <linux/hdmi.h>
 #include <linux/of_device.h>
+#include <linux/regulator/consumer.h>
 
 #include <drm/drm_of.h>
 #include <drm/drmP.h>
@@ -114,6 +115,9 @@ struct dw_hdmi {
 	struct hdmi_data_info hdmi_data;
 	const struct dw_hdmi_plat_data *plat_data;
 
+	struct regulator_bulk_data *supplies;
+	int nsupplies;
+
 	int vic;
 
 	u8 edid[HDMI_EDID_LEN];
@@ -879,6 +883,12 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
 	int i, ret;
 	bool cscon = false;
 
+	if (hdmi->nsupplies > 0) {
+		ret = regulator_bulk_enable(hdmi->nsupplies, hdmi->supplies);
+		if (ret)
+			return ret;
+	}
+
 	/*check csc whether needed activated in HDMI mode */
 	cscon = (is_color_space_conversion(hdmi) &&
 			!hdmi->hdmi_data.video_mode.mdvi);
@@ -1105,6 +1115,9 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi)
 	dw_hdmi_phy_enable_tmds(hdmi, 0);
 	dw_hdmi_phy_enable_power(hdmi, 0);
 
+	if (hdmi->nsupplies > 0)
+		regulator_bulk_disable(hdmi->nsupplies, hdmi->supplies);
+
 	hdmi->phy_enabled = false;
 }
 
@@ -1549,7 +1562,8 @@ static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi)
 int dw_hdmi_bind(struct device *dev, struct device *master,
 		 void *data, struct drm_encoder *encoder,
 		 struct resource *iores, int irq,
-		 const struct dw_hdmi_plat_data *plat_data)
+		 const struct dw_hdmi_plat_data *plat_data,
+		 struct regulator_bulk_data *supplies, int nsupplies)
 {
 	struct drm_device *drm = data;
 	struct device_node *np = dev->of_node;
@@ -1602,6 +1616,17 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->regs))
 		return PTR_ERR(hdmi->regs);
 
+	if (nsupplies > 0) {
+		ret = devm_regulator_bulk_get(hdmi->dev, nsupplies, supplies);
+		if (ret == -EPROBE_DEFER)
+			return ret;
+		if (ret)
+			nsupplies = 0;
+	}
+
+	hdmi->supplies = supplies;
+	hdmi->nsupplies = nsupplies;
+
 	hdmi->isfr_clk = devm_clk_get(hdmi->dev, "isfr");
 	if (IS_ERR(hdmi->isfr_clk)) {
 		ret = PTR_ERR(hdmi->isfr_clk);
diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index 121d30c..153e259 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -214,7 +214,8 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS);
 
-	return dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data);
+	return dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data,
+			    NULL, 0);
 }
 
 static void dw_hdmi_imx_unbind(struct device *dev, struct device *master,
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index d236faa..c085e88 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -11,6 +11,7 @@
 #include <linux/platform_device.h>
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <drm/drm_of.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
@@ -24,6 +25,8 @@
 #define GRF_SOC_CON6                    0x025c
 #define HDMI_SEL_VOP_LIT                (1 << 4)
 
+#define HDMI_NUM_REGULATORS             2
+
 struct rockchip_hdmi {
 	struct device *dev;
 	struct regmap *regmap;
@@ -248,6 +251,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	struct platform_device *pdev = to_platform_device(dev);
 	const struct dw_hdmi_plat_data *plat_data;
 	const struct of_device_id *match;
+	struct regulator_bulk_data *supplies;
 	struct drm_device *drm = data;
 	struct drm_encoder *encoder;
 	struct rockchip_hdmi *hdmi;
@@ -275,6 +279,14 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	if (!iores)
 		return -ENXIO;
 
+	supplies = devm_kcalloc(&pdev->dev, HDMI_NUM_REGULATORS,
+				sizeof(*supplies), GFP_KERNEL);
+	if (!supplies)
+		return -ENOMEM;
+
+	supplies[0].supply = "avdd1v0";
+	supplies[1].supply = "avdd1v8";
+
 	platform_set_drvdata(pdev, hdmi);
 
 	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
@@ -297,7 +309,8 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS);
 
-	return dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data);
+	return dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data,
+			    supplies, HDMI_NUM_REGULATORS);
 }
 
 static void dw_hdmi_rockchip_unbind(struct device *dev, struct device *master,
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 5a4f490..7b0fac5 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -57,5 +57,6 @@ void dw_hdmi_unbind(struct device *dev, struct device *master, void *data);
 int dw_hdmi_bind(struct device *dev, struct device *master,
 		 void *data, struct drm_encoder *encoder,
 		 struct resource *iores, int irq,
-		 const struct dw_hdmi_plat_data *plat_data);
+		 const struct dw_hdmi_plat_data *plat_data,
+		 struct regulator_bulk_data *supplies, int nsupplies);
 #endif /* __IMX_HDMI_H__ */
-- 
2.1.4

Re: [PATCH 2/3] drm/bridge: dw-hdmi: support optional supply regulators

[Philipp Zabel]

Hi Heiko,

Am Dienstag, den 10.03.2015, 00:22 +0100 schrieb Heiko Stuebner:
> diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
> index a905c14..bb74640 100644
> --- a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
> +++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
> @@ -22,6 +22,11 @@ Optional properties
>  - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
>  - clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"
>  
> +Supplies:

"Optional supplies:" ? It seems to be common to always state optional or
required for the properties.

> +rockchip,rk3288-dw-hdmi handles two optional power supplies:
> +- avdd1v0-supply: 1.0V power supply
> +- avdd1v8-supply: 1.8V power supply
> +
>  Example:
>  	hdmi: hdmi@0120000 {
>  		compatible = "fsl,imx6q-hdmi";
> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
> index cd6a706..9f8999d 100644
> --- a/drivers/gpu/drm/bridge/dw_hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c
> @@ -17,6 +17,7 @@
>  #include <linux/clk.h>
>  #include <linux/hdmi.h>
>  #include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
>  
>  #include <drm/drm_of.h>
>  #include <drm/drmP.h>
> @@ -114,6 +115,9 @@ struct dw_hdmi {
>  	struct hdmi_data_info hdmi_data;
>  	const struct dw_hdmi_plat_data *plat_data;
>  

Superfluous empty line.

> +	struct regulator_bulk_data *supplies;
> +	int nsupplies;
> +
>  	int vic;
>  
>  	u8 edid[HDMI_EDID_LEN];
> @@ -879,6 +883,12 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
>  	int i, ret;
>  	bool cscon = false;
>  
> +	if (hdmi->nsupplies > 0) {
> +		ret = regulator_bulk_enable(hdmi->nsupplies, hdmi->supplies);
> +		if (ret)
> +			return ret;
> +	}
> +

Are these really supplies to the PHY or is this just a convenient place
to enable them? Looking at the i.MX6 docs, I suppose yes.

>  	/*check csc whether needed activated in HDMI mode */
>  	cscon = (is_color_space_conversion(hdmi) &&
>  			!hdmi->hdmi_data.video_mode.mdvi);
> @@ -1105,6 +1115,9 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi)
>  	dw_hdmi_phy_enable_tmds(hdmi, 0);
>  	dw_hdmi_phy_enable_power(hdmi, 0);
>  
> +	if (hdmi->nsupplies > 0)
> +		regulator_bulk_disable(hdmi->nsupplies, hdmi->supplies);
> +
>  	hdmi->phy_enabled = false;
>  }
>  
> @@ -1549,7 +1562,8 @@ static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi)
>  int dw_hdmi_bind(struct device *dev, struct device *master,
>  		 void *data, struct drm_encoder *encoder,
>  		 struct resource *iores, int irq,
> -		 const struct dw_hdmi_plat_data *plat_data)
> +		 const struct dw_hdmi_plat_data *plat_data,
> +		 struct regulator_bulk_data *supplies, int nsupplies)

I'm not sure I like this function sprouting so many parameters. Is there
a good reason not to add this to struct dw_hdmi_plat_data?

>  {
>  	struct drm_device *drm = data;
>  	struct device_node *np = dev->of_node;
> @@ -1602,6 +1616,17 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
>  	if (IS_ERR(hdmi->regs))
>  		return PTR_ERR(hdmi->regs);
>  
> +	if (nsupplies > 0) {
> +		ret = devm_regulator_bulk_get(hdmi->dev, nsupplies, supplies);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +		if (ret)
> +			nsupplies = 0;
> +	}
> +
> +	hdmi->supplies = supplies;
> +	hdmi->nsupplies = nsupplies;
> +
>  	hdmi->isfr_clk = devm_clk_get(hdmi->dev, "isfr");
>  	if (IS_ERR(hdmi->isfr_clk)) {
>  		ret = PTR_ERR(hdmi->isfr_clk);
> diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> index 121d30c..153e259 100644
> --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> @@ -214,7 +214,8 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
>  	drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
>  			 DRM_MODE_ENCODER_TMDS);
>  
> -	return dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data);
> +	return dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data,
> +			    NULL, 0);
>  }
>  
>  static void dw_hdmi_imx_unbind(struct device *dev, struct device *master,
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index d236faa..c085e88 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -11,6 +11,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  #include <drm/drm_of.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
> @@ -24,6 +25,8 @@
>  #define GRF_SOC_CON6                    0x025c
>  #define HDMI_SEL_VOP_LIT                (1 << 4)
>  
> +#define HDMI_NUM_REGULATORS             2
> +
>  struct rockchip_hdmi {
>  	struct device *dev;
>  	struct regmap *regmap;
> @@ -248,6 +251,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>  	struct platform_device *pdev = to_platform_device(dev);
>  	const struct dw_hdmi_plat_data *plat_data;
>  	const struct of_device_id *match;
> +	struct regulator_bulk_data *supplies;
>  	struct drm_device *drm = data;
>  	struct drm_encoder *encoder;
>  	struct rockchip_hdmi *hdmi;
> @@ -275,6 +279,14 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>  	if (!iores)
>  		return -ENXIO;
>  
> +	supplies = devm_kcalloc(&pdev->dev, HDMI_NUM_REGULATORS,
> +				sizeof(*supplies), GFP_KERNEL);
> +	if (!supplies)
> +		return -ENOMEM;
> +
> +	supplies[0].supply = "avdd1v0";
> +	supplies[1].supply = "avdd1v8";
> +
>  	platform_set_drvdata(pdev, hdmi);
>  
>  	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
> @@ -297,7 +309,8 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>  	drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs,
>  			 DRM_MODE_ENCODER_TMDS);
>  
> -	return dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data);
> +	return dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data,
> +			    supplies, HDMI_NUM_REGULATORS);
>  }
>  
>  static void dw_hdmi_rockchip_unbind(struct device *dev, struct device *master,
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 5a4f490..7b0fac5 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -57,5 +57,6 @@ void dw_hdmi_unbind(struct device *dev, struct device *master, void *data);
>  int dw_hdmi_bind(struct device *dev, struct device *master,
>  		 void *data, struct drm_encoder *encoder,
>  		 struct resource *iores, int irq,
> -		 const struct dw_hdmi_plat_data *plat_data);
> +		 const struct dw_hdmi_plat_data *plat_data,
> +		 struct regulator_bulk_data *supplies, int nsupplies);
>  #endif /* __IMX_HDMI_H__ */

regards
Philipp

Re: [PATCH 2/3] drm/bridge: dw-hdmi: support optional supply regulators

[Heiko Stuebner]

Hi Philipp,

Am Dienstag, 10. März 2015, 10:16:53 schrieb Philipp Zabel:

[cut the obvious stuff I need to fix]

> Am Dienstag, den 10.03.2015, 00:22 +0100 schrieb Heiko Stuebner:
> > @@ -879,6 +883,12 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
> > 
> >  	int i, ret;
> >  	bool cscon = false;
> > 
> > +	if (hdmi->nsupplies > 0) {
> > +		ret = regulator_bulk_enable(hdmi->nsupplies, hdmi->supplies);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> 
> Are these really supplies to the PHY or is this just a convenient place
> to enable them? Looking at the i.MX6 docs, I suppose yes.

According to the TRM, the supplies are "DC supply voltage for Analog part of 
HDMI", so I'd think phy supplies.

> 
> >  	/*check csc whether needed activated in HDMI mode */
> >  	cscon = (is_color_space_conversion(hdmi) &&
> >  	
> >  			!hdmi->hdmi_data.video_mode.mdvi);
> > 
> > @@ -1105,6 +1115,9 @@ static void dw_hdmi_phy_disable(struct dw_hdmi
> > *hdmi)
> > 
> >  	dw_hdmi_phy_enable_tmds(hdmi, 0);
> >  	dw_hdmi_phy_enable_power(hdmi, 0);
> > 
> > +	if (hdmi->nsupplies > 0)
> > +		regulator_bulk_disable(hdmi->nsupplies, hdmi->supplies);
> > +
> > 
> >  	hdmi->phy_enabled = false;
> >  
> >  }
> > 
> > @@ -1549,7 +1562,8 @@ static int dw_hdmi_register(struct drm_device *drm,
> > struct dw_hdmi *hdmi)> 
> >  int dw_hdmi_bind(struct device *dev, struct device *master,
> >  
> >  		 void *data, struct drm_encoder *encoder,
> >  		 struct resource *iores, int irq,
> > 
> > -		 const struct dw_hdmi_plat_data *plat_data)
> > +		 const struct dw_hdmi_plat_data *plat_data,
> > +		 struct regulator_bulk_data *supplies, int nsupplies)
> 
> I'm not sure I like this function sprouting so many parameters. Is there
> a good reason not to add this to struct dw_hdmi_plat_data?

Nope, moving this to platdata sound sensible and I'll give this a try :-)


Heiko

[PATCH 3/3] ARM: dts: rockchip: add hdmi analog power supplies to rk3288 boards

[Heiko Stuebner]

Add the recently added hdmi power supplies to evb and firefly boards.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/arm/boot/dts/rk3288-evb.dtsi     | 2 ++
 arch/arm/boot/dts/rk3288-firefly.dtsi | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi b/arch/arm/boot/dts/rk3288-evb.dtsi
index 5e895a5..edd45a0 100644
--- a/arch/arm/boot/dts/rk3288-evb.dtsi
+++ b/arch/arm/boot/dts/rk3288-evb.dtsi
@@ -118,6 +118,8 @@
 };
 
 &hdmi {
+	avdd1v0-supply = <&vdd10_lcd>;
+	avdd1v8-supply = <&vcc18_lcd>;
 	ddc-i2c-bus = <&i2c5>;
 	status = "okay";
 };
diff --git a/arch/arm/boot/dts/rk3288-firefly.dtsi b/arch/arm/boot/dts/rk3288-firefly.dtsi
index e6f873a..09fcded 100644
--- a/arch/arm/boot/dts/rk3288-firefly.dtsi
+++ b/arch/arm/boot/dts/rk3288-firefly.dtsi
@@ -180,6 +180,8 @@
 };
 
 &hdmi {
+	avdd1v0-supply = <&vdd10_lcd>;
+	avdd1v8-supply = <&vcc18_lcd>;
 	ddc-i2c-bus = <&i2c5>;
 	status = "okay";
 };
-- 
2.1.4

Re: [PATCH 0/3] add power-supply support to dw_hdmi

[Philipp Zabel]

Am Dienstag, den 10.03.2015, 00:21 +0100 schrieb Heiko Stuebner:
> At least the Rockchip variant of the dw_hdmi should control its supplying
> regulators. A cursory glance at the imx manual didn't any equivalent there,
> so I'm not sure if there are similar controllable regulators present.

On i.MX6 the HDMI_VP (1.1 V) and HDMI_VPH (2.5 V) are usually supplied
by the internal LDO_SoC via VDD_SOC_CAP (also used for ARM cache, SATA,
and PCIe) and the LDO_2P5 via VDDHIGH_CAP (also used for PLLs, MIPI,
SATA, PCIe, USB, and LVDS), respectively.
While I haven't seen anyone ever do this, it should be possible to
supply them from an external regulator.

regards
Philipp

Re: [PATCH 0/3] add power-supply support to dw_hdmi

[Russell King - ARM Linux]

On Tue, Mar 10, 2015 at 12:21:21AM +0100, Heiko Stuebner wrote:
> At least the Rockchip variant of the dw_hdmi should control its supplying
> regulators. A cursory glance at the imx manual didn't any equivalent there,
> so I'm not sure if there are similar controllable regulators present.
> 
> Patch1 is only a small fix to keep {devm_}regulator_bulk_get quiet in
> the deferring case and could go directly to the regulator tree, if ok.
> 
> Patch3 is the addition of the supplies to the rk3288 boards and I would
> take it into my dts branch if patch2 gets deemed acceptable.

I have a problem with this approach.

Rather than trying to shove everything into the generic part of the driver,
what we should be doing is ensuring that the generic part stays generic,
and the specific implementation details end up in the appropriate place.

As Rockchip needs these external regulators, they should be in that part
of the driver, and controlled appropriately from the encoder callbacks.
The encoder has sufficient callbacks to be able to turn on/off regulators
both before and after the bridge has been disabled/enabled.

When we see more implementations with regulators, then we can think about
moving it into the core dw_hdmi driver code.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.