Hi,

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com wrote:

The AUX adapter registered in probe() need to be unregistered in remove(). Do so.

Fixes: b814ec6d4535 ("drm/bridge: ti-sn65dsi86: Implement AUX channel") Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

---

drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index da78a12e58b5..c45420a50e73 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1307,6 +1307,9 @@ static int ti_sn_bridge_remove(struct i2c_client *client) return -EINVAL;

    kfree(pdata->edid);

•   drm_dp_aux_unregister(&pdata->aux);
  

•   ti_sn_debugfs_remove(pdata);
  
    of_node_put(pdata->host_node);
  

Good catch. One question, though. I know DRM sometimes has different conventions than the rest of the kernel, but I always look for the "remove" to be backwards of probe. That means that your code (and probably most of the remove function) should come _after_ the drm_bridge_remove(), right? ...since drm_bridge_add() was the last thing in probe then drm_bridge_remove() should be the first thing in remove?

-Doug

Hi,

On Tue, Mar 23, 2021 at 2:42 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

Hi Doug,

On Tue, Mar 23, 2021 at 02:08:42PM -0700, Doug Anderson wrote:

...

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote:

...

The AUX adapter registered in probe() need to be unregistered in remove(). Do so.

Fixes: b814ec6d4535 ("drm/bridge: ti-sn65dsi86: Implement AUX channel") Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

---

drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index da78a12e58b5..c45420a50e73 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1307,6 +1307,9 @@ static int ti_sn_bridge_remove(struct i2c_client *client) return -EINVAL;

    kfree(pdata->edid);

•   drm_dp_aux_unregister(&pdata->aux);
  

•   ti_sn_debugfs_remove(pdata);
  
    of_node_put(pdata->host_node);
  

Good catch. One question, though. I know DRM sometimes has different conventions than the rest of the kernel, but I always look for the "remove" to be backwards of probe. That means that your code (and probably most of the remove function) should come _after_ the drm_bridge_remove(), right? ...since drm_bridge_add() was the last thing in probe then drm_bridge_remove() should be the first thing in remove?

I agree in theory, yes. However, in practice, if you remove a bridge that is currently in use, all hell will break lose. And if the bridge isn't being used, it makes no difference. Still, it's worth changing the order of operations to move drm_bridge_remove() first, as it won't hurt in any case and is logically better. It's not an issue introduced by this series though, so how how about it on top, or in parallel ?

Sure, it can be a separate patch. I'd kinda prefer it be a patch _before_ ${SUBJECT} patch, though. Specifically it's harder for me to reason about whether your new function call is in the right place and won't cause any problems with the order being all jumbled. If we fix the order first then it's easy to reason about your patch.

You can even submit a patch if you want :-)

Happy to post it up if it won't cause more confusion w/ you posting your next version and trying to figure out what to base it on (since it will definitely conflict with your series).

-Doug

Hi,

On Tue, Mar 23, 2021 at 4:03 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

Hi Doug,

On Tue, Mar 23, 2021 at 03:55:05PM -0700, Doug Anderson wrote:

...

On Tue, Mar 23, 2021 at 2:42 PM Laurent Pinchart wrote:

...

On Tue, Mar 23, 2021 at 02:08:42PM -0700, Doug Anderson wrote:

...

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote:

...

The AUX adapter registered in probe() need to be unregistered in remove(). Do so.

Fixes: b814ec6d4535 ("drm/bridge: ti-sn65dsi86: Implement AUX channel") Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

---

drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index da78a12e58b5..c45420a50e73 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1307,6 +1307,9 @@ static int ti_sn_bridge_remove(struct i2c_client *client) return -EINVAL;

    kfree(pdata->edid);

•   drm_dp_aux_unregister(&pdata->aux);
  

•   ti_sn_debugfs_remove(pdata);
  
    of_node_put(pdata->host_node);
  

Good catch. One question, though. I know DRM sometimes has different conventions than the rest of the kernel, but I always look for the "remove" to be backwards of probe. That means that your code (and probably most of the remove function) should come _after_ the drm_bridge_remove(), right? ...since drm_bridge_add() was the last thing in probe then drm_bridge_remove() should be the first thing in remove?

I agree in theory, yes. However, in practice, if you remove a bridge that is currently in use, all hell will break lose. And if the bridge isn't being used, it makes no difference. Still, it's worth changing the order of operations to move drm_bridge_remove() first, as it won't hurt in any case and is logically better. It's not an issue introduced by this series though, so how how about it on top, or in parallel ?

Sure, it can be a separate patch. I'd kinda prefer it be a patch _before_ ${SUBJECT} patch, though. Specifically it's harder for me to reason about whether your new function call is in the right place and won't cause any problems with the order being all jumbled. If we fix the order first then it's easy to reason about your patch.

...

You can even submit a patch if you want :-)

