arm64: apq8016-sbc: Enable regulators for ADV7533

Add the regulator supply properties needed for ADV7533. The regulator names are named after the pin names. The exact use of each supply isn't clear from the data sheets.

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

There might be a similar set of regulators for ADV7511, but we don't have the docs at the moment to find out what supplies it needs. The regulator properties needed by ADV7511 can be updated as we get more data on it.

Cc Rob Herring <[email protected]>

Cc [email protected]

Signed-off-by Archit Taneja <[email protected]>

---
 Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 6 ++++++
 1 file changed, 6 insertions(+)

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +0 -0

index 6532a59..669b6f2 100644

Quoted 4 lines · Archit Taneja

Add the regulator supply properties needed for ADV7533. The regulator names are named after the pin names. The exact use of each supply isn't clear from the data sheets.

Quoted 8 lines

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

There might be a similar set of regulators for ADV7511, but we don't have the docs at the moment to find out what supplies it needs. The regulator properties needed by ADV7511 can be updated as we get more data on it.

The ADV7510 is available and there's a pinout comparison with the 7511:

http://www.analog.com/media/en/technical-documentation/evaluation-documentation/ADV7510_to_ADV7511_differences.pdf

These 2 seem to have the same supplies...

Quoted 9 lines

Cc Rob Herring <[email protected]>

Cc [email protected]

Signed-off-by Archit Taneja <[email protected]>

---
  Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 6 ++++++
  1 file changed, 6 insertions(+)

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt Optional properties: · line 60

Quoted 10 lines

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +2 -0

index 6532a59..669b6f2 100644
@@ -56,6 +56,12 @@ Optional properties:
  - adi,disable-timing-generator: Only for ADV7533. Disables the internal timing
    generator. The chip will rely on the sync signals in the DSI data lanes,
    rather than generate its own timings for HDMI output.
+- avdd-supply: Only for ADV7533. A 1.8V supply that powers up the AVDD, DVDD
+  PVDD and A2VDD pins on the chip.

This sounds wrong. Each input to the chip should be a property even if they are typically from the same supply.

The 7511 has DVDD, AVDD, and PVDD.

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt Optional properties: · line 62

Quoted 2 lines

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +2 -0

+- v1p2-supply: Only for ADV7533. A supply that powers up the V1P2 pin on the
+  chip. Can be either 1.2V or 1.8V.

This appears to be 7533 specific.

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt Optional properties: · line 64

Quoted 2 lines

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +2 -0

+- v3p3-supply: Only for ADV7533. A 3.3V supply that powers up the V3P3 pin on
+  the chip.

DVDD_3V on 7511

7510/7511 also have BGVDD (for Bandgap). Not clear what the voltage is.

Rob

ADV7533 requires supply to the AVDD, V1P2 and V3P3 pins for proper functionality.

Initialize and enable the regulators during probe itself. Controlling these dynamically is left for later.

Cc [email protected]

Cc Laurent Pinchart <[email protected]>

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/gpu/drm/bridge/adv7511/adv7511.h     | 16 ++++++++++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 34 +++++++++++++++------
 drivers/gpu/drm/bridge/adv7511/adv7533.c     | 45 +++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 161c923..3fcb202 100644

drivers/gpu/drm/bridge/adv7511/adv7511.h +16 -0

@@ -12,6 +12,7 @@
 #include <linux/hdmi.h>
 #include <linux/i2c.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_mipi_dsi.h>

@@ -327,6 +328,10 @@ struct adv7511 {
 
 	struct gpio_desc *gpio_pd;
 
+	struct regulator *avdd;
+	struct regulator *v1p2;
+	struct regulator *v3p3;
+
 	/* ADV7533 DSI RX related params */
 	struct device_node *host_node;
 	struct mipi_dsi_device *dsi;

@@ -343,6 +348,8 @@ void adv7533_mode_set(struct adv7511 *adv, struct
 drm_display_mode *mode); int adv7533_patch_registers(struct adv7511 *adv);
 void adv7533_uninit_cec(struct adv7511 *adv);
 int adv7533_init_cec(struct adv7511 *adv);
+int adv7533_init_regulators(struct adv7511 *adv);
+void adv7533_uninit_regulators(struct adv7511 *adv);
 int adv7533_attach_dsi(struct adv7511 *adv);
 void adv7533_detach_dsi(struct adv7511 *adv);
 int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv);

@@ -374,6 +381,15 @@ static inline int adv7533_init_cec(struct adv7511 *adv)
 return -ENODEV;
 }
 
+static inline int adv7533_init_regulators(struct adv7511 *adv)
+{
+	return -ENODEV;
+}
+
+static inline void adv7533_uninit_regulators(struct adv7511 *adv)
+{
+}
+
 static inline int adv7533_attach_dsi(struct adv7511 *adv)
 {
 	return -ENODEV;

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index ec8fb2e..221bc75 100644

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +1 -0

@@ -941,6 +941,7 @@ static int adv7511_probe(struct i2c_client *i2c, const
 struct i2c_device_id *id) if (!adv7511)
 		return -ENOMEM;
 
+	adv7511->i2c_main = i2c;
 	adv7511->powered = false;
 	adv7511->status = connector_status_disconnected;
 

@@ -958,13 +959,21 @@ static int adv7511_probe(struct i2c_client *i2c, const
 struct i2c_device_id *id) if (ret)

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +9 -5

 	 * inactive to wake up the encoder.
 	 */
 	adv7511->gpio_pd = devm_gpiod_get_optional(dev, "pd", GPIOD_OUT_HIGH);
-	if (IS_ERR(adv7511->gpio_pd))
-		return PTR_ERR(adv7511->gpio_pd);
+	if (IS_ERR(adv7511->gpio_pd)) {
+		ret = PTR_ERR(adv7511->gpio_pd);
+		goto uninit_regulators;
+	}
 
 	if (adv7511->gpio_pd) {
 		mdelay(5);

@@ -972,12 +981,14 @@ static int adv7511_probe(struct i2c_client *i2c, const
 struct i2c_device_id *id) }
 
 	adv7511->regmap = devm_regmap_init_i2c(i2c, &adv7511_regmap_config);
-	if (IS_ERR(adv7511->regmap))
-		return PTR_ERR(adv7511->regmap);
+	if (IS_ERR(adv7511->regmap)) {
+		ret = PTR_ERR(adv7511->regmap);
+		goto uninit_regulators;
+	}
 
 	ret = regmap_read(adv7511->regmap, ADV7511_REG_CHIP_REVISION, &val);
 	if (ret)
-		return ret;
+		goto uninit_regulators;
 	dev_dbg(dev, "Rev. %d\n", val);
 
 	if (adv7511->type == ADV7511)

@@ -987,7 +998,7 @@ static int adv7511_probe(struct i2c_client *i2c, const

struct i2c_device_id *id) else

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +4 -3

 	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,

@@ -995,10 +1006,11 @@ static int adv7511_probe(struct i2c_client *i2c,
 const struct i2c_device_id *id) regmap_write(adv7511->regmap,
 ADV7511_REG_CEC_I2C_ADDR, cec_i2c_addr); adv7511_packet_disable(adv7511,
 0xffff);
 
-	adv7511->i2c_main = i2c;
 	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
-	if (!adv7511->i2c_edid)
-		return -ENOMEM;
+	if (!adv7511->i2c_edid) {
+		ret = -ENOMEM;
+		goto uninit_regulators;
+	}
 
 	if (adv7511->type == ADV7533) {

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +4 -0

 		ret = adv7533_init_cec(adv7511);

@@ -1043,6 +1055,9 @@ err_unregister_cec:
 	adv7533_uninit_cec(adv7511);
 err_i2c_unregister_edid:
 	i2c_unregister_device(adv7511->i2c_edid);
+uninit_regulators:
+	if (adv7511->type == ADV7533)
+		adv7533_uninit_regulators(adv7511);
 
 	return ret;
 }

@@ -1054,6 +1069,7 @@ static int adv7511_remove(struct i2c_client *i2c)
 	if (adv7511->type == ADV7533) {
 		adv7533_detach_dsi(adv7511);
 		adv7533_uninit_cec(adv7511);
+		adv7533_uninit_regulators(adv7511);
 	}
 
 	drm_bridge_remove(&adv7511->bridge);

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7533.c index 5eebd15..03a59fd 100644

drivers/gpu/drm/bridge/adv7511/adv7533.c +0 -0

@@ -178,6 +178,51 @@ err:
 	return ret;

drivers/gpu/drm/bridge/adv7511/adv7533.c +9 -0

+	adv->v1p2 = devm_regulator_get(dev, "v1p2");
+	if (IS_ERR(adv->v1p2))
+		return PTR_ERR(adv->v1p2);
+
+	adv->v3p3 = devm_regulator_get(dev, "v3p3");
+	if (IS_ERR(adv->v3p3))
+		return PTR_ERR(adv->v3p3);
+
+	ret = regulator_enable(adv->avdd);

drivers/gpu/drm/bridge/adv7511/adv7533.c +16 -0

+	return 0;
+err_v3p3:
+	regulator_disable(adv->v1p2);
+err_v1p2:
+	regulator_disable(adv->avdd);
+
+	return ret;
+}
+
+void adv7533_uninit_regulators(struct adv7511 *adv)
+{
+	regulator_disable(adv->v3p3);
+	regulator_disable(adv->v1p2);
+	regulator_disable(adv->avdd);
+}
+
 int adv7533_attach_dsi(struct adv7511 *adv)
 {
 	struct device *dev = &adv->i2c_main->dev;

Hi Archit,

Thank you for the patch.

Quoted 5 lines · On Wednesday 31 Aug 2016 16:22:09 Archit Taneja wrote:

ADV7533 requires supply to the AVDD, V1P2 and V3P3 pins for proper functionality.

Initialize and enable the regulators during probe itself. Controlling these dynamically is left for later.

You should document the power supplies in the DT bindings.

Quoted 73 lines

Cc [email protected]

Cc Laurent Pinchart <[email protected]>

Signed-off-by Archit Taneja <[email protected]>

---
  drivers/gpu/drm/bridge/adv7511/adv7511.h     | 16 ++++++++++
  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 34 +++++++++++++++------
  drivers/gpu/drm/bridge/adv7511/adv7533.c     | 45 +++++++++++++++++++++++++
  3 files changed, 86 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 161c923..3fcb202 100644

drivers/gpu/drm/bridge/adv7511/adv7511.h +16 -0

@@ -12,6 +12,7 @@
  #include <linux/hdmi.h>
  #include <linux/i2c.h>
  #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 
  #include <drm/drm_crtc_helper.h>
  #include <drm/drm_mipi_dsi.h>

@@ -327,6 +328,10 @@ struct adv7511 {
 
  	struct gpio_desc *gpio_pd;
 
+	struct regulator *avdd;
+	struct regulator *v1p2;
+	struct regulator *v3p3;
+
  	/* ADV7533 DSI RX related params */
  	struct device_node *host_node;
  	struct mipi_dsi_device *dsi;

@@ -343,6 +348,8 @@ void adv7533_mode_set(struct adv7511 *adv, struct
 drm_display_mode *mode); int adv7533_patch_registers(struct adv7511 *adv);
  void adv7533_uninit_cec(struct adv7511 *adv);
  int adv7533_init_cec(struct adv7511 *adv);
+int adv7533_init_regulators(struct adv7511 *adv);
+void adv7533_uninit_regulators(struct adv7511 *adv);
  int adv7533_attach_dsi(struct adv7511 *adv);
  void adv7533_detach_dsi(struct adv7511 *adv);
  int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv);

@@ -374,6 +381,15 @@ static inline int adv7533_init_cec(struct adv7511 *adv)
 return -ENODEV;
  }
 
