add power-supply support to dw_hdmi

drivers/regulator/core.c int regulator_bulk_get(struct device *dev, int num_consumers, · line 3168

Quoted 10 lines

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.

drivers/regulator/core.c +3 -2

-			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.

Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt +4 -0

+rockchip,rk3288-dw-hdmi handles two optional power supplies:
+- avdd1v0-supply: 1.0V power supply
+- avdd1v8-supply: 1.8V power supply
+
 Example:
 	hdmi: hdmi at 0120000 {
 		compatible = "fsl,imx6q-hdmi";

drivers/gpu/drm/bridge/dw_hdmi.c +1 -0

index cd6a706..9f8999d 100644
@@ -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>

drivers/gpu/drm/bridge/dw_hdmi.c +3 -0

+	struct regulator_bulk_data *supplies;
+	int nsupplies;
+
 	int vic;
 
 	u8 edid[HDMI_EDID_LEN];

drivers/gpu/drm/bridge/dw_hdmi.c +3 -0

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

drivers/gpu/drm/bridge/dw_hdmi.c +11 -0

 {
 	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);

drivers/gpu/drm/imx/dw_hdmi-imx.c +2 -1

index 121d30c..153e259 100644
@@ -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,

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +14 -1

index d236faa..c085e88 100644
@@ -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,

include/drm/bridge/dw_hdmi.h +0 -0

index 5a4f490..7b0fac5 100644

Am Dienstag, den 10.03.2015, 00:22 +0100 schrieb Heiko Stuebner:

Quoted 5 lines

drivers/gpu/drm/bridge/dw_hdmi.c +0 -0

@@ -879,6 +883,12 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
 
 	int i, ret;
 	bool cscon = false;
 

drivers/gpu/drm/bridge/dw_hdmi.c static int dw_hdmi_phy_init(struct dw_hdmi *hdmi) · line 891

Quoted 6 lines

drivers/gpu/drm/bridge/dw_hdmi.c +6 -0

+	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.

Quoted 27 lines

drivers/gpu/drm/bridge/dw_hdmi.c +0 -0

 	/*check csc whether needed activated in HDMI mode */
 	cscon = (is_color_space_conversion(hdmi) &&

drivers/gpu/drm/bridge/dw_hdmi.c +3 -0

 			!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);
+
 

drivers/gpu/drm/bridge/dw_hdmi.c +0 -0

 	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,

(quoted diff) +0 -0

 		 void *data, struct drm_encoder *encoder,
 		 struct resource *iores, int irq,

drivers/gpu/drm/bridge/dw_hdmi.c static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi) · line 1566

Quoted 3 lines

drivers/gpu/drm/bridge/dw_hdmi.c +2 -1

-		 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?

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.

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.