Happy to post it up if it won't cause more confusion w/ you posting your next version and trying to figure out what to base it on (since it will definitely conflict with your series).

I'll need quite a bit of time before v2, as I'd like to test it, and that requires finishing support for the DSI bridge and the display controller :-) Please feel free to post a patch if you have time, I think it could get merged in drm-misc quite quickly.

I haven't forgotten about this and I've got it written, but I'm trying to put it together with the work I'm doing to fix EDID reading and that's still going to take me a while longer. I'm out tomorrow but _hoping_ that I'll be able to at least get a new patch series (at least RFC quality) next week...

-Doug

Hi,

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com wrote:

The valid rates are stored in an array of 8 booleans. Replace it with a bitmask to save space.

I'm curious: do you have evidence that this does anything useful? I guess you're expecting it to save .text space, right? Stack usage and execution time differences should be irrelevant--it's not in a critical section and the difference should be tiny anyway. As far as .text segment goes, it's not obvious to me that the compiler will use fewer instructions to manipulate bits compared to booleans.

Doing a super simple "ls -ah" on vmlinux (unstripped):

Before: 224820232 bytes After: 224820376 bytes

...so your change made it _bigger_. OK, so running "strip --strip-debug" on those:

Before: 26599464 bytes After: 26599464 bytes

...so exactly the same. I tried finding some evidence using "readelf -ah":

Before: [ 2] .text PROGBITS ffffffc010010000 00020000 0000000000b03508 0000000000000000 WAX 0 0 65536 [ 3] .rodata PROGBITS ffffffc010b20000 00b30000 00000000002e84b3 0000000000000000 WAMS 0 0 4096

After: [ 2] .text PROGBITS ffffffc010010000 00020000 0000000000b03508 0000000000000000 WAX 0 0 65536 [ 3] .rodata PROGBITS ffffffc010b20000 00b30000 00000000002e84b3 0000000000000000 WAMS 0 0 4096

Maybe you have some evidence showing an improvement? Ah, OK. I disassembled ti_sn_bridge_enable() and your patch saves 12 bytes, but I guess maybe alignment washes it out in reality...

In terms of readability / conventions, I personally find this change a bit of a wash. I mean, I guess I originally implemented it as an array and I thought that was the most readable, but I like bitfields fine too. If everyone loves it then I won't object, but to me it feels like touching lines of code for something that's personal preference. ;-)

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/bridge/ti-sn65dsi86.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index c45420a50e73..1d1be791d5ba 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -557,9 +557,9 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn_bridge *pdata) return i; }

-static void ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata,

•                                     bool rate_valid[])
  

+static unsigned int ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata) {

•   unsigned int valid_rates = 0;
    unsigned int rate_per_200khz;
    unsigned int rate_mhz;
    u8 dpcd_val;
  

@@ -599,13 +599,13 @@ static void ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata, j < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); j++) { if (ti_sn_bridge_dp_rate_lut[j] == rate_mhz)

•                                   rate_valid[j] = true;
  

•                                   valid_rates |= BIT(j);
                    }
            }
  
            for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); i++) {
  

•                   if (rate_valid[i])
  

•                           return;
  

•                   if (valid_rates & BIT(i))
  

•                           return valid_rates;
            }
            DRM_DEV_ERROR(pdata->dev,
                          "No matching eDP rates in table; falling back\n");
  

@@ -627,15 +627,17 @@ static void ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata, (int)dpcd_val); fallthrough; case DP_LINK_BW_5_4:

•           rate_valid[7] = 1;
  

•           valid_rates |= BIT(7);
            fallthrough;
    case DP_LINK_BW_2_7:
  

•           rate_valid[4] = 1;
  

•           valid_rates |= BIT(4);
            fallthrough;
    case DP_LINK_BW_1_62:
  

•           rate_valid[1] = 1;
  

•           valid_rates |= BIT(1);
            break;
    }
  

•   return valid_rates;
  

}

static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge *pdata) @@ -753,8 +755,8 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, static void ti_sn_bridge_enable(struct drm_bridge *bridge) { struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);

•   bool rate_valid[ARRAY_SIZE(ti_sn_bridge_dp_rate_lut)] = { };
    const char *last_err_str = "No supported DP rate";
  

•   unsigned int valid_rates;
    int dp_rate_idx;
    unsigned int val;
    int ret = -EINVAL;
  

@@ -793,13 +795,13 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge) regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG, DP_NUM_LANES_MASK, val);

•   ti_sn_bridge_read_valid_rates(pdata, rate_valid);
  