+static inline int adv7533_init_regulators(struct adv7511 *adv)
+{
+	return -ENODEV;
+}
+
+static inline void adv7533_uninit_regulators(struct adv7511 *adv)
+{
+}
+
  static inline int adv7533_attach_dsi(struct adv7511 *adv)
  {
  	return -ENODEV;

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index ec8fb2e..221bc75 100644

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +1 -0

@@ -941,6 +941,7 @@ static int adv7511_probe(struct i2c_client *i2c, const
 struct i2c_device_id *id) if (!adv7511)
  		return -ENOMEM;
 
+	adv7511->i2c_main = i2c;
  	adv7511->powered = false;
  	adv7511->status = connector_status_disconnected;
 

@@ -958,13 +959,21 @@ static int adv7511_probe(struct i2c_client *i2c, const
 struct i2c_device_id *id) if (ret)

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c static int adv7511_probe(struct i2c_client *i2c, const · line 969

Quoted 10 lines

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +6 -0

  		return ret;
 
+	if (adv7511->type == ADV7533) {
+		ret = adv7533_init_regulators(adv7511);
+		if (ret)
+			return ret;
+	}
+
  	/*
  	 * The power down GPIO is optional. If present, toggle it from active

to

Quoted 34 lines

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +9 -5

  	 * inactive to wake up the encoder.
  	 */
  	adv7511->gpio_pd = devm_gpiod_get_optional(dev, "pd", GPIOD_OUT_HIGH);
-	if (IS_ERR(adv7511->gpio_pd))
-		return PTR_ERR(adv7511->gpio_pd);
+	if (IS_ERR(adv7511->gpio_pd)) {
+		ret = PTR_ERR(adv7511->gpio_pd);
+		goto uninit_regulators;
+	}
 
  	if (adv7511->gpio_pd) {
  		mdelay(5);

@@ -972,12 +981,14 @@ static int adv7511_probe(struct i2c_client *i2c, const
 struct i2c_device_id *id) }
 
  	adv7511->regmap = devm_regmap_init_i2c(i2c, &adv7511_regmap_config);
-	if (IS_ERR(adv7511->regmap))
-		return PTR_ERR(adv7511->regmap);
+	if (IS_ERR(adv7511->regmap)) {
+		ret = PTR_ERR(adv7511->regmap);
+		goto uninit_regulators;
+	}
 
  	ret = regmap_read(adv7511->regmap, ADV7511_REG_CHIP_REVISION, &val);
  	if (ret)
-		return ret;
+		goto uninit_regulators;
  	dev_dbg(dev, "Rev. %d\n", val);
 
  	if (adv7511->type == ADV7511)

@@ -987,7 +998,7 @@ static int adv7511_probe(struct i2c_client *i2c, const

struct i2c_device_id *id) else
ret = adv7533_patch_registers(adv7511);
if (ret)

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)

Quoted 4 lines

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +1 -1

-		return ret;
+		goto uninit_regulators;
 
  	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,

edid_i2c_addr);

Quoted 43 lines

regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +4 -3

@@ -995,10 +1006,11 @@ static int adv7511_probe(struct i2c_client *i2c,
 const struct i2c_device_id *id) regmap_write(adv7511->regmap,
 ADV7511_REG_CEC_I2C_ADDR, cec_i2c_addr); adv7511_packet_disable(adv7511,
 0xffff);
 
-	adv7511->i2c_main = i2c;
  	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
-	if (!adv7511->i2c_edid)
-		return -ENOMEM;
+	if (!adv7511->i2c_edid) {
+		ret = -ENOMEM;
+		goto uninit_regulators;
+	}
 
  	if (adv7511->type == ADV7533) {

  		ret = adv7533_init_cec(adv7511);

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +4 -0

@@ -1043,6 +1055,9 @@ err_unregister_cec:
  	adv7533_uninit_cec(adv7511);
  err_i2c_unregister_edid:
  	i2c_unregister_device(adv7511->i2c_edid);
+uninit_regulators:
+	if (adv7511->type == ADV7533)
+		adv7533_uninit_regulators(adv7511);
 
  	return ret;
  }

@@ -1054,6 +1069,7 @@ static int adv7511_remove(struct i2c_client *i2c)
  	if (adv7511->type == ADV7533) {
  		adv7533_detach_dsi(adv7511);
  		adv7533_uninit_cec(adv7511);
+		adv7533_uninit_regulators(adv7511);
  	}
 
  	drm_bridge_remove(&adv7511->bridge);

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7533.c index 5eebd15..03a59fd 100644

drivers/gpu/drm/bridge/adv7511/adv7533.c +0 -0

@@ -178,6 +178,51 @@ err:
  	return ret;
  }
 

drivers/gpu/drm/bridge/adv7511/adv7533.c err: · line 188

Quoted 8 lines

drivers/gpu/drm/bridge/adv7511/adv7533.c +8 -0

