[0/3] drm/bridge: display-connector: add external supply support

On some devices the 5V pin of the HDMI connector and/or the ESD
protection logic is powered on by a separate regulator. The dp-connector
for such usecases provides dp-pwr supply support. Follow this example
and make hdmi-connector support the hdmi-pwr supply.

Dmitry Baryshkov (3):
  dt-bindings: display: hdmi-connector: add hdmi-pwr supply
  drm/bridge: display-connector: rename dp_pwr to connector_pwr
  drm/bridge: display-connector: handle hdmi-pwr supply

 .../display/connector/hdmi-connector.yaml     |  3 ++
 drivers/gpu/drm/bridge/display-connector.c    | 51 +++++++++++++------
 2 files changed, 39 insertions(+), 15 deletions(-)

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;