•   valid_rates = ti_sn_bridge_read_valid_rates(pdata);
  
    /* Train until we run out of rates */
    for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
         dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
         dp_rate_idx++) {
  

•           if (!rate_valid[dp_rate_idx])
  

•           if (!(valid_rates & BIT(dp_rate_idx)))
                    continue;
  
            ret = ti_sn_link_training(pdata, dp_rate_idx, &last_err_str);
  

In any case, since it does save 12 bytes:

Reviewed-by: Douglas Anderson dianders@chromium.org

Quoting Laurent Pinchart (2021-03-21 20:01:22)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 1d1be791d5ba..c21a7f7d452b 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -418,8 +420,18 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, } pdata->dsi = dsi;

•   /* Attach the next bridge */
  

•   ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
  

•                           &pdata->bridge, flags);
  

•   if (ret < 0) {
  

•           DRM_ERROR("failed to attach next bridge\n");
  

Can this be pushed into drm_bridge_attach() instead of in each caller?

•           goto err_dsi_detach;
  

•   }
  

•   return 0;
  
  

+err_dsi_detach:

•   mipi_dsi_detach(dsi);
  

err_dsi_attach: mipi_dsi_device_unregister(dsi); err_dsi_host: static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) @@ -1245,6 +1249,14 @@ static int ti_sn_bridge_probe(struct i2c_client *client, return ret; }

•   pdata->next_bridge = devm_drm_panel_bridge_add(pdata->dev,
  

•                                                  pdata->panel);
  

•   if (IS_ERR(pdata->next_bridge)) {
  

•           DRM_ERROR("failed to create panel bridge\n");
  

•           ret = PTR_ERR(pdata->next_bridge);
  

•           return ret;
  

Just return PTR_ERR(pdata->next_bridge)?

•   }
  

•   dev_set_drvdata(&client->dev, pdata);
  

Hi,

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com wrote:

To simplify interfacing with the panel, wrap it in a panel-bridge and let the DRM bridge helpers handle chaining of operations.

This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which requires all components in the display pipeline to be represented by bridges.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/bridge/ti-sn65dsi86.c | 30 +++++++++++++++++++-------- 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 1d1be791d5ba..c21a7f7d452b 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -124,6 +124,7 @@

• @edid: Detected EDID of eDP panel.
• @refclk: Our reference clock.
• @panel: Our panel.

• • @next_bridge: The next bridge.

To make it easier for folks who don't work with DRM all day, could you somehow clarify which direction "next" is talking about. AKA the next "outward" (towards the sink) or the next "inward" (toward the source)?

• @enable_gpio: The GPIO we toggle to enable the bridge.
• @supplies: Data for bulk enabling/disabling our regulators.
• @dp_lanes: Count of dp_lanes we're using.

@@ -152,6 +153,7 @@ struct ti_sn_bridge { struct mipi_dsi_device *dsi; struct clk *refclk; struct drm_panel *panel;

•   struct drm_bridge               *next_bridge;
  

There's no reason to store the "panel" pointer anymore, right? It can just be a local variable in probe?

@@ -850,8 +856,6 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge) */ regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE, HPD_DISABLE);

•   drm_panel_prepare(pdata->panel);
  

Ugh, I guess conflicts with my EDID patch [1] which assumes that this function will directly turn the panel on. I'll see if I can find some solution...

[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c7...

Hi,

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com wrote:

Implement the bridge connector-related .get_edid() operation, and report the related bridge capabilities and type. The .get_edid() operation is implemented with the same backend as the EDID retrieval from the connector .get_modes() operation.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/bridge/ti-sn65dsi86.c | 30 ++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index dc300fab4319..6f6e075544e8 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -261,6 +261,18 @@ static void ti_sn_debugfs_remove(struct ti_sn_bridge *pdata) pdata->debugfs = NULL; }

+static struct edid *__ti_sn_bridge_get_edid(struct ti_sn_bridge *pdata,

•                                       struct drm_connector *connector)
  

+{

•   struct edid *edid;
  

•   pm_runtime_get_sync(pdata->dev);
  

•   edid = drm_get_edid(connector, &pdata->aux.ddc);
  

•   pm_runtime_put(pdata->dev);
  

As mentioned in my patch [1], the above is a bit iffy for eDP. Specifically just doing a pm_runtime_get_sync() isn't enough to actually be able to read the EDID. We also need to do any panel power sequencing and potentially set the "ignore HPD" bit in the bridge to actually read the EDID.

I'll try to find some time to make this better--let's see if I get distracted before I manage it.

•   return edid;
  

+}

/* -----------------------------------------------------------------------------

• DRM Connector Operations

*/ @@ -277,11 +289,8 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) struct edid *edid = pdata->edid; int num, ret;

•   if (!edid) {
  

•           pm_runtime_get_sync(pdata->dev);
  

•           edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
  

•           pm_runtime_put(pdata->dev);
  

•   }
  

•   if (!edid)
  

•           edid = pdata->edid = __ti_sn_bridge_get_edid(pdata, connector);
  

