[PATCH 2/3] drm/bridge: adv7533: Initialize regulators

[PATCH 2/3] drm/bridge: adv7533: Initialize regulators

[Archit Taneja]

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: dri-devel@lists.freedesktop.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 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(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index 161c923..3fcb202 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -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;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index ec8fb2e..221bc75 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -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)
 		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
 	 * 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)
-		return ret;
+		goto uninit_regulators;
 
 	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
 	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) {
 		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);
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c
index 5eebd15..03a59fd 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
@@ -178,6 +178,51 @@ err:
 	return ret;
 }
 
+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);
+
+	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);
+	if (ret)
+		goto err_v1p2;
+
+	ret = regulator_enable(adv->v3p3);
+	if (ret)
+		goto err_v3p3;
+
+	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;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/bridge: adv7533: Initialize regulators

[Laurent Pinchart]

Hi Archit,

Thank you for the patch.

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.

> Cc: dri-devel@lists.freedesktop.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 161c923..3fcb202 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -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;
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index ec8fb2e..221bc75
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -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)
>  		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
>  	 * 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)
> -		return ret;
> +		goto uninit_regulators;
> 
>  	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, 
edid_i2c_addr);
>  	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) {
>  		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);
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c
> b/drivers/gpu/drm/bridge/adv7511/adv7533.c index 5eebd15..03a59fd 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
> @@ -178,6 +178,51 @@ err:
>  	return ret;
>  }
> 
> +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 ?

> +	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);
> +	if (ret)
> +		goto err_v1p2;
> +
> +	ret = regulator_enable(adv->v3p3);
> +	if (ret)
> +		goto err_v3p3;

How about using the devm_regulator_bulk_*() API ?

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] drm/bridge: adv7533: Initialize regulators

[Archit Taneja]

Hi Laurent,

On 8/31/2016 9:23 PM, Laurent Pinchart wrote:
> Hi Archit,
>
> Thank you for the patch.
>
> 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.

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

>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>>   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(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 161c923..3fcb202 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -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;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index ec8fb2e..221bc75
>> 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -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)
>>   		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
>>   	 * 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)
>> -		return ret;
>> +		goto uninit_regulators;
>>
>>   	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> edid_i2c_addr);
>>   	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) {
>>   		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);
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7533.c index 5eebd15..03a59fd 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
>> @@ -178,6 +178,51 @@ err:
>>   	return ret;
>>   }
>>
>> +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 ?

Well, there isn't any DT upstream that doesn't declare these regulators.
The first DT file using ADV7533 would be merged in 4.9, and same for
this patch.

Also, if a DT file doesn't declare a regulator here (maybe because the
board has a constant supply to this pin since it's powered up), a dummy
regulator would be used here.

>
>> +	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);
>> +	if (ret)
>> +		goto err_v1p2;
>> +
>> +	ret = regulator_enable(adv->v3p3);
>> +	if (ret)
>> +		goto err_v3p3;
>
> How about using the devm_regulator_bulk_*() API ?

Yes, that makes more sense, should make the error handling
simpler too.

Thanks for the review,
Archit

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/bridge: adv7533: Initialize regulators

[Laurent Pinchart]

Hi Archit,

On Wednesday 31 Aug 2016 22:24:30 Archit Taneja wrote:
> On 8/31/2016 9:23 PM, Laurent Pinchart wrote:
> > 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.
> 
> 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 ?

> >> Cc: dri-devel@lists.freedesktop.org
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> >> ---
> >> 
> >>   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(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/bridge: adv7533: Initialize regulators

[Archit Taneja]

On 09/01/2016 02:00 AM, Laurent Pinchart wrote:
> Hi Archit,
>
> On Wednesday 31 Aug 2016 22:24:30 Archit Taneja wrote:
>> On 8/31/2016 9:23 PM, Laurent Pinchart wrote:
>>> 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.
>>
>> 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 ?

It is: dt-bindings: drm/bridge: adv7511: Add regulators for ADV7533

https://patchwork.kernel.org/patch/9306945/

Thanks,
Archit

>
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>>>> ---
>>>>
>>>>    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(-)
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/4] drm/bridge: adv7511: Initialize regulators

[Archit Taneja]

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: dri-devel@lists.freedesktop.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
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(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index 161c923..83ebdaa 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -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,9 @@ struct adv7511 {
 
 	struct gpio_desc *gpio_pd;
 
+	struct regulator_bulk_data *supplies;
+	int num_supplies;
+
 	/* ADV7533 DSI RX related params */
 	struct device_node *host_node;
 	struct mipi_dsi_device *dsi;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 8ed3906..f7e79ed 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -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",
+	"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;
+
+	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;
+	}
 
 	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
 		ret = adv7533_patch_registers(adv7511);
 	if (ret)
-		return ret;
+		goto uninit_regulators;
 
 	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
 	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) {
 		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);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/4] drm/bridge: adv7511: Initialize regulators

