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

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

Hi,

>> +static int ti_sn65dsi86_read(struct ti_sn65dsi86 *pdata, unsigned int reg,
>> +                            unsigned int *val)
>
>This is reading a byte, right? So "val" should be an "u8 *". Yeah,
>that means you need a local variable to adjust for the generic regmap
>call, but it makes a cleaner and more obvious API to the users in this
>file.

The regmap_read function takes in an "unsigned int *" as the "val"
parameter and I'm using it to return u32 values (which could probably
be u8 instead). Would it be better to leave this as the more generic 
int type or change it to u8 so its more specific to this driver?
If this function gets used elsewhere in this file at some point, I'm 
not sure everything that could be read would be single bytes.

>> @@ -1219,12 +1246,28 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
>>          */
>>
>>         pm_runtime_get_sync(pdata->dev);
>> +
>> +       /* Enable HPD and PLL events. */
>> +       regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
>> +                    PLL_UNLOCK_EN |
>> +                    HPD_REPLUG_EN |
>> +                    HPD_REMOVAL_EN |
>> +                    HPD_INSERTION_EN |
>> +                    IRQ_HPD_EN);
>
>* Shouldn't this be `regmap_update_bits()` to just update the bits
>related to HPD?
>
>* why enable "PLL_UNLOCK_EN" when you don't handle it?
>
>* I also don't think your IRQ handler handles "replug" and "irq_hpd",
>right? So you shouldn't enable those either?

The IRQ_HPD_EN documentation said:
"When IRQ_EN and IRQ_HPD_EN is enabled, the DSIx6 will assert the 
IRQ whenever the eDP generates a IRQ_HPD event. An IRQ_HPD event 
is defined as a change from INSERTION state to the IRQ_HPD state."

I thought that meant the IRQ_HPD_EN needed to be enabled to get any irqs,
but when I tried removing the IRQ_HPD_EN and it doesn't seem to change
anything, so I'm not sure what the documentation is trying to say.

>> @@ -1309,6 +1352,32 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
>>         return 0;
>>  }
>>
>> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
>> +{
>> +       struct ti_sn65dsi86 *pdata = private;
>> +       struct drm_device *dev = pdata->bridge.dev;
>
>I'm unsure if accessing "dev" here without any sort of locking is
>safe... It feels like, in theory, "detach" could be called and race
>with the IRQ handler? Maybe you need a spinlock to be sure?

I tested a spinlock added to the ti-sn65dsi86 structure that gets used
in the ti_sn_bridge_detach and ti_sn_bridge_interrupt functions and it
seems to work. Is there another spinlock created somewhere that I could
use instead? Is using the spin lock in the interrupt and detach functions
the correct way to do it?