It feels weird to me that we are now exposing the get_edid() function directly but we still need to implement get_modes(). I guess this is because we might need to fallback to the hardcoded modes? ...but that seems like it would be a common pattern for eDP bridges...

    if (edid && drm_edid_is_valid(edid)) {
            ret = drm_connector_update_edid_property(connector, edid);

@@ -871,12 +880,21 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) pm_runtime_put_sync(pdata->dev); }

+static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,

•                                     struct drm_connector *connector)
  

+{

•   struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
  

•   return __ti_sn_bridge_get_edid(pdata, connector);
  

+}

static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .attach = ti_sn_bridge_attach, .pre_enable = ti_sn_bridge_pre_enable, .enable = ti_sn_bridge_enable, .disable = ti_sn_bridge_disable, .post_disable = ti_sn_bridge_post_disable,

•   .get_edid = ti_sn_bridge_get_edid,
  

};

/* ----------------------------------------------------------------------------- @@ -1335,6 +1353,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,

    pdata->bridge.funcs = &ti_sn_bridge_funcs;
    pdata->bridge.of_node = client->dev.of_node;

•   pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
  

•   pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
  

Even with the few comments above, I think this is still fine to move us in the right direction. Unless I manage to fix up the EDID reading (in which case your patch would conflict and would need to be tweaked), then:

Reviewed-by: Douglas Anderson dianders@chromium.org

[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c7...

Hi,

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com wrote:

Despite the SN65DSI86 being an eDP bridge, on some systems its output is routed to a DisplayPort connector. Enable DisplayPort mode when the next component in the display pipeline is not a panel, and disable eDP features in that case.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 ++++++++++++++++++++------- 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index e2527d597ccb..f792227142a7 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -55,6 +55,7 @@ #define SN_LN_ASSIGN_REG 0x59 #define LN_ASSIGN_WIDTH 2 #define SN_ENH_FRAME_REG 0x5A +#define ASSR_CONTROL BIT(0) #define VSTREAM_ENABLE BIT(3) #define LN_POLRS_OFFSET 4 #define LN_POLRS_MASK 0xf0 @@ -86,6 +87,8 @@ #define SN_DATARATE_CONFIG_REG 0x94 #define DP_DATARATE_MASK GENMASK(7, 5) #define DP_DATARATE(x) ((x) << 5) +#define SN_TRAINING_SETTING_REG 0x95 +#define SCRAMBLE_DISABLE BIT(4) #define SN_ML_TX_MODE_REG 0x96 #define ML_TX_MAIN_LINK_OFF 0 #define ML_TX_NORMAL_MODE BIT(0) @@ -723,6 +726,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));

•   /* For DisplayPort, use the standard DP scrambler seed. */
  

•   if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
  

•           regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
  

•                              ASSR_CONTROL, 0);
  

I don't actually know anything about DP scrambler seeds. However:

1. From reading the docs, this field seems to be documented to be "read only" unless:

1a) The "TEST2" pin is pulled high when you power on the bridge. 1b) You set "ASSR_OVERRIDE" (page select to page 7, write to register 0x16, page select back to page 0).

I don't know if TEST2 is being pulled high in your hardware, but at least I can see that 1b) isn't done. So I'm guessing that this line is a no-op? If I had to guess from all the hoops they're making you jump through there's some sort of errata around standard scrambling on this bridge chip. Are you sure it works OK?

2. The docs I see claim that this field is 2 bits big. It seems like it would be nice to honor. Yeah, it's silly because 0x11 and 0x10 are "reserved" so it's really more like a 1-bit field, but still seems like it would be better to set both bits, or at least add a comment explaining why you're not matching the datasheet.

3. Your patch doesn't seem to touch the bit of code in ti_sn_bridge_enable() that says this:

/** * The SN65DSI86 only supports ASSR Display Authentication method and * this method is enabled by default. An eDP panel must support this * authentication method. We need to enable this method in the eDP panel * at DisplayPort address 0x0010A prior to link training. */ drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);

Won't that be a problem?

•   /* enable DP PLL */
    regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
  
  

@@ -734,6 +742,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, goto exit; }

•   /* For DisplayPort, disable scrambling mode. */
  

•   if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
  

•           regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
  

•                              SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
  

I'm assuming that this is the important part of your patch? Would be sorta nice to include the "why" in your comment. Why do you want to disable scrambling mode for DP but not for eDP? Maybe you care about compatibility but not EMI if you're hooking up to random DP things?

-Doug

Hi Doug, Laurent,

Quoting Laurent Pinchart (2021-06-23 14:59:00)

Hi Doug,

On Wed, Mar 24, 2021 at 03:47:07PM -0700, Doug Anderson wrote:

...

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote:

...

