diff mbox

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

Message ID 2297963.iMSNqyfSUx@phil (mailing list archive)
State New, archived
Headers show

Commit Message

Heiko Stuebner March 9, 2015, 11:22 p.m. UTC
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(-)

Comments

Philipp Zabel March 10, 2015, 9:16 a.m. UTC | #1
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
Heiko Stuebner March 10, 2015, 9:43 a.m. UTC | #2
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
diff mbox

Patch

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__ */