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

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;