+int adv7533_init_regulators(struct adv7511 *adv)
+{
+	struct device *dev = &adv->i2c_main->dev;
+	int ret;
+
+	adv->avdd = devm_regulator_get(dev, "avdd");
+	if (IS_ERR(adv->avdd))
+		return PTR_ERR(adv->avdd);

Doesn't this cause backward compatibility issues with existing device trees that don't declare regulators ?

Quoted 14 lines

drivers/gpu/drm/bridge/adv7511/adv7533.c +13 -0

+	adv->v1p2 = devm_regulator_get(dev, "v1p2");
+	if (IS_ERR(adv->v1p2))
+		return PTR_ERR(adv->v1p2);
+
+	adv->v3p3 = devm_regulator_get(dev, "v3p3");
+	if (IS_ERR(adv->v3p3))
+		return PTR_ERR(adv->v3p3);
+
+	ret = regulator_enable(adv->avdd);
+	if (ret)
+		return ret;
+
+	ret = regulator_enable(adv->v1p2);

drivers/gpu/drm/bridge/adv7511/adv7533.c err: · line 208

Quoted 6 lines

drivers/gpu/drm/bridge/adv7511/adv7533.c +6 -0

+	if (ret)
+		goto err_v1p2;
+
+	ret = regulator_enable(adv->v3p3);
+	if (ret)
+		goto err_v3p3;

How about using the devm_regulator_bulk_*() API ?

Quoted 20 lines

drivers/gpu/drm/bridge/adv7511/adv7533.c +16 -0

+	return 0;
+err_v3p3:
+	regulator_disable(adv->v1p2);
+err_v1p2:
+	regulator_disable(adv->avdd);
+
+	return ret;
+}
+
+void adv7533_uninit_regulators(struct adv7511 *adv)
+{
+	regulator_disable(adv->v3p3);
+	regulator_disable(adv->v1p2);
+	regulator_disable(adv->avdd);
+}
+
  int adv7533_attach_dsi(struct adv7511 *adv)
  {
  	struct device *dev = &adv->i2c_main->dev;

Quoted 4 lines · Laurent Pinchart

Quoted 3 lines · On Wednesday 31 Aug 2016 16:22:09 Archit Taneja wrote:

ADV7533 requires supply to the AVDD, V1P2 and V3P3 pins for proper functionality.

Quoted 4 lines

Quoted 2 lines

Initialize and enable the regulators during probe itself. Controlling these dynamically is left for later.

You should document the power supplies in the DT bindings.

The DT bindings doc update was a part of the same series. I accidentally Cc'd you only for this patch.

Quoted 9 lines

Quoted 9 lines

Cc [email protected]

Cc Laurent Pinchart <[email protected]>

Signed-off-by Archit Taneja <[email protected]>

---

  drivers/gpu/drm/bridge/adv7511/adv7511.h     | 16 ++++++++++
  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 34 +++++++++++++++------
  drivers/gpu/drm/bridge/adv7511/adv7533.c     | 45 +++++++++++++++++++++
  3 files changed, 86 insertions(+), 9 deletions(-)

Hi Archit,

Quoted 8 lines · On Wednesday 31 Aug 2016 22:24:30 Archit Taneja wrote:

Quoted 7 lines · Laurent Pinchart

Quoted 5 lines · On Wednesday 31 Aug 2016 16:22:09 Archit Taneja wrote:

ADV7533 requires supply to the AVDD, V1P2 and V3P3 pins for proper functionality.

Initialize and enable the regulators during probe itself. Controlling these dynamically is left for later.

Quoted 4 lines

Quoted 1 line

You should document the power supplies in the DT bindings.

The DT bindings doc update was a part of the same series. I accidentally Cc'd you only for this patch.

No worries. What's the patch's subject ?

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Cc [email protected]

Cc Laurent Pinchart <[email protected]>

Signed-off-by Archit Taneja <[email protected]>

---

   drivers/gpu/drm/bridge/adv7511/adv7511.h     | 16 ++++++++++
   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 34 +++++++++++++++------
   drivers/gpu/drm/bridge/adv7511/adv7533.c     | 45 +++++++++++++++++++++
   3 files changed, 86 insertions(+), 9 deletions(-)

On the APQ8016 SBC, the LDO2 PM8916 regulator feeds 1.2V to the following:

- VDDA_1P2_MIPI_DSI and VDDA_MIPI_CSI pins on APQ8016.
- VCCCAD pins on the LPDDR3 chip.
- VDDPX_1 pins on APQ8016.

The LDO6 regulator feeds 1.8V to:
- VDAA_MIPI_DSI0_PLL pin on APQ8016.
- QFPROM_BLOW_VDD pin on PM8916.
- The AVDD, A2VDD and DVDD pins on ADV7533 bridge.

The LDO17 regulator feeds 3.3V to:
- The V3P3 pin on ADV7533 bridge.

Currently, the regulator min/max voltages for all the LDOs are set to the range of what the PMIC supports. Set the ranges for L2, L6 and L17 to what we need, i.e. 1.2V, 1.8V and 3.3V respectively.

Quoted 14 lines · Archit Taneja

On the APQ8016 SBC, the LDO2 PM8916 regulator feeds 1.2V to the following:

- VDDA_1P2_MIPI_DSI and VDDA_MIPI_CSI pins on APQ8016.
- VCCCAD pins on the LPDDR3 chip.
- VDDPX_1 pins on APQ8016.

The LDO6 regulator feeds 1.8V to:
- VDAA_MIPI_DSI0_PLL pin on APQ8016.
- QFPROM_BLOW_VDD pin on PM8916.
- The AVDD, A2VDD and DVDD pins on ADV7533 bridge.

The LDO17 regulator feeds 3.3V to:
- The V3P3 pin on ADV7533 bridge.

Quoted 3 lines

Currently, the regulator min/max voltages for all the LDOs are set to the range of what the PMIC supports. Set the ranges for L2, L6 and L17 to what we need, i.e. 1.2V, 1.8V and 3.3V respectively.

Why doesn't the consuming driver request a particular voltage on these LDOs? It doesn't seem correct to modify board constraints to satisfy a consumer (even if that consumer is on the board).

Quoted 14 lines · Archit Taneja

On the APQ8016 SBC, the LDO2 PM8916 regulator feeds 1.2V to the following:

- VDDA_1P2_MIPI_DSI and VDDA_MIPI_CSI pins on APQ8016.
- VCCCAD pins on the LPDDR3 chip.
- VDDPX_1 pins on APQ8016.

The LDO6 regulator feeds 1.8V to:
- VDAA_MIPI_DSI0_PLL pin on APQ8016.
- QFPROM_BLOW_VDD pin on PM8916.
- The AVDD, A2VDD and DVDD pins on ADV7533 bridge.

The LDO17 regulator feeds 3.3V to:
- The V3P3 pin on ADV7533 bridge.

Quoted 3 lines

Currently, the regulator min/max voltages for all the LDOs are set to the range of what the PMIC supports. Set the ranges for L2, L6 and L17 to what we need, i.e. 1.2V, 1.8V and 3.3V respectively.

Why doesn't the consuming driver request a particular voltage on these LDOs? It doesn't seem correct to modify board constraints to satisfy a consumer (even if that consumer is on the board).

Add the regulator supply properties needed by ADV7511 and ADV7533. The regulator names are named after the pin names, except for DVDD_3V on ADV7511, which is left v3p3 to have the same name as it's on ADV7533.

ADV7511 has a BGVDD pin for which the voltage to be configured isn't clear. This needs to be updated in the document later.

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

Cc Rob Herring <[email protected]>

Cc [email protected]

Signed-off-by Archit Taneja <[email protected]>

---
v2:
- Specify regulator supplies for ADV7511 too.
- Have separate supply entries for AVDD, DVDD, PVDD, A2VDD pins.
- Add additional BGVDD supply for ADV7511.

 .../devicetree/bindings/display/bridge/adi,adv7511.txt     | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Add the regulator supply properties needed by ADV7511 and ADV7533. The regulator names are named after the pin names, except for DVDD_3V on ADV7511, which is left v3p3 to have the same name as it's on ADV7533.

ADV7511 has a BGVDD pin for which the voltage to be configured isn't clear. This needs to be updated in the document later.

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

Cc Rob Herring <[email protected]>

Cc [email protected]

Signed-off-by Archit Taneja <[email protected]>

---
v2:
- Specify regulator supplies for ADV7511 too.
- Have separate supply entries for AVDD, DVDD, PVDD, A2VDD pins.
- Add additional BGVDD supply for ADV7511.

 .../devicetree/bindings/display/bridge/adi,adv7511.txt     | 14 +++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index 6532a59..66e6bae 100644

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +0 -0

@@ -56,6 +56,20 @@ Optional properties:
 - adi,disable-timing-generator: Only for ADV7533. Disables the internal

timing generator. The chip will rely on the sync signals in the DSI data lanes, rather than generate its own timings for HDMI output.

+- avdd-supply: A 1.8V supply that powers up the AVDD pin on the chip.
+- dvdd-supply: A 1.8V supply that powers up the DVDD pin on the chip.
+- pvdd-supply: A 1.8V supply that powers up the PVDD pin on the chip.
+- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +4 -0

+ADV7533 specific supplies:
+- a2vdd-supply: 1.8V supply that powers up the A2VDD pin on the chip.
+- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can be
+  either 1.2V or 1.8V.
 
 Required nodes:

Hi Archit,

Thank you for the patch.

Quoted 37 lines · On Friday 23 Sep 2016 14:50:27 Archit Taneja wrote:

Add the regulator supply properties needed by ADV7511 and ADV7533. The regulator names are named after the pin names, except for DVDD_3V on ADV7511, which is left v3p3 to have the same name as it's on ADV7533.

ADV7511 has a BGVDD pin for which the voltage to be configured isn't clear. This needs to be updated in the document later.

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

Cc Rob Herring <[email protected]>

Cc [email protected]

Signed-off-by Archit Taneja <[email protected]>

---
v2:
- Specify regulator supplies for ADV7511 too.
- Have separate supply entries for AVDD, DVDD, PVDD, A2VDD pins.
- Add additional BGVDD supply for ADV7511.

  .../devicetree/bindings/display/bridge/adi,adv7511.txt     | 14 +++++++++++
  1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index 6532a59..66e6bae 100644

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +0 -0

@@ -56,6 +56,20 @@ Optional properties:
  - adi,disable-timing-generator: Only for ADV7533. Disables the internal

timing generator. The chip will rely on the sync signals in the DSI data lanes, rather than generate its own timings for HDMI output.

+- avdd-supply: A 1.8V supply that powers up the AVDD pin on the chip.
+- dvdd-supply: A 1.8V supply that powers up the DVDD pin on the chip.
+- pvdd-supply: A 1.8V supply that powers up the PVDD pin on the chip.
+- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt Optional properties: · line 67

Quoted 5 lines

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +5 -0

+  ADV7511 and V3P3 on ADV7533.
+
+ADV7511 specific supplies:
+- bgvdd-supply: Powers the BGVDD pin. The voltage isn't clear from
+  available datasheets.

It's a 1.8V supply. The ADV7511 hardware user's guide recommends tying it to the PVDD supply, so I'm not sure we need a separate supply in DT.

Similarly, the user's guide recommends supplying the AVDD, DVDD and PVDD pins with a single 1.8V LDO and three independent filters. We could thus possibly combine them into a single 1.8V supply.

Quoted 6 lines

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +4 -0

+ADV7533 specific supplies:
+- a2vdd-supply: 1.8V supply that powers up the A2VDD pin on the chip.
+- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can be
+  either 1.2V or 1.8V.
 
  Required nodes:

Quoted 34 lines · Laurent Pinchart

Quoted 33 lines · On Friday 23 Sep 2016 14:50:27 Archit Taneja wrote:

Add the regulator supply properties needed by ADV7511 and ADV7533. The regulator names are named after the pin names, except for DVDD_3V on ADV7511, which is left v3p3 to have the same name as it's on ADV7533.

ADV7511 has a BGVDD pin for which the voltage to be configured isn't clear. This needs to be updated in the document later.

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

Cc Rob Herring <[email protected]>

Cc [email protected]

Signed-off-by Archit Taneja <[email protected]>

---
v2:
- Specify regulator supplies for ADV7511 too.
- Have separate supply entries for AVDD, DVDD, PVDD, A2VDD pins.
- Add additional BGVDD supply for ADV7511.

  .../devicetree/bindings/display/bridge/adi,adv7511.txt     | 14 +++++++
  1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index 6532a59..66e6bae 100644

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +0 -0

@@ -56,6 +56,20 @@ Optional properties:
  - adi,disable-timing-generator: Only for ADV7533. Disables the internal
 

Quoted 14 lines

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt Optional properties: · line 67

Quoted 11 lines

timing generator. The chip will rely on the sync signals in the DSI data lanes, rather than generate its own timings for HDMI output.

+- avdd-supply: A 1.8V supply that powers up the AVDD pin on the chip.
+- dvdd-supply: A 1.8V supply that powers up the DVDD pin on the chip.
+- pvdd-supply: A 1.8V supply that powers up the PVDD pin on the chip.
+- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +5 -0

+  ADV7511 and V3P3 on ADV7533.
+
+ADV7511 specific supplies:
+- bgvdd-supply: Powers the BGVDD pin. The voltage isn't clear from
+  available datasheets.

It's a 1.8V supply. The ADV7511 hardware user's guide recommends tying it to the PVDD supply, so I'm not sure we need a separate supply in DT.

Thanks for providing the info. I'll update it in the doc.

Quoted 3 lines

Similarly, the user's guide recommends supplying the AVDD, DVDD and PVDD pins with a single 1.8V LDO and three independent filters. We could thus possibly combine them into a single 1.8V supply.

I'd originally specified only one AVDD supply for all the 1.8 V supplies, but Rob had recommended using separate supplies for each pin.

I guess it doesn't do much harm specifying separate supplies, a board can just provide avdd-supply, and the rest would be assumed dummy.

It could also help describe boards which may not have followed the user guide's recommendation, but I agree that's probably a rare scenario.

Is it okay if we stick with multiple supplies as it is in this version?

Quoted 7 lines

Quoted 7 lines

+ADV7533 specific supplies:

+- a2vdd-supply: 1.8V supply that powers up the A2VDD pin on the chip.
+- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can

be
+ either 1.2V or 1.8V.

Required nodes:

Hi Archit,

Quoted 56 lines · On Monday 26 Sep 2016 12:10:11 Archit Taneja wrote:

Quoted 34 lines · Laurent Pinchart

Quoted 33 lines · On Friday 23 Sep 2016 14:50:27 Archit Taneja wrote:

Add the regulator supply properties needed by ADV7511 and ADV7533. The regulator names are named after the pin names, except for DVDD_3V on ADV7511, which is left v3p3 to have the same name as it's on ADV7533.

ADV7511 has a BGVDD pin for which the voltage to be configured isn't clear. This needs to be updated in the document later.

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

Cc Rob Herring <[email protected]>

Cc [email protected]

Signed-off-by Archit Taneja <[email protected]>

---
v2:
- Specify regulator supplies for ADV7511 too.
- Have separate supply entries for AVDD, DVDD, PVDD, A2VDD pins.
- Add additional BGVDD supply for ADV7511.

  .../devicetree/bindings/display/bridge/adi,adv7511.txt     | 14 +++++++
  1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index 6532a59..66e6bae 100644

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +0 -0

@@ -56,6 +56,20 @@ Optional properties:
  - adi,disable-timing-generator: Only for ADV7533. Disables the internal
 

Quoted 14 lines

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt Optional properties: · line 67

Quoted 11 lines

timing generator. The chip will rely on the sync signals in the DSI data lanes, rather than generate its own timings for HDMI output.

+- avdd-supply: A 1.8V supply that powers up the AVDD pin on the chip.
+- dvdd-supply: A 1.8V supply that powers up the DVDD pin on the chip.
+- pvdd-supply: A 1.8V supply that powers up the PVDD pin on the chip.
+- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +5 -0

+  ADV7511 and V3P3 on ADV7533.
+
+ADV7511 specific supplies:
+- bgvdd-supply: Powers the BGVDD pin. The voltage isn't clear from
+  available datasheets.

It's a 1.8V supply. The ADV7511 hardware user's guide recommends tying it to the PVDD supply, so I'm not sure we need a separate supply in DT.

Thanks for providing the info. I'll update it in the doc.

Quoted 3 lines

Similarly, the user's guide recommends supplying the AVDD, DVDD and PVDD pins with a single 1.8V LDO and three independent filters. We could thus possibly combine them into a single 1.8V supply.

Quoted 5 lines

I'd originally specified only one AVDD supply for all the 1.8 V supplies, but Rob had recommended using separate supplies for each pin.

I guess it doesn't do much harm specifying separate supplies, a board can just provide avdd-supply, and the rest would be assumed dummy.

DT should use the same regulator phandle for all power supplies in that case, not specify a single one.

Quoted 4 lines

It could also help describe boards which may not have followed the user guide's recommendation, but I agree that's probably a rare scenario.

Quoted 1 line

Is it okay if we stick with multiple supplies as it is in this version?

It seems overkill to me but I can live with that. I'd remove the bgvdd supply though.

Rob, what's your opinion on this ? Is there a specific reason why you prefer splitting the 1.8V supply in one supply per pin, when the hardware user's guide recommend powering them from the same LDO ?

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

+ADV7533 specific supplies:

+- a2vdd-supply: 1.8V supply that powers up the A2VDD pin on the chip.
+- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can

be
+ either 1.2V or 1.8V.

Required nodes:

Hi Rob,

Quoted 93 lines · Laurent Pinchart

Hi Archit,

Quoted 67 lines · On Monday 26 Sep 2016 12:10:11 Archit Taneja wrote:

Quoted 48 lines · Laurent Pinchart

Quoted 46 lines · On Friday 23 Sep 2016 14:50:27 Archit Taneja wrote:

Add the regulator supply properties needed by ADV7511 and ADV7533. The regulator names are named after the pin names, except for DVDD_3V on ADV7511, which is left v3p3 to have the same name as it's on ADV7533.

ADV7511 has a BGVDD pin for which the voltage to be configured isn't clear. This needs to be updated in the document later.

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

Cc Rob Herring <[email protected]>

Cc [email protected]

Signed-off-by Archit Taneja <[email protected]>

---
v2:
- Specify regulator supplies for ADV7511 too.
- Have separate supply entries for AVDD, DVDD, PVDD, A2VDD pins.
- Add additional BGVDD supply for ADV7511.

  .../devicetree/bindings/display/bridge/adi,adv7511.txt     | 14
+++++++
  1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index
6532a59..66e6bae 100644

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +0 -0

@@ -56,6 +56,20 @@ Optional properties:
  - adi,disable-timing-generator: Only for ADV7533. Disables the

internal

timing generator. The chip will rely on the sync signals in the DSI data
lanes, rather than generate its own timings for HDMI output.

+- avdd-supply: A 1.8V supply that powers up the AVDD pin on the chip.
+- dvdd-supply: A 1.8V supply that powers up the DVDD pin on the chip.
+- pvdd-supply: A 1.8V supply that powers up the PVDD pin on the chip.
+- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +2 -0

+  ADV7511 and V3P3 on ADV7533.
+

Quoted 8 lines

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt Optional properties: · line 67

Quoted 3 lines

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +3 -0

+ADV7511 specific supplies:
+- bgvdd-supply: Powers the BGVDD pin. The voltage isn't clear from
+  available datasheets.

It's a 1.8V supply. The ADV7511 hardware user's guide recommends tying it
to the PVDD supply, so I'm not sure we need a separate supply in DT.

Thanks for providing the info. I'll update it in the doc.

Quoted 3 lines

Similarly, the user's guide recommends supplying the AVDD, DVDD and PVDD pins with a single 1.8V LDO and three independent filters. We could thus possibly combine them into a single 1.8V supply.

Quoted 5 lines

I'd originally specified only one AVDD supply for all the 1.8 V supplies, but Rob had recommended using separate supplies for each pin.

I guess it doesn't do much harm specifying separate supplies, a board can just provide avdd-supply, and the rest would be assumed dummy.

DT should use the same regulator phandle for all power supplies in that case,
not specify a single one.

Quoted 5 lines

It could also help describe boards which may not have followed the user guide's recommendation, but I agree that's probably a rare scenario.

Is it okay if we stick with multiple supplies as it is in this version?

It seems overkill to me but I can live with that. I'd remove the bgvdd supply
though.

Quoted 4 lines

Rob, what's your opinion on this ? Is there a specific reason why you prefer
splitting the 1.8V supply in one supply per pin, when the hardware user's guide recommend powering them from the same LDO ?

Could you suggest us which way to go?

Quoted 103 lines · Archit Taneja

Hi Rob,

Quoted 97 lines · Laurent Pinchart

Hi Archit,

Quoted 67 lines · On Monday 26 Sep 2016 12:10:11 Archit Taneja wrote:

Quoted 48 lines · Laurent Pinchart

Quoted 46 lines · On Friday 23 Sep 2016 14:50:27 Archit Taneja wrote:

Add the regulator supply properties needed by ADV7511 and ADV7533. The regulator names are named after the pin names, except for DVDD_3V on ADV7511, which is left v3p3 to have the same name as it's on ADV7533.

ADV7511 has a BGVDD pin for which the voltage to be configured isn't clear. This needs to be updated in the document later.

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

Cc Rob Herring <[email protected]>

Cc [email protected]

Signed-off-by Archit Taneja <[email protected]>

---
v2:
- Specify regulator supplies for ADV7511 too.
- Have separate supply entries for AVDD, DVDD, PVDD, A2VDD pins.
- Add additional BGVDD supply for ADV7511.

  .../devicetree/bindings/display/bridge/adi,adv7511.txt     | 14
+++++++
  1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index
6532a59..66e6bae 100644

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +0 -0

@@ -56,6 +56,20 @@ Optional properties:
  - adi,disable-timing-generator: Only for ADV7533. Disables the

internal

timing generator. The chip will rely on the sync signals in the DSI data
lanes, rather than generate its own timings for HDMI output.

+- avdd-supply: A 1.8V supply that powers up the AVDD pin on the chip.
+- dvdd-supply: A 1.8V supply that powers up the DVDD pin on the chip.
+- pvdd-supply: A 1.8V supply that powers up the PVDD pin on the chip.
+- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +2 -0

+  ADV7511 and V3P3 on ADV7533.
+

Quoted 8 lines

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt

Quoted 3 lines

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +3 -0

+ADV7511 specific supplies:
+- bgvdd-supply: Powers the BGVDD pin. The voltage isn't clear from
+  available datasheets.

It's a 1.8V supply. The ADV7511 hardware user's guide recommends tying it
to the PVDD supply, so I'm not sure we need a separate supply in DT.

Thanks for providing the info. I'll update it in the doc.

Quoted 3 lines

Similarly, the user's guide recommends supplying the AVDD, DVDD and PVDD pins with a single 1.8V LDO and three independent filters. We could thus possibly combine them into a single 1.8V supply.

Quoted 5 lines

I'd originally specified only one AVDD supply for all the 1.8 V supplies, but Rob had recommended using separate supplies for each pin.

I guess it doesn't do much harm specifying separate supplies, a board can just provide avdd-supply, and the rest would be assumed dummy.

DT should use the same regulator phandle for all power supplies in that case,
not specify a single one.

Quoted 5 lines

It could also help describe boards which may not have followed the user guide's recommendation, but I agree that's probably a rare scenario.

Is it okay if we stick with multiple supplies as it is in this version?

It seems overkill to me but I can live with that. I'd remove the bgvdd supply
though.

Rob, what's your opinion on this ? Is there a specific reason why you prefer
splitting the 1.8V supply in one supply per pin, when the hardware user's guide recommend powering them from the same LDO ?

Quoted 1 line

Could you suggest us which way to go?

If they are really intended to be the same supply, then combining them is fine with me.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Maintain a table of regulator names expect by ADV7511 and ADV7533. Use regulator_bulk_* api to configure these.

Initialize and enable the regulators during probe itself. Controlling these dynamically is left for later.

Cc [email protected]

Cc Laurent Pinchart <[email protected]>

Signed-off-by Archit Taneja <[email protected]>

---
v2:
- Use regulator_bulk_* API to configure regulators as suggested by Laurent.
- Set up regulators for ADV7511 too.

 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  4 ++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 85 ++++++++++++++++++++++---
 2 files changed, 80 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 161c923..83ebdaa 100644

drivers/gpu/drm/bridge/adv7511/adv7511.h +1 -0

@@ -12,6 +12,7 @@
 #include <linux/hdmi.h>
 #include <linux/i2c.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_mipi_dsi.h>

drivers/gpu/drm/bridge/adv7511/adv7511.h +1 -0

+
 	/* ADV7533 DSI RX related params */
 	struct device_node *host_node;
 	struct mipi_dsi_device *dsi;

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8ed3906..f7e79ed 100644

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +12 -0

@@ -839,6 +839,57 @@ static struct drm_bridge_funcs adv7511_bridge_funcs = {
 * Probe & remove
  */
 
+static const char * const adv7511_supply_names[] = {
+	"avdd",
+	"dvdd",
+	"pvdd",
+	"v3p3",
+	"bgvdd",
+};
+
+static const char * const adv7533_supply_names[] = {
+	"avdd",
+	"dvdd",
+	"pvdd",

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +40 -2

+
+	if (adv->type == ADV7511) {
+		supply_names = adv7511_supply_names;
+		adv->num_supplies = ARRAY_SIZE(adv7511_supply_names);
+	} else {
+		supply_names = adv7533_supply_names;
+		adv->num_supplies = ARRAY_SIZE(adv7533_supply_names);
+	}
+
+	adv->supplies = devm_kcalloc(dev, adv->num_supplies,
+				     sizeof(*adv->supplies), GFP_KERNEL);
+	if (!adv->supplies)
+		return -ENOMEM;
+
+	for (i = 0; i < adv->num_supplies; i++)
+		adv->supplies[i].supply = supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, adv->num_supplies, adv->supplies);
+	if (ret)
+		return ret;
+
+	return regulator_bulk_enable(adv->num_supplies, adv->supplies);
+}
+
+static void adv7511_uninit_regulators(struct adv7511 *adv)
+{
+	regulator_bulk_disable(adv->num_supplies, adv->supplies);
+}
+
 static int adv7511_parse_dt(struct device_node *np,
 			    struct adv7511_link_config *config)
 {

@@ -939,6 +990,7 @@ static int adv7511_probe(struct i2c_client *i2c, const
 struct i2c_device_id *id) if (!adv7511)
 		return -ENOMEM;
 
+	adv7511->i2c_main = i2c;
 	adv7511->powered = false;
 	adv7511->status = connector_status_disconnected;
 

@@ -956,13 +1008,21 @@ static int adv7511_probe(struct i2c_client *i2c,
 const struct i2c_device_id *id)
 	if (ret)
 		return ret;
 
+	ret = adv7511_init_regulators(adv7511);
+	if (ret) {
+		dev_err(dev, "failed to init regulators\n");
+		return ret;
+	}
+
 	/*
 	 * The power down GPIO is optional. If present, toggle it from active
 	 to
 	 * inactive to wake up the encoder.
 	 */
 	adv7511->gpio_pd = devm_gpiod_get_optional(dev, "pd", GPIOD_OUT_HIGH);
-	if (IS_ERR(adv7511->gpio_pd))
-		return PTR_ERR(adv7511->gpio_pd);
+	if (IS_ERR(adv7511->gpio_pd)) {
+		ret = PTR_ERR(adv7511->gpio_pd);
+		goto uninit_regulators;
+	}
 

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +5 -3

 	if (adv7511->gpio_pd) {
 		mdelay(5);

@@ -970,12 +1030,14 @@ static int adv7511_probe(struct i2c_client *i2c,
 const struct i2c_device_id *id) }
 
 	adv7511->regmap = devm_regmap_init_i2c(i2c, &adv7511_regmap_config);
-	if (IS_ERR(adv7511->regmap))
-		return PTR_ERR(adv7511->regmap);
+	if (IS_ERR(adv7511->regmap)) {
+		ret = PTR_ERR(adv7511->regmap);
+		goto uninit_regulators;
+	}
 
 	ret = regmap_read(adv7511->regmap, ADV7511_REG_CHIP_REVISION, &val);
 	if (ret)
-		return ret;
+		goto uninit_regulators;
 	dev_dbg(dev, "Rev. %d\n", val);
 
 	if (adv7511->type == ADV7511)

@@ -985,7 +1047,7 @@ static int adv7511_probe(struct i2c_client *i2c, const

struct i2c_device_id *id) else

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +4 -3

 	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,

@@ -995,10 +1057,11 @@ static int adv7511_probe(struct i2c_client *i2c,
 const struct i2c_device_id *id)
 
 	adv7511_packet_disable(adv7511, 0xffff);
 
-	adv7511->i2c_main = i2c;
 	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
-	if (!adv7511->i2c_edid)
-		return -ENOMEM;
+	if (!adv7511->i2c_edid) {
+		ret = -ENOMEM;
+		goto uninit_regulators;
+	}
 
 	if (adv7511->type == ADV7533) {

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +4 -0

 		ret = adv7533_init_cec(adv7511);

@@ -1043,6 +1106,8 @@ err_unregister_cec:
 	adv7533_uninit_cec(adv7511);
 err_i2c_unregister_edid:
 	i2c_unregister_device(adv7511->i2c_edid);
+uninit_regulators:
+	adv7511_uninit_regulators(adv7511);
 
 	return ret;
 }

@@ -1056,6 +1121,8 @@ static int adv7511_remove(struct i2c_client *i2c)
 		adv7533_uninit_cec(adv7511);
 	}
 
+	adv7511_uninit_regulators(adv7511);
+
 	drm_bridge_remove(&adv7511->bridge);
 
 	i2c_unregister_device(adv7511->i2c_edid);

Hi Archit,

Thank you for the patch.

Quoted 31 lines · On Friday 23 Sep 2016 14:50:28 Archit Taneja wrote:

Maintain a table of regulator names expect by ADV7511 and ADV7533. Use regulator_bulk_* api to configure these.

Initialize and enable the regulators during probe itself. Controlling these dynamically is left for later.

Cc [email protected]

Cc Laurent Pinchart <[email protected]>

Signed-off-by Archit Taneja <[email protected]>

---
v2:
- Use regulator_bulk_* API to configure regulators as suggested by Laurent.
- Set up regulators for ADV7511 too.

  drivers/gpu/drm/bridge/adv7511/adv7511.h     |  4 ++
  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 85 ++++++++++++++++++++++---
  2 files changed, 80 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 161c923..83ebdaa 100644

drivers/gpu/drm/bridge/adv7511/adv7511.h +1 -0

@@ -12,6 +12,7 @@
  #include <linux/hdmi.h>
  #include <linux/i2c.h>
  #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 
  #include <drm/drm_crtc_helper.h>
  #include <drm/drm_mipi_dsi.h>

drivers/gpu/drm/bridge/adv7511/adv7511.h struct adv7511 { · line 332

Quoted 5 lines

drivers/gpu/drm/bridge/adv7511/adv7511.h +2 -0

@@ -327,6 +328,9 @@ struct adv7511 {
 
  	struct gpio_desc *gpio_pd;
 
+	struct regulator_bulk_data *supplies;
+	int num_supplies;

num_supplies is always positive, you can make it an unsigned int.

Quoted 25 lines

drivers/gpu/drm/bridge/adv7511/adv7511.h +1 -0

+
  	/* ADV7533 DSI RX related params */
  	struct device_node *host_node;
  	struct mipi_dsi_device *dsi;

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8ed3906..f7e79ed 100644

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +12 -0

@@ -839,6 +839,57 @@ static struct drm_bridge_funcs adv7511_bridge_funcs = {
 * Probe & remove
   */
 
+static const char * const adv7511_supply_names[] = {
+	"avdd",
+	"dvdd",
+	"pvdd",
+	"v3p3",
+	"bgvdd",
+};
+
+static const char * const adv7533_supply_names[] = {
+	"avdd",
+	"dvdd",
+	"pvdd",

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c static struct drm_bridge_funcs adv7511_bridge_funcs = { · line 863

Quoted 10 lines

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +10 -0

+	"a2vdd",
+	"v3p3",
+	"v1p2",
+};
+
+static int adv7511_init_regulators(struct adv7511 *adv)
+{
+	struct device *dev = &adv->i2c_main->dev;
+	const char * const *supply_names;
+	int i, ret;

i only takes positive values, you can make it an unsigned int.

Apart from that,

Reviewed-by Laurent Pinchart <[email protected]>

Add the regulator supply properties needed by ADV7511 and ADV7533.

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

Cc [email protected]

Acked-by Rob Herring <[email protected]>

Signed-off-by Archit Taneja <[email protected]>

---
v3:
- Revert back to having a common avdd-supply property for the 1.8V supplies

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 9 ++++++

1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index 6532a59..13d53bc 100644

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +0 -0

@@ -56,6 +56,15 @@ Optional properties:
 - adi,disable-timing-generator: Only for ADV7533. Disables the internal

timing generator. The chip will rely on the sync signals in the DSI data lanes, rather than generate its own timings for HDMI output.

+- avdd-supply: A common 1.8V supply that powers up the AVDD, DVDD and PVDD
+  pins. On ADV7511, it also feeds to the BGVDD pin. On ADV7533, it also

powers

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +7 -0

+  up the A2VDD pin.
+- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on
+  ADV7511 and V3P3 on ADV7533.
+
+ADV7533 specific supplies:
+- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can be
+  either 1.2V or 1.8V.
 
 Required nodes:

Hi Archit,

Thank you for the patch.

Quoted 2 lines · On Tuesday 29 Nov 2016 11:37:41 Archit Taneja wrote:

Add the regulator supply properties needed by ADV7511 and ADV7533.

Quoted 3 lines

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

That's why we have support for dummy supplies in the kernel, isn't it ? Isn't it better to make the supplies mandatory in the bindings (and obviously handling them as optional in the driver for backward-compatibility) ?

Apart from that,

Acked-by Laurent Pinchart <[email protected]>

Quoted 33 lines

Cc [email protected]

Acked-by Rob Herring <[email protected]>

Signed-off-by Archit Taneja <[email protected]>

---
v3:
- Revert back to having a common avdd-supply property for the 1.8V supplies

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 9 ++++++

1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index 6532a59..13d53bc 100644

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +0 -0

@@ -56,6 +56,15 @@ Optional properties:
 - adi,disable-timing-generator: Only for ADV7533. Disables the internal

timing generator. The chip will rely on the sync signals in the DSI data lanes, rather than generate its own timings for HDMI output.

+- avdd-supply: A common 1.8V supply that powers up the AVDD, DVDD and PVDD
+  pins. On ADV7511, it also feeds to the BGVDD pin. On ADV7533, it also

powers

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +7 -0

+  up the A2VDD pin.
+- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on
+  ADV7511 and V3P3 on ADV7533.
+
+ADV7533 specific supplies:
+- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can be
+  either 1.2V or 1.8V.
 
 Required nodes:

Quoted 11 lines · Laurent Pinchart

Quoted 5 lines · On Tuesday 29 Nov 2016 11:37:41 Archit Taneja wrote:

Add the regulator supply properties needed by ADV7511 and ADV7533.

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

That's why we have support for dummy supplies in the kernel, isn't it ? Isn't it better to make the supplies mandatory in the bindings (and obviously handling them as optional in the driver for backward-compatibility) ?

I'm a bit unclear on this.

I thought we couldn't add mandatory properties once the device is already present in DT for one or more platforms.

Say, if we do make it mandatory for future additions, we would need to have DT property for the supplies for the new platforms. If the regulators on these boards are fixed supplies, they would be need to be modeled using "regulator-fixed", possibly without any input supply. Is that what you're suggesting?

Quoted 41 lines

Apart from that,

Acked-by Laurent Pinchart <[email protected]>

Quoted 37 lines

Cc [email protected]

Acked-by Rob Herring <[email protected]>

Signed-off-by Archit Taneja <[email protected]>

---
v3:
- Revert back to having a common avdd-supply property for the 1.8V supplies

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 9 ++++

1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index 6532a59..13d53bc 100644

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +0 -0

@@ -56,6 +56,15 @@ Optional properties:
 - adi,disable-timing-generator: Only for ADV7533. Disables the internal
 

timing generator. The chip will rely on the sync signals in the DSI data lanes, rather than generate its own timings for HDMI output. +- avdd-supply: A common 1.8V supply that powers up the AVDD, DVDD and PVDD
+ pins. On ADV7511, it also feeds to the BGVDD pin. On ADV7533, it also powers

Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +6 -0

+  up the A2VDD pin.
+- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on
+  ADV7511 and V3P3 on ADV7533.
+
+ADV7533 specific supplies:
+- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can

be
+ either 1.2V or 1.8V.

Required nodes:

Quoted 2 lines · On Tuesday 29 Nov 2016 13:41:33 Archit Taneja wrote:

I thought we couldn't add mandatory properties once the device is already present in DT for one or more platforms.

You can, as long as you treat them as optional in the driver to retain backward compatibility. The DT bindings should document the properties expected from a new platform (older versions of the bindings will always be available in the git history).

Quoted 5 lines

Say, if we do make it mandatory for future additions, we would need to have DT property for the supplies for the new platforms. If the regulators on these boards are fixed supplies, they would be need to be modeled using "regulator-fixed", possibly without any input supply. Is that what you're suggesting?

That's the idea, yes. Clock maintainers have a similar opinion regarding the clock bindings, where a clock that is not optional at the hardware level should be specified in DT even if it's always present.

Mark, any opinion ?

Quoted 8 lines · Laurent Pinchart

Quoted 2 lines · On Tuesday 29 Nov 2016 13:41:33 Archit Taneja wrote:

I thought we couldn't add mandatory properties once the device is already present in DT for one or more platforms.

You can, as long as you treat them as optional in the driver to retain backward compatibility. The DT bindings should document the properties expected from a new platform (older versions of the bindings will always be available in the git history).

The device probably never worked without power... note that the kernel will substitute in dummy regulators for anything that isn't explicitly mapped so it won't actually break anything.

Quoted 10 lines

Quoted 5 lines

Say, if we do make it mandatory for future additions, we would need to have DT property for the supplies for the new platforms. If the regulators on these boards are fixed supplies, they would be need to be modeled using "regulator-fixed", possibly without any input supply. Is that what you're suggesting?

That's the idea, yes. Clock maintainers have a similar opinion regarding the clock bindings, where a clock that is not optional at the hardware level should be specified in DT even if it's always present.

Quoted 1 line

Mark, any opinion ?

It's best practice to always describe the power. The kernel will cope if people don't but it's not unknown for drivers to discover a reason for wanting information about their power and hard to retrofit that if it's not been in there from the get go.

Please note that if you're going to CC me into a graphics thread there's a good chance I will miss it, I get copied on quite a lot of graphics related mail that's not really relevant so I often skip it. Changing the subject line would help with that.

Quoted 4 lines · On Tuesday 29 Nov 2016 11:01:25 Mark Brown wrote:

Please note that if you're going to CC me into a graphics thread there's a good chance I will miss it, I get copied on quite a lot of graphics related mail that's not really relevant so I often skip it. Changing the subject line would help with that.

I try to add a (CC'ing xxx) at the beginning of the e-mail to draw attention when a question is targetted at a particular person. If that's not enough I can change the subject, but might forget to do so from time to time. Or you could whitelist me, unless I'm already blacklisted ;-)

Hi Archit,

(CC'ing Mark Brown)

Quoted 15 lines · On Tuesday 29 Nov 2016 13:41:33 Archit Taneja wrote:

Quoted 11 lines · Laurent Pinchart

Quoted 5 lines · On Tuesday 29 Nov 2016 11:37:41 Archit Taneja wrote:

Add the regulator supply properties needed by ADV7511 and ADV7533.

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

That's why we have support for dummy supplies in the kernel, isn't it ? Isn't it better to make the supplies mandatory in the bindings (and obviously handling them as optional in the driver for backward-compatibility) ?

I'm a bit unclear on this.

Quoted 2 lines

I thought we couldn't add mandatory properties once the device is already present in DT for one or more platforms.

You can, as long as you treat them as optional in the driver to retain backward compatibility. The DT bindings should document the properties expected from a new platform (older versions of the bindings will always be available in the git history).

Quoted 6 lines

Say, if we do make it mandatory for future additions, we would need to have DT property for the supplies for the new platforms. If the regulators on these boards are fixed supplies, they would be need to be modeled using "regulator-fixed", possibly without any input supply. Is that what you're suggesting?

That's the idea, yes. Clock maintainers have a similar opinion regarding the clock bindings, where a clock that is not optional at the hardware level should be specified in DT even if it's always present.

Quoted 16 lines · Laurent Pinchart

Quoted 15 lines · On Tuesday 29 Nov 2016 13:41:33 Archit Taneja wrote:

Quoted 11 lines · Laurent Pinchart

Quoted 5 lines · On Tuesday 29 Nov 2016 11:37:41 Archit Taneja wrote:

Add the regulator supply properties needed by ADV7511 and ADV7533.

The regulators are specified as optional properties since there can be boards which have a fixed supply directly routed to the pins, and these may not be modelled as regulator supplies.

That's why we have support for dummy supplies in the kernel, isn't it ? Isn't it better to make the supplies mandatory in the bindings (and obviously handling them as optional in the driver for backward-compatibility) ?

I'm a bit unclear on this.

Quoted 7 lines

Quoted 2 lines

I thought we couldn't add mandatory properties once the device is already present in DT for one or more platforms.

You can, as long as you treat them as optional in the driver to retain backward compatibility. The DT bindings should document the properties expected from a new platform (older versions of the bindings will always be available in the git history).

If you document them as required and don't do anything special in the implementation (i.e. just call devm_regulator_get() as usual) it will just work, in the absence of the property you will get a dummy regulator from the framework.

And then add the fixed-voltage regulators to the new DT to make that properly describe the hardware.

Quoted 6 lines

Quoted 5 lines

Say, if we do make it mandatory for future additions, we would need to have DT property for the supplies for the new platforms. If the regulators on these boards are fixed supplies, they would be need to be modeled using "regulator-fixed", possibly without any input supply. Is that what you're suggesting?

Quoted 3 lines

That's the idea, yes. Clock maintainers have a similar opinion regarding the clock bindings, where a clock that is not optional at the hardware level should be specified in DT even if it's always present.

Further more, a DT binding for a particular block should describe that block; so if we have three different 1.8V pins then the DT binding should reflect this - even if our current platform have them wired to the same regulator.

(And the supply names would preferably be based on the pin names in the component data sheet)

Quoted 4 lines · On Monday 05 Dec 2016 13:11:51 Bjorn Andersson wrote:

Further more, a DT binding for a particular block should describe that block; so if we have three different 1.8V pins then the DT binding should reflect this - even if our current platform have them wired to the same regulator.

This has been discussed previously, and Rob agreed that if the datasheet recommends to power all supplies from the same regulator we can take that as a good hint that a single supply should be enough. In the very unlikely event that a board would require control of more regulators we can always extend the DT bindings later without breaking backward compatibility.

Quoted 6 lines · Laurent Pinchart

Quoted 4 lines · On Monday 05 Dec 2016 13:11:51 Bjorn Andersson wrote:

Further more, a DT binding for a particular block should describe that block; so if we have three different 1.8V pins then the DT binding should reflect this - even if our current platform have them wired to the same regulator.

Quoted 6 lines

This has been discussed previously, and Rob agreed that if the datasheet recommends to power all supplies from the same regulator we can take that as a good hint that a single supply should be enough. In the very unlikely event that a board would require control of more regulators we can always extend the DT bindings later without breaking backward compatibility.

No, don't do this - introducing special snowflake bindings just makes things more complex at the system level and tells everyone else that they too can have special snowflake bindings. Someone should be able to connect up the regulators based purely on a schematic. Just describe the hardware, it's just one extra line in the DT per regulator.

Quoted 11 lines · On Tuesday 06 Dec 2016 10:05:17 Mark Brown wrote:

Quoted 3 lines · Laurent Pinchart

This has been discussed previously, and Rob agreed that if the datasheet recommends to power all supplies from the same regulator we can take that as a good hint that a single supply should be enough. In the very

No, don't do this - introducing special snowflake bindings just makes things more complex at the system level and tells everyone else that they too can have special snowflake bindings. Someone should be able to connect up the regulators based purely on a schematic. Just describe the hardware, it's just one extra line in the DT per regulator.

There are power supply pin that have different names but documented as having to be connected to the same supply. I really see no point in having multiple regulators for them.

Quoted 13 lines · Laurent Pinchart

Quoted 11 lines · On Tuesday 06 Dec 2016 10:05:17 Mark Brown wrote:

Quoted 4 lines · Laurent Pinchart

This has been discussed previously, and Rob agreed that if the datasheet recommends to power all supplies from the same regulator we can take that as a good hint that a single supply should be enough. In the very

No, don't do this - introducing special snowflake bindings just makes things more complex at the system level and tells everyone else that they too can have special snowflake bindings. Someone should be able to connect up the regulators based purely on a schematic. Just describe the hardware, it's just one extra line in the DT per regulator.

Quoted 3 lines

There are power supply pin that have different names but documented as having to be connected to the same supply. I really see no point in having multiple regulators for them.

The tiny amount of extra typing involved doesn't seem like much of a cost for keeping things consistent with every other regulator user out there.

Quoted 3 lines · On Tuesday 06 Dec 2016 13:20:20 Mark Brown wrote:

The tiny amount of extra typing involved doesn't seem like much of a cost for keeping things consistent with every other regulator user out there.

I'm not concerned by that at all, but by the additional runtime complexity :-/

Maintain a table of regulator names expect by ADV7511 and ADV7533. Use regulator_bulk_* api to configure these.

Initialize and enable the regulators during probe itself. Controlling these dynamically is left for later.

Signed-off-by Archit Taneja <[email protected]>

---
v3:
- Drop the additional 1.8V supply names

 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  4 ++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 79 +++++++++++++++++++++----
 2 files changed, 74 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 992d76c..18ec627 100644

drivers/gpu/drm/bridge/adv7511/adv7511.h +1 -0

@@ -12,6 +12,7 @@
 #include <linux/hdmi.h>
 #include <linux/i2c.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_mipi_dsi.h>

drivers/gpu/drm/bridge/adv7511/adv7511.h +1 -0

+
 	/* ADV7533 DSI RX related params */
 	struct device_node *host_node;
 	struct mipi_dsi_device *dsi;

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 2177440..f123fbb 100644

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +6 -0

@@ -843,6 +843,51 @@ static int adv7511_bridge_attach(struct drm_bridge
 *bridge) * Probe & remove
  */
 
+static const char * const adv7511_supply_names[] = {
+	"avdd",
+	"v3p3",
+};
+
+static const char * const adv7533_supply_names[] = {

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +30 -0

+
+	if (adv->type == ADV7511) {
+		supply_names = adv7511_supply_names;
+		adv->num_supplies = ARRAY_SIZE(adv7511_supply_names);
+	} else {
+		supply_names = adv7533_supply_names;
+		adv->num_supplies = ARRAY_SIZE(adv7533_supply_names);
+	}
+
+	adv->supplies = devm_kcalloc(dev, adv->num_supplies,
+				     sizeof(*adv->supplies), GFP_KERNEL);
+	if (!adv->supplies)
+		return -ENOMEM;
+
+	for (i = 0; i < adv->num_supplies; i++)
+		adv->supplies[i].supply = supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, adv->num_supplies, adv->supplies);
+	if (ret)
+		return ret;
+
+	return regulator_bulk_enable(adv->num_supplies, adv->supplies);
+}
+
+static void adv7511_uninit_regulators(struct adv7511 *adv)
+{
+	regulator_bulk_disable(adv->num_supplies, adv->supplies);
+}
+
 static int adv7511_parse_dt(struct device_node *np,
 			    struct adv7511_link_config *config)
 {

@@ -943,6 +988,7 @@ static int adv7511_probe(struct i2c_client *i2c, const
 struct i2c_device_id *id) if (!adv7511)
 		return -ENOMEM;
 
+	adv7511->i2c_main = i2c;
 	adv7511->powered = false;
 	adv7511->status = connector_status_disconnected;
 

@@ -960,13 +1006,21 @@ static int adv7511_probe(struct i2c_client *i2c,
 const struct i2c_device_id *id) if (ret)

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +9 -5

 	 * inactive to wake up the encoder.
 	 */
 	adv7511->gpio_pd = devm_gpiod_get_optional(dev, "pd", GPIOD_OUT_HIGH);
-	if (IS_ERR(adv7511->gpio_pd))
-		return PTR_ERR(adv7511->gpio_pd);
+	if (IS_ERR(adv7511->gpio_pd)) {
+		ret = PTR_ERR(adv7511->gpio_pd);
+		goto uninit_regulators;
+	}
 
 	if (adv7511->gpio_pd) {
 		mdelay(5);

@@ -974,12 +1028,14 @@ static int adv7511_probe(struct i2c_client *i2c,
 const struct i2c_device_id *id) }
 
 	adv7511->regmap = devm_regmap_init_i2c(i2c, &adv7511_regmap_config);
-	if (IS_ERR(adv7511->regmap))
-		return PTR_ERR(adv7511->regmap);
+	if (IS_ERR(adv7511->regmap)) {
+		ret = PTR_ERR(adv7511->regmap);
+		goto uninit_regulators;
+	}
 
 	ret = regmap_read(adv7511->regmap, ADV7511_REG_CHIP_REVISION, &val);
 	if (ret)
-		return ret;
+		goto uninit_regulators;
 	dev_dbg(dev, "Rev. %d\n", val);
 
 	if (adv7511->type == ADV7511)

@@ -989,7 +1045,7 @@ static int adv7511_probe(struct i2c_client *i2c, const

struct i2c_device_id *id) else

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +4 -3

 	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,

@@ -999,10 +1055,11 @@ static int adv7511_probe(struct i2c_client *i2c,
 const struct i2c_device_id *id)
 
 	adv7511_packet_disable(adv7511, 0xffff);
 
-	adv7511->i2c_main = i2c;
 	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
-	if (!adv7511->i2c_edid)
-		return -ENOMEM;
+	if (!adv7511->i2c_edid) {
+		ret = -ENOMEM;
+		goto uninit_regulators;
+	}
 
 	if (adv7511->type == ADV7533) {

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +4 -0

 		ret = adv7533_init_cec(adv7511);

@@ -1049,6 +1106,8 @@ static int adv7511_probe(struct i2c_client *i2c, const
 struct i2c_device_id *id) adv7533_uninit_cec(adv7511);
 err_i2c_unregister_edid:
 	i2c_unregister_device(adv7511->i2c_edid);
+uninit_regulators:
+	adv7511_uninit_regulators(adv7511);
 
 	return ret;
 }

@@ -1062,6 +1121,8 @@ static int adv7511_remove(struct i2c_client *i2c)
 		adv7533_uninit_cec(adv7511);
 	}
 
+	adv7511_uninit_regulators(adv7511);
+
 	drm_bridge_remove(&adv7511->bridge);
 
 	adv7511_audio_exit(adv7511);

Add the regulator supply properties needed by ADV7511 and ADV7533.

Cc [email protected]

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Archit Taneja <[email protected]>

---
 Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

Quoted 8 lines · Archit Taneja

Add the regulator supply properties needed by ADV7511 and ADV7533.

Cc [email protected]

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Archit Taneja <[email protected]>

---
 Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

Didn't I ack this already? Anyway,

Acked-by Rob Herring <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html