Despite the SN65DSI86 being an eDP bridge, on some systems its output is routed to a DisplayPort connector. Enable DisplayPort mode when the next component in the display pipeline is not a panel, and disable eDP features in that case.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 ++++++++++++++++++++------- 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index e2527d597ccb..f792227142a7 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -55,6 +55,7 @@ #define SN_LN_ASSIGN_REG 0x59 #define LN_ASSIGN_WIDTH 2 #define SN_ENH_FRAME_REG 0x5A +#define ASSR_CONTROL BIT(0) #define VSTREAM_ENABLE BIT(3) #define LN_POLRS_OFFSET 4 #define LN_POLRS_MASK 0xf0 @@ -86,6 +87,8 @@ #define SN_DATARATE_CONFIG_REG 0x94 #define DP_DATARATE_MASK GENMASK(7, 5) #define DP_DATARATE(x) ((x) << 5) +#define SN_TRAINING_SETTING_REG 0x95 +#define SCRAMBLE_DISABLE BIT(4) #define SN_ML_TX_MODE_REG 0x96 #define ML_TX_MAIN_LINK_OFF 0 #define ML_TX_NORMAL_MODE BIT(0) @@ -723,6 +726,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));

•   /* For DisplayPort, use the standard DP scrambler seed. */
  

•   if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
  

•           regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
  

•                              ASSR_CONTROL, 0);
  

I don't actually know anything about DP scrambler seeds. However:

1. From reading the docs, this field seems to be documented to be

"read only" unless:

1a) The "TEST2" pin is pulled high when you power on the bridge. 1b) You set "ASSR_OVERRIDE" (page select to page 7, write to register 0x16, page select back to page 0).

I don't know if TEST2 is being pulled high in your hardware, but at least I can see that 1b) isn't done. So I'm guessing that this line is a no-op? If I had to guess from all the hoops they're making you jump through there's some sort of errata around standard scrambling on this bridge chip. Are you sure it works OK?

Good question :-) We managed to get the SN65DSI86 to work with an external DP monitor yesterday, so it's possible (some modes don't operate correctly yet, but I assume that to be an issue with the DSI encoder).

The TEST2 pin is strapped to ground on the board.

According to the DisplayPort specification, eDP and DP use different scrambler seeds to prevent interoperability between an eDP source and a DP sink. I'll check what happens without this change.

Without this change, the display still works...

...

2. The docs I see claim that this field is 2 bits big. It seems like

it would be nice to honor. Yeah, it's silly because 0x11 and 0x10 are "reserved" so it's really more like a 1-bit field, but still seems like it would be better to set both bits, or at least add a comment explaining why you're not matching the datasheet.

Sure.

...

3. Your patch doesn't seem to touch the bit of code in

ti_sn_bridge_enable() that says this:

/**

• The SN65DSI86 only supports ASSR Display Authentication method and
• this method is enabled by default. An eDP panel must support this
• authentication method. We need to enable this method in the eDP panel
• at DisplayPort address 0x0010A prior to link training.

*/ drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);

Won't that be a problem?

I'll have a look.

I'm not sure I yet fully understand the requirements here, but could it be that only supporting ASSR is why the scrambling is disabled below?

Commenting out that write does not affect the bring up of my DP monitor.

...

...

•   /* enable DP PLL */
    regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
  
  

@@ -734,6 +742,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, goto exit; }

•   /* For DisplayPort, disable scrambling mode. */
  

•   if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
  

•           regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
  

•                              SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
  

I'm assuming that this is the important part of your patch? Would be sorta nice to include the "why" in your comment. Why do you want to disable scrambling mode for DP but not for eDP? Maybe you care about compatibility but not EMI if you're hooking up to random DP things?

I'll investigate and include proper documentation in v2 (or drop the change altogether if it's not required).

And indeed, this part is important. If I drop this hunk - then I get no display output.

I'm afraid I don't (yet) know the reasons 'why' to extend the comment, beyond "Scrambling is not supported for DP".

If anyone already does, please feel free to provide the text, and I'll include it in the next revision, or I'll try to do some more digging into this part.

-- Kieran

-- Regards,

Laurent Pinchart

Quoting Laurent Pinchart (2021-03-21 20:01:28)

When the SN65DSI86 is used in DisplayPort mode, its output is likely routed to a DisplayPort connector, which can benefit from hotplug detection. Support it in such cases, with polling mode only for now.

The implementation is limited to the bridge operations, as the connector operations are legacy and new users should use DRM_BRIDGE_ATTACH_NO_CONNECTOR.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Reviewed-by: Stephen Boyd swboyd@chromium.org

drivers/gpu/drm/bridge/ti-sn65dsi86.c | 46 +++++++++++++++++++-------- 1 file changed, 33 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index f792227142a7..72f6362adf44 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -890,6 +897,15 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) pm_runtime_put_sync(pdata->dev); }

+static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge) +{

•   struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
  

•   int val;
  

•   regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
  

•   return val ? connector_status_connected : connector_status_disconnected;
  

+}

