Message ID | 20230507201218.2339014-1-dmitry.baryshkov@linaro.org |
---|---|
Headers | show |
Series | drm/bridge: display-connector: add external supply support | expand |
Hi Dmitry, Thank you for the patch. On Sun, May 07, 2023 at 11:12:17PM +0300, Dmitry Baryshkov wrote: > In preparation to adding support for the hdmi_pwr supply, rename dp_pwr > structure field to the generic connector_pwr. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/bridge/display-connector.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c > index 9a12449ad7b8..0d94e6edea50 100644 > --- a/drivers/gpu/drm/bridge/display-connector.c > +++ b/drivers/gpu/drm/bridge/display-connector.c > @@ -24,7 +24,7 @@ struct display_connector { > struct gpio_desc *hpd_gpio; > int hpd_irq; > > - struct regulator *dp_pwr; > + struct regulator *connector_pwr; This makes sense, but I would shorten the name to just "pwr", "power" or "supply". The field is part of the display_connector structure, so it implicitly refers to the connector. With or without that change, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > struct gpio_desc *ddc_en; > }; > > @@ -319,14 +319,14 @@ static int display_connector_probe(struct platform_device *pdev) > if (type == DRM_MODE_CONNECTOR_DisplayPort) { > int ret; > > - conn->dp_pwr = devm_regulator_get_optional(&pdev->dev, "dp-pwr"); > + conn->connector_pwr = devm_regulator_get_optional(&pdev->dev, "dp-pwr"); > > - if (IS_ERR(conn->dp_pwr)) { > - ret = PTR_ERR(conn->dp_pwr); > + if (IS_ERR(conn->connector_pwr)) { > + ret = PTR_ERR(conn->connector_pwr); > > switch (ret) { > case -ENODEV: > - conn->dp_pwr = NULL; > + conn->connector_pwr = NULL; > break; > > case -EPROBE_DEFER: > @@ -338,8 +338,8 @@ static int display_connector_probe(struct platform_device *pdev) > } > } > > - if (conn->dp_pwr) { > - ret = regulator_enable(conn->dp_pwr); > + if (conn->connector_pwr) { > + ret = regulator_enable(conn->connector_pwr); > if (ret) { > dev_err(&pdev->dev, "failed to enable DP PWR regulator: %d\n", ret); > return ret; > @@ -389,8 +389,8 @@ static int display_connector_remove(struct platform_device *pdev) > if (conn->ddc_en) > gpiod_set_value(conn->ddc_en, 0); > > - if (conn->dp_pwr) > - regulator_disable(conn->dp_pwr); > + if (conn->connector_pwr) > + regulator_disable(conn->connector_pwr); > > drm_bridge_remove(&conn->bridge); >
Hi Dmitry, Thank you for the patch. On Sun, May 07, 2023 at 11:12:18PM +0300, Dmitry Baryshkov wrote: > On some devices the 5V pin of the HDMI connector and/or the ESD > protection logic is powered on by a separate regulator. Instead of > declaring this regulator as always-on, make hdmi-connector support the > additional hdmi-pwr supply. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/bridge/display-connector.c | 37 +++++++++++++++++----- > 1 file changed, 29 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c > index 0d94e6edea50..037ba2eb5a2f 100644 > --- a/drivers/gpu/drm/bridge/display-connector.c > +++ b/drivers/gpu/drm/bridge/display-connector.c > @@ -337,18 +337,12 @@ static int display_connector_probe(struct platform_device *pdev) > return ret; > } > } > - > - if (conn->connector_pwr) { > - ret = regulator_enable(conn->connector_pwr); > - if (ret) { > - dev_err(&pdev->dev, "failed to enable DP PWR regulator: %d\n", ret); > - return ret; > - } > - } > } > > /* enable DDC */ > if (type == DRM_MODE_CONNECTOR_HDMIA) { > + int ret; > + > conn->ddc_en = devm_gpiod_get_optional(&pdev->dev, "ddc-en", > GPIOD_OUT_HIGH); > > @@ -356,6 +350,33 @@ static int display_connector_probe(struct platform_device *pdev) > dev_err(&pdev->dev, "Couldn't get ddc-en gpio\n"); > return PTR_ERR(conn->ddc_en); > } > + > + conn->connector_pwr = devm_regulator_get_optional(&pdev->dev, "hdmi-pwr"); > + > + if (IS_ERR(conn->connector_pwr)) { > + ret = PTR_ERR(conn->connector_pwr); > + > + switch (ret) { > + case -ENODEV: > + conn->connector_pwr = NULL; > + break; > + > + case -EPROBE_DEFER: > + return -EPROBE_DEFER; > + > + default: > + dev_err(&pdev->dev, "failed to get HDMI PWR regulator: %d\n", ret); > + return ret; > + } > + } > + } Please share this logic with the DP code. You can move it to a separate function for instance, that would take the regulator name as a parameter. > + > + if (conn->connector_pwr) { > + ret = regulator_enable(conn->connector_pwr); > + if (ret) { > + dev_err(&pdev->dev, "failed to enable DP PWR regulator: %d\n", ret); > + return ret; > + } > } > > conn->bridge.funcs = &display_connector_bridge_funcs;