Quoting Doug Anderson (2022-03-07 23:22:17)
Hi,
On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham kieran.bingham+renesas@ideasonboard.com wrote:
@@ -1264,6 +1321,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev, return PTR_ERR(pdata->next_bridge); }
pdata->no_hpd = of_property_read_bool(np, "no-hpd");if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&!pdata->no_hpd) {dev_warn(pdata->dev,"HPD support requires a DisplayPort connector\n");Maybe "HPD support only implemented for full DP connectors".
Plausibly someone could come up with a reason to hook HPD up for eDP one day, but so far we haven't seen it.
Ok, updated.
@@ -1272,7 +1337,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
pdata->bridge.funcs = &ti_sn_bridge_funcs; pdata->bridge.of_node = np;
pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD)| DRM_BRIDGE_OP_EDID;Seems like "OP_HPD" ought to be dependent on whether the IRQ was provided, right? AKA you could have "detect" because HPD is plumbed through to the bridge but not "HPD" if the IRQ from the bridge isn't hooked up to the main processor...
Yes, I think that's right. If there's no IRQ, then OP_HPD shouldn't be set, and it will fall back to polling.
I'll fix that up.
@@ -1840,6 +1906,34 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata) pdata->supplies); }
+static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg) +{
struct ti_sn65dsi86 *pdata = arg;int ret;int hpd;`hpd` should be an `unsigned int` to match the prototype of regmap-read.
Agreed, and updated.
@@ -1881,6 +1975,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return dev_err_probe(dev, PTR_ERR(pdata->refclk), "failed to get reference clock\n");
if (client->irq > 0) {ret = devm_request_threaded_irq(dev, client->irq, NULL,ti_sn65dsi86_irq_handler,IRQF_ONESHOT, "sn65dsi86-irq",pdata);if (ret)return dev_err_probe(dev, ret,"Failed to register DP interrupt\n");}Is this the right place to request the IRQ, or should it be in ti_sn_bridge_probe(). As soon as you request it the interrupt can go off, but you're relying on a bunch of bridge stuff to have been initted, right?
Indeed, it will be relying upon the bridge to have been set up.
You're right I believe, ti_sn_bridge_probe() sounds reasonable. And only after that should we enable the interrupts.
Fixing ... (But getting stuck/blocked by the connector changes, so .. I'll keep plowing through).
@@ -1888,6 +1992,9 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, pm_runtime_set_autosuspend_delay(pdata->dev, 500); pm_runtime_use_autosuspend(pdata->dev);
/* Enable interrupt handling */regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);Shouldn't we only enable interrupt handling if client->irq > 0? AKA combine this with the "if" statement?
Agreed.
-Doug