static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge, struct drm_connector *connector) { @@ -904,6 +920,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .enable = ti_sn_bridge_enable, .disable = ti_sn_bridge_disable, .post_disable = ti_sn_bridge_post_disable,

•   .detect = ti_sn_bridge_detect,
    .get_edid = ti_sn_bridge_get_edid,
  

};

@@ -1327,6 +1344,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, return ret; }

•   pdata->no_hpd = of_property_read_bool(pdata->dev->of_node, "no-hpd");
  

I see that we missed adding this property to the DTS file but skated by because it was the default in the driver. I don't think it's a big deal just something we should fix in sc7180-trogdor.dtsi before this patch is merged.

•   ti_sn_bridge_parse_lanes(pdata, client->dev.of_node);
  
    ret = ti_sn_bridge_parse_regulators(pdata);
  

Hi Doug,

On Wed, Mar 24, 2021 at 03:47:38PM -0700, Doug Anderson wrote:

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote:

...

When the SN65DSI86 is used in DisplayPort mode, its output is likely routed to a DisplayPort connector, which can benefit from hotplug detection. Support it in such cases, with polling mode only for now.

The implementation is limited to the bridge operations, as the connector operations are legacy and new users should use DRM_BRIDGE_ATTACH_NO_CONNECTOR.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/bridge/ti-sn65dsi86.c | 46 +++++++++++++++++++-------- 1 file changed, 33 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index f792227142a7..72f6362adf44 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -167,6 +167,8 @@ struct ti_sn_bridge { struct gpio_chip gchip; DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS); #endif

•   bool                            no_hpd;
  

This structure is documented by kernel-doc, but you didn't add your new member.

Oops, sorry.

...

};

static const struct regmap_range ti_sn_bridge_volatile_ranges[] = { @@ -862,23 +864,28 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge) ti_sn_bridge_set_refclk_freq(pdata);

    /*

•    * HPD on this bridge chip is a bit useless.  This is an eDP bridge
  

•    * so the HPD is an internal signal that's only there to signal that
  

•    * the panel is done powering up.  ...but the bridge chip debounces
  

•    * this signal by between 100 ms and 400 ms (depending on process,
  

•    * voltage, and temperate--I measured it at about 200 ms).  One
  

•    * As this is an eDP bridge, the output will be connected to a fixed
  

•    * panel in most systems. HPD is in that case only an internal signal
  

•    * to signal that the panel is done powering up. The bridge chip
  

•    * debounces this signal by between 100 ms and 400 ms (depending on
  

•    * process, voltage, and temperate--I measured it at about 200 ms). One
     * particular panel asserted HPD 84 ms after it was powered on meaning
     * that we saw HPD 284 ms after power on.  ...but the same panel said
     * that instead of looking at HPD you could just hardcode a delay of
  

•    * 200 ms.  We'll assume that the panel driver will have the hardcoded
  

•    * delay in its prepare and always disable HPD.
  

•    * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll
  

•    * assume that the panel driver will have the hardcoded delay in its
  

•    * prepare and always disable HPD.
     *
  

•    * If HPD somehow makes sense on some future panel we'll have to
  

•    * change this to be conditional on someone specifying that HPD should
  

•    * be used.
  

•    * However, on some systems, the output is connected to a DisplayPort
  

•    * connector. HPD is needed in such cases. To accommodate both use
  

•    * cases, enable HPD only when requested.
     */
  

•   regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
  

•                      HPD_DISABLE);
  

•   if (pdata->no_hpd)
  

•           regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
  

•                              HPD_DISABLE, HPD_DISABLE);
  

•   else
  

•           regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
  

•                              HPD_DISABLE, 0);
  

Optionally you could skip the "else". HPD enabled is the default state and, in general, we don't exhaustively init all registers and rely on the power-on defaults for ones we don't explicitly control.

OK.

...

}

static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) @@ -890,6 +897,15 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) pm_runtime_put_sync(pdata->dev); }

+static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge) +{

•   struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
  

•   int val;
  

•   regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
  

•   return val ? connector_status_connected : connector_status_disconnected;
  

I would have expected that you would have used the interrupt signal, but I guess it just polls in this case. I suppose polling has the advantage that it's simpler... Maybe throw in a comment about why IRQ isn't being used?

Correct, I didn't want to include IRQ support yet. I'll add a TODO comment.

...

+}

static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge, struct drm_connector *connector) { @@ -904,6 +920,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .enable = ti_sn_bridge_enable, .disable = ti_sn_bridge_disable, .post_disable = ti_sn_bridge_post_disable,

•   .detect = ti_sn_bridge_detect,
    .get_edid = ti_sn_bridge_get_edid,
  

};

u> >

...

@@ -1327,6 +1344,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, return ret; }