[Laurent Pinchart]

Hi Archit,

Thank you for the patch.

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: dri-devel@lists.freedesktop.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
> 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(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 161c923..83ebdaa 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -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,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.

> +
>  	/* ADV7533 DSI RX related params */
>  	struct device_node *host_node;
>  	struct mipi_dsi_device *dsi;
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8ed3906..f7e79ed
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -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",
> +	"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 <laurent.pinchart@ideasonboard.com>

> +
> +	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;
> +	}
> 
>  	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
>  		ret = adv7533_patch_registers(adv7511);
>  	if (ret)
> -		return ret;
> +		goto uninit_regulators;
> 
>  	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, 
edid_i2c_addr);
>  	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) {
>  		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);

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/4] drm/bridge: adv7511: Initialize regulators

[Archit Taneja]

On 9/25/2016 11:05 PM, Laurent Pinchart wrote:
> Hi Archit,
>
> Thank you for the patch.
>
> 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: dri-devel@lists.freedesktop.org
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 161c923..83ebdaa 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -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,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.
>
>> +
>>   	/* ADV7533 DSI RX related params */
>>   	struct device_node *host_node;
>>   	struct mipi_dsi_device *dsi;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8ed3906..f7e79ed
>> 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -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",
>> +	"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 <laurent.pinchart@ideasonboard.com>

I'll fix these. Thanks for the review.

Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 0/2] drm/bridge: adv7511: Enable regulators

[Archit Taneja]

The patch below added regulator supplies to the ADV533 HDMI bridge
on the DB410c, but the corresponding DT binding doc wasn't updated
to list them:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=28546b09551190c727c94d1c5c96ca609065beb2

The first patch adds the regulator supply props to the binding
docs for both ADV7511 and ADV7533. The second patch updates the drm
bridge driver to enable these regulators.

Changes in v3:
- Revert back to having a common supply for AVDD, DVDD, PVDD pins.
- Dropped the DB410c DT patches.

Changes in v2:
- Provide the regulator supply for ADV7511 too in the binding docs.
- Update the driver to manage regulators for both ADV7511 and ADV7533.
- Have separate supply entries for AVDD, DVDD, PVDD, A2VDD pins.
- Use regulator_bulk_* API to configure regulators.

Archit Taneja (2):
  dt-bindings: drm/bridge: adv7511: Add regulator bindings
  drm/bridge: adv7511: Initialize regulators

 .../bindings/display/bridge/adi,adv7511.txt        |  9 +++
 drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 ++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 79 +++++++++++++++++++---
 3 files changed, 83 insertions(+), 9 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Archit Taneja]

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: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
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
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -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
+  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:
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Laurent Pinchart]

Hi Archit,

Thank you for the patch.

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

Apart from that,

Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
> 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
> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> @@ -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
> +  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:

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Archit Taneja]

On 11/29/2016 12:03 PM, Laurent Pinchart wrote:
> Hi Archit,
>
> Thank you for the patch.
>
> 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?

Thanks,
Archit

>
> Apart from that,
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>> Cc: devicetree@vger.kernel.org
>> Acked-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>> 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
>> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> @@ -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
>> +  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:
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Laurent Pinchart]

Hi Archit,

(CC'ing Mark Brown)

On Tuesday 29 Nov 2016 13:41:33 Archit Taneja wrote:
> On 11/29/2016 12:03 PM, Laurent Pinchart wrote:
> > 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.

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

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

> > Apart from that,
> > 
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> >> Cc: devicetree@vger.kernel.org
> >> Acked-by: Rob Herring <robh@kernel.org>
> >> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> >> ---
> >> 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
> >> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> 
> >> @@ -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
> >> +  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:

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Mark Brown]

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

On Tue, Nov 29, 2016 at 11:11:03AM +0200, Laurent Pinchart wrote:
> 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.

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

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Laurent Pinchart]

Hi Mark,

On Tuesday 29 Nov 2016 11:01:25 Mark Brown wrote:
> On Tue, Nov 29, 2016 at 11:11:03AM +0200, Laurent Pinchart wrote:
> > 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.
> 
> >> 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 ?
> 
> 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.

Sounds good to me, thanks.

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

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1006 bytes --]

