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@xxxxxxxxxxxxxxxxxxxxx
Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Signed-off-by: Archit Taneja <architt@xxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
