| Message ID | 1472640730-24326-3-git-send-email-architt@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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; >
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(-)
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(-) >
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;
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(-)