On Tue, Nov 29, 2016 at 09:37:31PM +0200, Laurent Pinchart wrote:
> 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 ;-)

That helps if I open the mail (especially with large e-mails, I do
sometimes get bored looking for something relevant) but I also filter
just on subject lines - once you've been involved with a thread about a
topic people often seem to end up copying you for anything even vaguely
related unfortunately.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Bjorn Andersson]

On Tue 29 Nov 01:11 PST 2016, Laurent Pinchart wrote:

> Hi Archit,
> 
> (CC'ing Mark Brown)
> 
> On Tuesday 29 Nov 2016 13:41:33 Archit Taneja wrote:
> > On 11/29/2016 12:03 PM, Laurent Pinchart wrote:
> > > 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.
> 
> 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.

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

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)

Regards,
Bjorn

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Laurent Pinchart]

Hi Bjorn,

On Monday 05 Dec 2016 13:11:51 Bjorn Andersson wrote:
> On Tue 29 Nov 01:11 PST 2016, Laurent Pinchart wrote:
> > On Tuesday 29 Nov 2016 13:41:33 Archit Taneja wrote:
> >> On 11/29/2016 12:03 PM, Laurent Pinchart wrote:
> >>> 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.
> > 
> > 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.
> 
> >> 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.
> 
> 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.

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

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1114 bytes --]

On Mon, Dec 05, 2016 at 11:16:22PM +0200, Laurent Pinchart wrote:
> 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.

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Laurent Pinchart]

Hi Mark,

On Tuesday 06 Dec 2016 10:05:17 Mark Brown wrote:
> On Mon, Dec 05, 2016 at 11:16:22PM +0200, Laurent Pinchart wrote:
> > 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.
> 
> 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.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1141 bytes --]

On Tue, Dec 06, 2016 at 02:46:55PM +0200, Laurent Pinchart wrote:
> On Tuesday 06 Dec 2016 10:05:17 Mark Brown wrote:
> > On Mon, Dec 05, 2016 at 11:16:22PM +0200, Laurent Pinchart wrote:

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

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Laurent Pinchart]

Hi Mark,

On Tuesday 06 Dec 2016 13:20:20 Mark Brown wrote:
> On Tue, Dec 06, 2016 at 02:46:55PM +0200, Laurent Pinchart wrote:
> > On Tuesday 06 Dec 2016 10:05:17 Mark Brown wrote:
> > > On Mon, Dec 05, 2016 at 11:16:22PM +0200, Laurent Pinchart wrote:
> > > > 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.
> 
> 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 :-/

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Mark Brown]

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

On Tue, Dec 06, 2016 at 06:08:55PM +0200, Laurent Pinchart wrote:
> 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 :-/

regulator_bulk_()! :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 2/2] drm/bridge: adv7511: Initialize regulators

[Archit Taneja]

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 <architt@codeaurora.org>
---
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(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index 992d76c..18ec627 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -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>
@@ -329,6 +330,9 @@ struct adv7511 {
 
 	struct gpio_desc *gpio_pd;
 
+	struct regulator_bulk_data *supplies;
+	int num_supplies;
+
 	/* ADV7533 DSI RX related params */
 	struct device_node *host_node;
 	struct mipi_dsi_device *dsi;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 2177440..f123fbb 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -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[] = {
+	"avdd",
+	"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;
+
+	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)
 		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;
+	}
 
 	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
 		ret = adv7533_patch_registers(adv7511);
 	if (ret)
-		return ret;
+		goto uninit_regulators;
 
 	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
 	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) {
 		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);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v3 2/2] drm/bridge: adv7511: Initialize regulators

[Laurent Pinchart]

Hi Archit,

Thank you for the patch.

On Tuesday 29 Nov 2016 11:37:42 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.
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
> 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(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 992d76c..18ec627 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -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>
> @@ -329,6 +330,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.

> +
>  	/* ADV7533 DSI RX related params */
>  	struct device_node *host_node;
>  	struct mipi_dsi_device *dsi;
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 2177440..f123fbb
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -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[] = {
> +	"avdd",
> +	"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;

Same comment for i.

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +	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)
>  		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;
> +	}
> 
>  	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
>  		ret = adv7533_patch_registers(adv7511);
>  	if (ret)
> -		return ret;
> +		goto uninit_regulators;
> 
>  	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, 
edid_i2c_addr);
>  	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) {
>  		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);

-- 
Regards,

Laurent Pinchart

[PATCH v4 0/2] drm/bridge: adv7511: Enable regulators

[Archit Taneja]