•   pdata->no_hpd = of_property_read_bool(pdata->dev->of_node, "no-hpd");
  

•   ti_sn_bridge_parse_lanes(pdata, client->dev.of_node);
  
    ret = ti_sn_bridge_parse_regulators(pdata);
  

@@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,

    pdata->bridge.funcs = &ti_sn_bridge_funcs;
    pdata->bridge.of_node = client->dev.of_node;

•   pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
  

•   pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT)
  

Checking for "no_hpd" here is not the right test IIUC. You want to check for eDP vs. DP (AKA whether a panel is downstream of you or a connector). Specifically if downstream of you is a panel then (I believe) HPD won't assert until you turn on the panel and you won't turn on the panel (which happens in pre_enable, right?) until HPD fires, so you've got a chicken-and-egg problem. If downstream of you is a connector, though, then by definition HPD has to just work without pre_enable running so then you're OK.

Agreed. It's even more true now that your rework has landed, as in the eDP case EDID is handled by the panel driver. I'll rework this.

Should I also condition setting HPD_DISABLE to the presence of a panel then ? I could drop of_property_read_bool() and set

pdata->no_hpd = !!panel;

I guess then you'd need to figure out what to do if someone wants to use "HPD" on eDP. Do you need to put a polling loop in pre_enable then? Or you could just punt not support this case until someone needs it.

I think I'll stop short of saving the world this time, yes :-) We'll see what to do when this case arises.

...

•                     | DRM_BRIDGE_OP_EDID;
  

IMO somewhere in here if HPD is being used like this you should throw in a call to pm_runtime_get_sync(). I guess in your solution the regulators (for the bridge, not the panel) and enable pin are just left on all the time,

Correct, on my development board the SN65DSI86 is on all the time, I can't control that.

but plausibly someone might want to build a system to use HPD and also have the enable pin and/or regulators controlled by this driver, right?

True. DRM doesn't make this very easy, as, as far as I can tell, there's no standard infrastructure for userspace to register an interest in HPD that could be notified to bridges. I think it should be fixable, but it's out of scope for this series :-) Should I still add a pm_runtime_get_sync() at probe time, or leave this to be addressed by someone who will need to implement power control ?

Hi All,

I'm working to respin the remainder of these patches, now that I have IRQ based HPD working on the SN65DSI86, and the (non-eDP) mode is used for Renesas R-Car boards.

Quoting Doug Anderson (2021-06-24 00:51:12)

Hi,

On Wed, Jun 23, 2021 at 4:26 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

...

...

...

@@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,

    pdata->bridge.funcs = &ti_sn_bridge_funcs;
    pdata->bridge.of_node = client->dev.of_node;

•   pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
  

•   pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT)
  

Checking for "no_hpd" here is not the right test IIUC. You want to check for eDP vs. DP (AKA whether a panel is downstream of you or a connector). Specifically if downstream of you is a panel then (I believe) HPD won't assert until you turn on the panel and you won't turn on the panel (which happens in pre_enable, right?) until HPD fires, so you've got a chicken-and-egg problem. If downstream of you is a connector, though, then by definition HPD has to just work without pre_enable running so then you're OK.

Agreed. It's even more true now that your rework has landed, as in the eDP case EDID is handled by the panel driver. I'll rework this.

Should I also condition setting HPD_DISABLE to the presence of a panel then ? I could drop of_property_read_bool() and set

    pdata->no_hpd = !!panel;

...

I guess then you'd need to figure out what to do if someone wants to use "HPD" on eDP. Do you need to put a polling loop in pre_enable then? Or you could just punt not support this case until someone needs it.

I think I'll stop short of saving the world this time, yes :-) We'll see what to do when this case arises.

How about as a compromise you still call of_property_read_bool() but add some type of warning in the logs if someone didn't set "no-hpd" but they have a panel?

Would a mix of the two work well?

What about:

pdata->no_hpd = of_property_read_bool(np, "no-hpd"); if (panel && !pdata->no_hpd) { DRM_ERROR("Panels will not function with HPD. Enforcing no-hpd\n"); pdata->no_hpd = true; }

That leaves it still optional to DP connectors, but enforced on eDP?

...

...

...

•                     | DRM_BRIDGE_OP_EDID;
  

IMO somewhere in here if HPD is being used like this you should throw in a call to pm_runtime_get_sync(). I guess in your solution the regulators (for the bridge, not the panel) and enable pin are just left on all the time,

Correct, on my development board the SN65DSI86 is on all the time, I can't control that.

...

but plausibly someone might want to build a system to use HPD and also have the enable pin and/or regulators controlled by this driver, right?

True. DRM doesn't make this very easy, as, as far as I can tell, there's no standard infrastructure for userspace to register an interest in HPD that could be notified to bridges. I think it should be fixable, but it's out of scope for this series :-) Should I still add a pm_runtime_get_sync() at probe time, or leave this to be addressed by someone who will need to implement power control ?

