"hdmi-connector.yaml" bindings defines an optional property
"ddc-en-gpios" for a single gpio to enable DDC operation.
Usually this controls +5V power on the HDMI connector.
This +5V may also be needed for HPD.
This was not reflected in code.
Now, the driver activates the ddc gpio after probe and
deactivates after remove so it is "almost on".
But only if this driver is loaded (and not e.g. blacklisted
as module).
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
drivers/gpu/drm/bridge/display-connector.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
index d24f5b90feabf..555395e301096 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -24,6 +24,7 @@ struct display_connector {
int hpd_irq;
struct regulator *dp_pwr;
+ struct gpio_desc *ddc_en;
};
static inline struct display_connector *
@@ -345,6 +346,19 @@ static int display_connector_probe(struct platform_device *pdev)
}
}
+ /* enable DDC */
+ if (type == DRM_MODE_CONNECTOR_HDMIA) {
+ conn->ddc_en = devm_gpiod_get_optional(&pdev->dev, "ddc-en",
+ GPIOD_OUT_HIGH);
+
+ if (IS_ERR(conn->ddc_en)) {
+ dev_err(&pdev->dev, "Couldn't get ddc-en gpio\n");
+ return PTR_ERR(conn->ddc_en);
+ }
+
+ gpiod_set_value(conn->ddc_en, 1);
+ }
+
conn->bridge.funcs = &display_connector_bridge_funcs;
conn->bridge.of_node = pdev->dev.of_node;
@@ -373,6 +387,9 @@ static int display_connector_remove(struct platform_device *pdev)
{
struct display_connector *conn = platform_get_drvdata(pdev);
+ if (conn->ddc_en)
+ gpiod_set_value(conn->ddc_en, 0);
+
if (conn->dp_pwr)
regulator_disable(conn->dp_pwr);
--
2.33.0
Hi Nikolaus,
Le lun., janv. 31 2022 at 13:26:53 +0100, H. Nikolaus Schaller
<[email protected]> a ?crit :
> "hdmi-connector.yaml" bindings defines an optional property
> "ddc-en-gpios" for a single gpio to enable DDC operation.
>
> Usually this controls +5V power on the HDMI connector.
> This +5V may also be needed for HPD.
>
> This was not reflected in code.
>
> Now, the driver activates the ddc gpio after probe and
> deactivates after remove so it is "almost on".
>
> But only if this driver is loaded (and not e.g. blacklisted
> as module).
>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> drivers/gpu/drm/bridge/display-connector.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/display-connector.c
> b/drivers/gpu/drm/bridge/display-connector.c
> index d24f5b90feabf..555395e301096 100644
> --- a/drivers/gpu/drm/bridge/display-connector.c
> +++ b/drivers/gpu/drm/bridge/display-connector.c
> @@ -24,6 +24,7 @@ struct display_connector {
> int hpd_irq;
>
> struct regulator *dp_pwr;
> + struct gpio_desc *ddc_en;
> };
>
> static inline struct display_connector *
> @@ -345,6 +346,19 @@ static int display_connector_probe(struct
> platform_device *pdev)
> }
> }
>
> + /* enable DDC */
> + if (type == DRM_MODE_CONNECTOR_HDMIA) {
> + conn->ddc_en = devm_gpiod_get_optional(&pdev->dev, "ddc-en",
> + GPIOD_OUT_HIGH);
> +
> + if (IS_ERR(conn->ddc_en)) {
> + dev_err(&pdev->dev, "Couldn't get ddc-en gpio\n");
> + return PTR_ERR(conn->ddc_en);
> + }
> +
> + gpiod_set_value(conn->ddc_en, 1);
You already requested the gpio with the GPIOD_OUT_HIGH flag, so this
can be removed.
> + }
> +
> conn->bridge.funcs = &display_connector_bridge_funcs;
> conn->bridge.of_node = pdev->dev.of_node;
>
> @@ -373,6 +387,9 @@ static int display_connector_remove(struct
> platform_device *pdev)
> {
> struct display_connector *conn = platform_get_drvdata(pdev);
>
> + if (conn->ddc_en)
> + gpiod_set_value(conn->ddc_en, 0);
Note that gpiod_set_value() already does the null-check internally. I
actually do prefer your solution, so this is fine with me, but
maintainers may have a different opinion.
Cheers,
-Paul
> +
> if (conn->dp_pwr)
> regulator_disable(conn->dp_pwr);
>
> --
> 2.33.0
>
Hi Paul,
> Am 02.02.2022 um 11:32 schrieb Paul Cercueil <[email protected]>:
>
> Hi Nikolaus,
>
> Le lun., janv. 31 2022 at 13:26:53 +0100, H. Nikolaus Schaller <[email protected]> a écrit :
>> "hdmi-connector.yaml" bindings defines an optional property
>> "ddc-en-gpios" for a single gpio to enable DDC operation.
>> Usually this controls +5V power on the HDMI connector.
>> This +5V may also be needed for HPD.
>> This was not reflected in code.
>> Now, the driver activates the ddc gpio after probe and
>> deactivates after remove so it is "almost on".
>> But only if this driver is loaded (and not e.g. blacklisted
>> as module).
>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>> ---
>> drivers/gpu/drm/bridge/display-connector.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>> diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
>> index d24f5b90feabf..555395e301096 100644
>> --- a/drivers/gpu/drm/bridge/display-connector.c
>> +++ b/drivers/gpu/drm/bridge/display-connector.c
>> @@ -24,6 +24,7 @@ struct display_connector {
>> int hpd_irq;
>> struct regulator *dp_pwr;
>> + struct gpio_desc *ddc_en;
>> };
>> static inline struct display_connector *
>> @@ -345,6 +346,19 @@ static int display_connector_probe(struct platform_device *pdev)
>> }
>> }
>> + /* enable DDC */
>> + if (type == DRM_MODE_CONNECTOR_HDMIA) {
>> + conn->ddc_en = devm_gpiod_get_optional(&pdev->dev, "ddc-en",
>> + GPIOD_OUT_HIGH);
>> +
>> + if (IS_ERR(conn->ddc_en)) {
>> + dev_err(&pdev->dev, "Couldn't get ddc-en gpio\n");
>> + return PTR_ERR(conn->ddc_en);
>> + }
>> +
>> + gpiod_set_value(conn->ddc_en, 1);
>
> You already requested the gpio with the GPIOD_OUT_HIGH flag, so this can be removed.
Ah, ok!
>
>
>> + }
>> +
>> conn->bridge.funcs = &display_connector_bridge_funcs;
>> conn->bridge.of_node = pdev->dev.of_node;
>> @@ -373,6 +387,9 @@ static int display_connector_remove(struct platform_device *pdev)
>> {
>> struct display_connector *conn = platform_get_drvdata(pdev);
>> + if (conn->ddc_en)
>> + gpiod_set_value(conn->ddc_en, 0);
>
> Note that gpiod_set_value() already does the null-check internally.
Indeed.
> I actually do prefer your solution, so this is fine with me, but maintainers may have a different opinion.
I am fine with any of them. Just need to know which one to take (and test).
BR,
Nikolaus
>
> Cheers,
> -Paul
>
>> +
>> if (conn->dp_pwr)
>> regulator_disable(conn->dp_pwr);
>> --
>> 2.33.0
>
>