The patch below added regulator supplies to the ADV533 HDMI bridge
on the DB410c, but the corresponding DT binding doc wasn't updated
to list them:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=28546b09551190c727c94d1c5c96ca609065beb2

The first patch adds the regulator supply props to the binding
docs for both ADV7511 and ADV7533. The second patch updates the drm
bridge driver to enable these regulators.

Changes in v4:
- Moved the regulator supply bindings from optional to mandatory.
- Used unsigned int for num_supplies and iterator used for parsing
the regulator supply properties.

Changes in v3:
- Revert back to having a common supply for AVDD, DVDD, PVDD pins.
- Dropped the DB410c DT patches.

Changes in v2:
- Provide the regulator supply for ADV7511 too in the binding docs.
- Update the driver to manage regulators for both ADV7511 and ADV7533.
- Have separate supply entries for AVDD, DVDD, PVDD, A2VDD pins.
- Use regulator_bulk_* API to configure regulators.


Archit Taneja (2):
  dt-bindings: drm/bridge: adv7511: Add regulator bindings
  drm/bridge: adv7511: Initialize regulators

 .../bindings/display/bridge/adi,adv7511.txt        |  8 +++
 drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 ++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 80 +++++++++++++++++++---
 3 files changed, 83 insertions(+), 9 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v4 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Archit Taneja]

Add the regulator supply properties needed by ADV7511 and ADV7533.

Cc: devicetree@vger.kernel.org
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 6532a59..00ce479 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -38,10 +38,18 @@ The following input format properties are required except in "rgb 1x" and
 - adi,input-justification: The input bit justification ("left", "evenly",
   "right").
 
+- 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
+  up the A2VDD pin.
+- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on
+  ADV7511 and V3P3 on ADV7533.
+
 The following properties are required for ADV7533:
 
 - adi,dsi-lanes: Number of DSI data lanes connected to the DSI host. It should
   be one of 1, 2, 3 or 4.
+- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can be
+  either 1.2V or 1.8V. This supply is specific to ADV7533.
 
 Optional properties:
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Rob Herring]

On Mon, Dec 05, 2016 at 01:23:54PM +0530, Archit Taneja wrote:
> Add the regulator supply properties needed by ADV7511 and ADV7533.
> 
> Cc: devicetree@vger.kernel.org
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
>  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 <robh@kernel.org>

Re: [PATCH v4 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Archit Taneja]

On 12/10/2016 3:41 AM, Rob Herring wrote:
> On Mon, Dec 05, 2016 at 01:23:54PM +0530, Archit Taneja wrote:
>> Add the regulator supply properties needed by ADV7511 and ADV7533.
>>
>> Cc: devicetree@vger.kernel.org
>> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>
> Didn't I ack this already? Anyway,

You had Acked it before. This set moved the regulator bindings from
optional to mandatory, so I thought it may need you review again.

Mark Brown insisted that we have a regulator supply per pin even if
the chip's specs recommend using a common supply for them, so I'm
going to bring that back again. I'll keep your Ack for the next
revision since that's something you'd originally recommended.

Thanks,
Archit

>
> Acked-by: Rob Herring <robh@kernel.org>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 2/2] drm/bridge: adv7511: Initialize regulators

[Archit Taneja]

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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  4 ++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 80 ++++++++++++++++++++++++----
 2 files changed, 75 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index 992d76c..039147f 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -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>
@@ -329,6 +330,9 @@ struct adv7511 {
 
 	struct gpio_desc *gpio_pd;
 
+	struct regulator_bulk_data *supplies;
+	unsigned int num_supplies;
+
 	/* ADV7533 DSI RX related params */
 	struct device_node *host_node;
 	struct mipi_dsi_device *dsi;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 2177440..b5e61be 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -843,6 +843,52 @@ 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[] = {
+	"avdd",
+	"v3p3",
+	"v1p2",
+};
+
+static int adv7511_init_regulators(struct adv7511 *adv)
+{
+	struct device *dev = &adv->i2c_main->dev;
+	const char * const *supply_names;
+	unsigned int i;
+	int ret;
+
+	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 +989,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 +1007,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;
+	}
 
 	if (adv7511->gpio_pd) {
 		mdelay(5);
@@ -974,12 +1029,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 +1046,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	else
 		ret = adv7533_patch_registers(adv7511);
 	if (ret)
-		return ret;
+		goto uninit_regulators;
 
 	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
 	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
@@ -999,10 +1056,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) {
 		ret = adv7533_init_cec(adv7511);
@@ -1049,6 +1107,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 +1122,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);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 0/2] drm/bridge: adv7511: Enable regulators

[Archit Taneja]