IMO if you've detected you're running in DP mode you should add the pm_runtime_get_sync() in probe to keep it powered all the time and that seems the simplest. Technically I guess that's not really required since you're polling and you could power off between polls, but then you'd have to re-init a bunch of your state each time you polled too. If you ever transitioned to using an IRQ for HPD then you'd have to keep it always powered anyway.

Hrm ... that's precisely what I've done. It's not IRQ based HPD...

So would you like to see something like this during ti_sn_bridge_probe()?

/* The device must remain powered up for HPD to be supported. */ if (!pdata->no_hpd) pm_runtime_get_sync(pdata->dev);

-- Regards

Kieran

-Doug

Hi Doug,

Quoting Doug Anderson (2022-02-23 18:25:13)

Hi,

On Wed, Feb 23, 2022 at 9:43 AM Kieran Bingham kieran.bingham@ideasonboard.com wrote:

...

Hi All,

I'm working to respin the remainder of these patches, now that I have IRQ based HPD working on the SN65DSI86, and the (non-eDP) mode is used for Renesas R-Car boards.

Quoting Doug Anderson (2021-06-24 00:51:12)

...

Hi,

On Wed, Jun 23, 2021 at 4:26 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

...

...

...

@@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,

    pdata->bridge.funcs = &ti_sn_bridge_funcs;
    pdata->bridge.of_node = client->dev.of_node;

•   pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
  

•   pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT)
  

Checking for "no_hpd" here is not the right test IIUC. You want to check for eDP vs. DP (AKA whether a panel is downstream of you or a connector). Specifically if downstream of you is a panel then (I believe) HPD won't assert until you turn on the panel and you won't turn on the panel (which happens in pre_enable, right?) until HPD fires, so you've got a chicken-and-egg problem. If downstream of you is a connector, though, then by definition HPD has to just work without pre_enable running so then you're OK.

Agreed. It's even more true now that your rework has landed, as in the eDP case EDID is handled by the panel driver. I'll rework this.

Should I also condition setting HPD_DISABLE to the presence of a panel then ? I could drop of_property_read_bool() and set

    pdata->no_hpd = !!panel;

...

I guess then you'd need to figure out what to do if someone wants to use "HPD" on eDP. Do you need to put a polling loop in pre_enable then? Or you could just punt not support this case until someone needs it.

I think I'll stop short of saving the world this time, yes :-) We'll see what to do when this case arises.

How about as a compromise you still call of_property_read_bool() but add some type of warning in the logs if someone didn't set "no-hpd" but they have a panel?

Would a mix of the two work well?

What about:

    pdata->no_hpd = of_property_read_bool(np, "no-hpd");
    if (panel && !pdata->no_hpd) {
            DRM_ERROR("Panels will not function with HPD. Enforcing no-hpd\n");
            pdata->no_hpd = true;
    }

That leaves it still optional to DP connectors, but enforced on eDP?

Yeah, that's fine with me. Nits would be to use "warn" instead of "error" since this isn't fatal and use the non-SHOUTING versions of the prints since the SHOUTING versions are deprecated.

Could you clarify this please? The whole driver uses DRM_ERROR style. Is there a new debug macro somewhere?

...

...

...

...

...

•                     | DRM_BRIDGE_OP_EDID;
  

IMO somewhere in here if HPD is being used like this you should throw in a call to pm_runtime_get_sync(). I guess in your solution the regulators (for the bridge, not the panel) and enable pin are just left on all the time,

Correct, on my development board the SN65DSI86 is on all the time, I can't control that.

...

but plausibly someone might want to build a system to use HPD and also have the enable pin and/or regulators controlled by this driver, right?

True. DRM doesn't make this very easy, as, as far as I can tell, there's no standard infrastructure for userspace to register an interest in HPD that could be notified to bridges. I think it should be fixable, but it's out of scope for this series :-) Should I still add a pm_runtime_get_sync() at probe time, or leave this to be addressed by someone who will need to implement power control ?

IMO if you've detected you're running in DP mode you should add the pm_runtime_get_sync() in probe to keep it powered all the time and that seems the simplest. Technically I guess that's not really required since you're polling and you could power off between polls, but then you'd have to re-init a bunch of your state each time you polled too. If you ever transitioned to using an IRQ for HPD then you'd have to keep it always powered anyway.

Hrm ... that's precisely what I've done. It's not IRQ based HPD...

So would you like to see something like this during ti_sn_bridge_probe()?

    /* The device must remain powered up for HPD to be supported. */
    if (!pdata->no_hpd)
            pm_runtime_get_sync(pdata->dev);

Yeah, seems reasonable. Probably you'd want to add a devm action to put it too?

Ok, looking at this now - then I should be able to get these updated patches out.

-- Thanks.

Kieran

-Doug