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 at lists.freedesktop.org >> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com> >> Signed-off-by: Archit Taneja <architt at 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