The patch below added regulator supplies to the ADV533 HDMI bridge
on the DB410c, but the corresponding DT binding doc wasn't updated
to list them:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=28546b09551190c727c94d1c5c96ca609065beb2

The first patch adds the regulator supply props to the binding
docs for both ADV7511 and ADV7533. The second patch updates the drm
bridge driver to enable these regulators.

Changes in v5:
- Switch back to having individual supplies for AVDD, DVDD, PVDD
  pins.

Changes in v4:
- Moved the regulator supply bindings from optional to mandatory.
- Used unsigned int for num_supplies and iterator used for parsing
the regulator supply properties.

Changes in v3:
- Revert back to having a common supply for AVDD, DVDD, PVDD pins.
- Dropped the DB410c DT patches.

Changes in v2:
- Provide the regulator supply for ADV7511 too in the binding docs.
- Update the driver to manage regulators for both ADV7511 and ADV7533.
- Have separate supply entries for AVDD, DVDD, PVDD, A2VDD pins.
- Use regulator_bulk_* API to configure regulators.

Archit Taneja (2):
  dt-bindings: drm/bridge: adv7511: Add regulator bindings
  drm/bridge: adv7511: Initialize regulators

 .../bindings/display/bridge/adi,adv7511.txt        | 12 +++
 drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 86 +++++++++++++++++++---
 3 files changed, 93 insertions(+), 9 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings

[Archit Taneja]

Add the regulator supply properties needed by ADV7511 and ADV7533.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
v5:
- Bring back supplies for individual pins
- In v2, we had a v3p3-supply for DVDD_3V on ADV7511 and V3P3 pin
  on ADV7533. We don't really need to force a common name here
  (the adv7511 driver manages 2 different regulator name tables
  anyway).
  The supply on ADV7511 is called dvdd-3v-supply to match the
  pin name.

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

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 6532a59..00ea670 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -38,10 +38,22 @@ The following input format properties are required except in "rgb 1x" and
 - adi,input-justification: The input bit justification ("left", "evenly",
   "right").
 
+- 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.
+- dvdd-3v-supply: A 3.3V supply that powers up the pin called DVDD_3V
+  on the chip.
+- bgvdd-supply: A 1.8V supply that powers up the BGVDD pin. This is
+  needed only for ADV7511.
+
 The following properties are required for ADV7533:
 
 - adi,dsi-lanes: Number of DSI data lanes connected to the DSI host. It should
   be one of 1, 2, 3 or 4.
+- a2vdd-supply: 1.8V supply that powers up the A2VDD pin on the chip.
+- v3p3-supply: A 3.3V supply that powers up the V3P3 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.
 
 Optional properties:
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 2/2] drm/bridge: adv7511: Initialize regulators

[Archit Taneja]

Maintain a table of regulator names expected 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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  4 ++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 86 +++++++++++++++++++++++++---
 2 files changed, 81 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index 0396791..fe18a5d 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -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>
@@ -331,6 +332,9 @@ struct adv7511 {
 
 	struct gpio_desc *gpio_pd;
 
+	struct regulator_bulk_data *supplies;
+	unsigned int num_supplies;
+
 	/* ADV7533 DSI RX related params */
 	struct device_node *host_node;
 	struct mipi_dsi_device *dsi;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 24573e0..2f1aae7 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -859,6 +859,58 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge)
  * Probe & remove
  */
 
+static const char * const adv7511_supply_names[] = {
+	"avdd",
+	"dvdd",
+	"pvdd",
+	"bgvdd",
+	"dvdd-3v",
+};
+
+static const char * const adv7533_supply_names[] = {
+	"avdd",
+	"dvdd",
+	"pvdd",
+	"a2vdd",
+	"v3p3",
+	"v1p2",
+};
+
+static int adv7511_init_regulators(struct adv7511 *adv)
+{
+	struct device *dev = &adv->i2c_main->dev;
+	const char * const *supply_names;
+	unsigned int i;
+	int ret;
+
+	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)
 {
@@ -959,6 +1011,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;
 
@@ -976,13 +1029,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;
+	}
 
 	if (adv7511->gpio_pd) {
 		mdelay(5);
@@ -990,12 +1051,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)
@@ -1005,7 +1068,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	else
 		ret = adv7533_patch_registers(adv7511);
 	if (ret)
-		return ret;
+		goto uninit_regulators;
 
 	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
 	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
@@ -1015,10 +1078,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) {
 		ret = adv7533_init_cec(adv7511);
@@ -1067,6 +1131,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;
 }
@@ -1080,6 +1146,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);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation