Re: [PATCH V3] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

[John Ripple <john.ripple@keysight.com>]

Hi, 

>...and you don't need to check for "dev" being NULL because there's no
>way "hpd_enabled" could be true with "dev" being NULL. At least this
>is my assumption that the core DRM framework won't detach a bridge
>while HPD is enabled. If nothing else, I guess you could call
>ti_sn_bridge_hpd_disable() from ti_sn_bridge_detach()

I don't think ti_sn_bridge_hpd_disable() needs to be in 
ti_sn_brdige_detach(). The DRM framework should run the disable for hpd
before detaching the device. I haven't seen any issues with it so far.

>> @@ -1971,6 +2075,28 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>>         if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
>>                 return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device >id\n");
>>
>> +       if (client->irq) {
>> +               ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
>> +                                               ti_sn_bridge_interrupt,
>> +                                               IRQF_TRIGGER_RISING |
>> +                                               IRQF_TRIGGER_FALLING |
>> +                                               IRQF_ONESHOT,
>> +                                               "ti_sn65dsi86", pdata);
>> +
>> +               if (ret) {
>> +                       return dev_err_probe(dev, ret,
>> +                                            "failed to request interrupt\n");
>> +               }
>> +
>> +               /*
>> +                * Cleaning status register at probe is needed because if the >irq is
>> +                * already high, the rising/falling condition will never occur
>> +                */
>> +               ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, 0xFF);
>> +               if (ret)
>> +                       pr_warn("Failed to clear IRQ initial state: %d\n", >ret);
>
>Actually, wait. Why do you want "rising" and "falling". Isn't this a
>level-triggered interrupt? Then you also don't need this bogus clear
>of interrupts here...

I changed it out for a high level interrupt and it looks fine. The IRQ
line off the check also seems to only send one pulse for about 1.09 ms
when the IRQ is toggled, so I think its doing a level interrupt since 
1 KHz is way slower than the refclk. For 0xE0 the documentation also 
says "the IRQ output is driven high to communicate IRQ events" so I
think you're correct.

>...and also, I seem to recall it's usually better to not specify a
>type here and rely on the type in the device tree. I seem to remember
>there being some weird corner cases (maybe around remove / reprobe or
>maybe about deferred probes?) if an interrupt type is specified in
>both code and device tree and those types don't match...

I couldn't find anything about this and all the other drivers in 
drivers/gpu-drm/bridge that use the devm_request_threaded_irq just 
directly set the irq type. I couldn't find any that read in the device 
tree for it. The display-connector.c general driver also seems to just 
set the type directly. Do you have an example where this is used?

The tisn65dsi86 chip also shouldn't be changing how it does its 
interrupts, so having the hardcoded high interrupt in the driver seems
like it would be fine.