[PATCHv2,21/22] drm/bridge: tc358767: add IRQ and HPD support

Message ID 20190326103146.24795-22-tomi.valkeinen@ti.com
State Superseded
Headers show
Series
  • drm/bridge: tc358767: DP support
Related show

Commit Message

Tomi Valkeinen March 26, 2019, 10:31 a.m.
Add support for interrupt and hotplug handling. Both are optional.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 157 ++++++++++++++++++++++++++----
 1 file changed, 139 insertions(+), 18 deletions(-)

Comments

Andrey Smirnov April 2, 2019, 2:16 a.m. | #1
On Tue, Mar 26, 2019 at 3:33 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> Add support for interrupt and hotplug handling. Both are optional.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 157 ++++++++++++++++++++++++++----
>  1 file changed, 139 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 8606de29c9b2..6978129428a8 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -71,6 +71,7 @@
>
>  /* System */
>  #define TC_IDREG               0x0500
> +#define SYSSTAT                        0x0508
>  #define SYSCTRL                        0x0510
>  #define DP0_AUDSRC_NO_INPUT            (0 << 3)
>  #define DP0_AUDSRC_I2S_RX              (1 << 3)
> @@ -79,9 +80,16 @@
>  #define DP0_VIDSRC_DPI_RX              (2 << 0)
>  #define DP0_VIDSRC_COLOR_BAR           (3 << 0)
>  #define GPIOM                  0x0540
> +#define GPIOC                  0x0544
> +#define GPIOO                  0x0548
>  #define GPIOI                  0x054c
>  #define INTCTL_G               0x0560
>  #define INTSTS_G               0x0564
> +
> +#define INT_SYSERR             BIT(16)
> +#define INT_GPIO_H(x)          (1 << (x == 0 ? 2 : 10))
> +#define INT_GPIO_LC(x)         (1 << (x == 0 ? 3 : 11))
> +
>  #define INT_GP0_LCNT           0x0584
>  #define INT_GP1_LCNT           0x0588
>
> @@ -219,6 +227,12 @@ struct tc_data {
>         struct gpio_desc        *sd_gpio;
>         struct gpio_desc        *reset_gpio;
>         struct clk              *refclk;
> +
> +       /* do we have IRQ */
> +       bool                    have_irq;
> +
> +       /* HPD pin number (0 or 1) or -ENODEV */
> +       int                     hpd_num;
>  };
>
>  static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
> @@ -1095,6 +1109,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>         struct tc_data *tc = bridge_to_tc(bridge);
>         int ret;
>
> +       ret = tc_get_display_props(tc);
> +       if (ret < 0) {
> +               dev_err(tc->dev, "failed to read display props: %d\n", ret);
> +               return;
> +       }
> +
>         ret = tc_main_link_enable(tc);
>         if (ret < 0) {
>                 dev_err(tc->dev, "main link enable error: %d\n", ret);
> @@ -1200,19 +1220,42 @@ static int tc_connector_get_modes(struct drm_connector *connector)
>         return count;
>  }
>
> -static void tc_connector_set_polling(struct tc_data *tc,
> -                                    struct drm_connector *connector)
> -{
> -       /* TODO: add support for HPD */
> -       connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> -                           DRM_CONNECTOR_POLL_DISCONNECT;
> -}
> -
>  static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
>         .get_modes = tc_connector_get_modes,
>  };
>
> +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force)
> +{
> +       struct tc_data *tc = connector_to_tc(connector);
> +       bool conn;
> +       u32 val;
> +       int ret;
> +
> +       if (tc->hpd_num < 0) {
> +               if (tc->panel)
> +                       return connector_status_connected;
> +               else
> +                       return connector_status_unknown;
> +       }
> +

The early return above causes tc_get_display_props() to never be
called for eDP case, which in turn result in tc_mode_valid() returning
MODE_BAD for every mode it is given since it depends on tc->link.base
being initialized properly. I had to change this code to:

if (tc->hpd_num < 0) {
    if (!tc->panel)
        return connector_status_unknown;

    conn = true;
} else {
    tc_read(GPIOI, &val);

    conn = val & BIT(tc->hpd_num);
}

to fix the problem.

Thanks,
Andrey Smirnov
Tomi Valkeinen April 3, 2019, 11:34 a.m. | #2
On 02/04/2019 05:16, Andrey Smirnov wrote:

> The early return above causes tc_get_display_props() to never be
> called for eDP case, which in turn result in tc_mode_valid() returning
> MODE_BAD for every mode it is given since it depends on tc->link.base
> being initialized properly. I had to change this code to:
> 
> if (tc->hpd_num < 0) {
>     if (!tc->panel)
>         return connector_status_unknown;
> 
>     conn = true;
> } else {
>     tc_read(GPIOI, &val);
> 
>     conn = val & BIT(tc->hpd_num);
> }
> 
> to fix the problem.

Ah, right. There's tc_get_display_props() in tc_bridge_enable() but
that's of course too late (and maybe even not needed at all).

What you suggest here looks fine to me, so I'll change my patch
accordingly. Thanks!

 Tomi
Tomi Valkeinen April 12, 2019, 8:02 a.m. | #3
Hi Andrey,

On 03/04/2019 14:34, Tomi Valkeinen wrote:
> On 02/04/2019 05:16, Andrey Smirnov wrote:
> 
>> The early return above causes tc_get_display_props() to never be
>> called for eDP case, which in turn result in tc_mode_valid() returning
>> MODE_BAD for every mode it is given since it depends on tc->link.base
>> being initialized properly. I had to change this code to:
>>
>> if (tc->hpd_num < 0) {
>>     if (!tc->panel)
>>         return connector_status_unknown;
>>
>>     conn = true;
>> } else {
>>     tc_read(GPIOI, &val);
>>
>>     conn = val & BIT(tc->hpd_num);
>> }
>>
>> to fix the problem.
> 
> Ah, right. There's tc_get_display_props() in tc_bridge_enable() but
> that's of course too late (and maybe even not needed at all).
> 
> What you suggest here looks fine to me, so I'll change my patch
> accordingly. Thanks!

With the change above, does the series work with your setup? Can I add
your tested-by? I'll send v3 series soon with the DT change suggested by
Rob and the above fix.

Tomi
Andrey Smirnov April 12, 2019, 6:17 p.m. | #4
On Fri, Apr 12, 2019 at 1:02 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> Hi Andrey,
>
> On 03/04/2019 14:34, Tomi Valkeinen wrote:
> > On 02/04/2019 05:16, Andrey Smirnov wrote:
> >
> >> The early return above causes tc_get_display_props() to never be
> >> called for eDP case, which in turn result in tc_mode_valid() returning
> >> MODE_BAD for every mode it is given since it depends on tc->link.base
> >> being initialized properly. I had to change this code to:
> >>
> >> if (tc->hpd_num < 0) {
> >>     if (!tc->panel)
> >>         return connector_status_unknown;
> >>
> >>     conn = true;
> >> } else {
> >>     tc_read(GPIOI, &val);
> >>
> >>     conn = val & BIT(tc->hpd_num);
> >> }
> >>
> >> to fix the problem.
> >
> > Ah, right. There's tc_get_display_props() in tc_bridge_enable() but
> > that's of course too late (and maybe even not needed at all).
> >
> > What you suggest here looks fine to me, so I'll change my patch
> > accordingly. Thanks!
>
> With the change above, does the series work with your setup? Can I add
> your tested-by? I'll send v3 series soon with the DT change suggested by
> Rob and the above fix.
>

Yeah, other than that, the series seemed to work as expected on my
setup. You can add

Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>

to v3.

Thanks,
Andrey Smirnov
Andrzej Hajda April 15, 2019, 10:42 a.m. | #5
On 26.03.2019 11:31, Tomi Valkeinen wrote:
> Add support for interrupt and hotplug handling. Both are optional.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 157 ++++++++++++++++++++++++++----
>  1 file changed, 139 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 8606de29c9b2..6978129428a8 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -71,6 +71,7 @@
>  
>  /* System */
>  #define TC_IDREG		0x0500
> +#define SYSSTAT			0x0508
>  #define SYSCTRL			0x0510
>  #define DP0_AUDSRC_NO_INPUT		(0 << 3)
>  #define DP0_AUDSRC_I2S_RX		(1 << 3)
> @@ -79,9 +80,16 @@
>  #define DP0_VIDSRC_DPI_RX		(2 << 0)
>  #define DP0_VIDSRC_COLOR_BAR		(3 << 0)
>  #define GPIOM			0x0540
> +#define GPIOC			0x0544
> +#define GPIOO			0x0548
>  #define GPIOI			0x054c
>  #define INTCTL_G		0x0560
>  #define INTSTS_G		0x0564
> +
> +#define INT_SYSERR		BIT(16)
> +#define INT_GPIO_H(x)		(1 << (x == 0 ? 2 : 10))
> +#define INT_GPIO_LC(x)		(1 << (x == 0 ? 3 : 11))
> +
>  #define INT_GP0_LCNT		0x0584
>  #define INT_GP1_LCNT		0x0588
>  
> @@ -219,6 +227,12 @@ struct tc_data {
>  	struct gpio_desc	*sd_gpio;
>  	struct gpio_desc	*reset_gpio;
>  	struct clk		*refclk;
> +
> +	/* do we have IRQ */
> +	bool			have_irq;
> +
> +	/* HPD pin number (0 or 1) or -ENODEV */
> +	int			hpd_num;
>  };
>  
>  static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
> @@ -1095,6 +1109,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	int ret;
>  
> +	ret = tc_get_display_props(tc);
> +	if (ret < 0) {
> +		dev_err(tc->dev, "failed to read display props: %d\n", ret);
> +		return;
> +	}
> +
>  	ret = tc_main_link_enable(tc);
>  	if (ret < 0) {
>  		dev_err(tc->dev, "main link enable error: %d\n", ret);
> @@ -1200,19 +1220,42 @@ static int tc_connector_get_modes(struct drm_connector *connector)
>  	return count;
>  }
>  
> -static void tc_connector_set_polling(struct tc_data *tc,
> -				     struct drm_connector *connector)
> -{
> -	/* TODO: add support for HPD */
> -	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> -			    DRM_CONNECTOR_POLL_DISCONNECT;
> -}
> -
>  static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
>  	.get_modes = tc_connector_get_modes,
>  };
>  
> +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	struct tc_data *tc = connector_to_tc(connector);
> +	bool conn;
> +	u32 val;
> +	int ret;
> +
> +	if (tc->hpd_num < 0) {
> +		if (tc->panel)
> +			return connector_status_connected;
> +		else
> +			return connector_status_unknown;


Ok we have here 4 combinations:

1. noHPD + eDP.

2. noHPD + DP.

3. HPD + eDP.

4. HPD + DP.


Which ones do you want to support, disallow. It is not clear to me.

Moreover what connector_status_unknown should mean here for users?


> +	}
> +
> +	tc_read(GPIOI, &val);
> +
> +	conn = val & BIT(tc->hpd_num);
> +
> +	if (force && conn)
> +		tc_get_display_props(tc);
> +
> +	if (conn)
> +		return connector_status_connected;
> +	else
> +		return connector_status_disconnected;
> +
> +err:
> +	return connector_status_unknown;
> +}
> +
>  static const struct drm_connector_funcs tc_connector_funcs = {
> +	.detect = tc_connector_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.destroy = drm_connector_cleanup,
>  	.reset = drm_atomic_helper_connector_reset,
> @@ -1227,7 +1270,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
>  	struct drm_device *drm = bridge->dev;
>  	int ret;
>  
> -	/* Create eDP connector */
> +	/* Create DP/eDP connector */
>  	drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs);
>  	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs,
>  				 tc->panel ? DRM_MODE_CONNECTOR_eDP :
> @@ -1235,6 +1278,15 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
>  	if (ret)
>  		return ret;
>  
> +	/* Don't poll if don't have HPD connected */
> +	if (tc->hpd_num >= 0) {
> +		if (tc->have_irq)
> +			tc->connector.polled = DRM_CONNECTOR_POLL_HPD;
> +		else
> +			tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
> +					       DRM_CONNECTOR_POLL_DISCONNECT;
> +	}
> +
>  	if (tc->panel)
>  		drm_panel_attach(tc->panel, &tc->connector);
>  
> @@ -1301,6 +1353,43 @@ static const struct regmap_config tc_regmap_config = {
>  	.val_format_endian = REGMAP_ENDIAN_LITTLE,
>  };
>  
> +static irqreturn_t tc_irq_handler(int irq, void *arg)
> +{
> +	struct tc_data *tc = arg;
> +	u32 val;
> +	int r;
> +
> +	r = regmap_read(tc->regmap, INTSTS_G, &val);
> +	if (r)
> +		return IRQ_NONE;
> +
> +	if (!val)
> +		return IRQ_NONE;
> +
> +	if (val & INT_SYSERR) {
> +		u32 stat = 0;
> +
> +		regmap_read(tc->regmap, SYSSTAT, &stat);
> +
> +		dev_err(tc->dev, "syserr %x\n", stat);
> +	}
> +
> +	if (tc->hpd_num >= 0 && tc->bridge.dev) {
> +		bool h = val & INT_GPIO_H(tc->hpd_num);
> +		bool lc = val & INT_GPIO_LC(tc->hpd_num);
> +
> +		dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num,
> +			h ? "H" : "", lc ? "LC" : "");


What does "h" and "lc" mean?


Regards

Andrzej


> +
> +		if (h || lc)
> +			drm_kms_helper_hotplug_event(tc->bridge.dev);
> +	}
> +
> +	regmap_write(tc->regmap, INTSTS_G, val);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
>  	struct device *dev = &client->dev;
> @@ -1352,6 +1441,31 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		return ret;
>  	}
>  
> +	ret = of_property_read_u32(dev->of_node, "hpd-num", &tc->hpd_num);
> +	if (ret) {
> +		tc->hpd_num = -ENODEV;
> +	} else {
> +		if (tc->hpd_num < 0 || tc->hpd_num > 1) {
> +			dev_err(dev, "failed to parse HPD number\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (client->irq > 0) {
> +		/* enable SysErr */
> +		regmap_write(tc->regmap, INTCTL_G, INT_SYSERR);
> +
> +		ret = devm_request_threaded_irq(dev, client->irq,
> +						NULL, tc_irq_handler, IRQF_ONESHOT,
> +						"tc358767-irq", tc);
> +		if (ret) {
> +			dev_err(dev, "failed to register dp interrupt\n");
> +			return ret;
> +		}
> +
> +		tc->have_irq = true;
> +	}
> +
>  	ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev);
>  	if (ret) {
>  		dev_err(tc->dev, "can not read device ID: %d\n", ret);
> @@ -1365,6 +1479,22 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  
>  	tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */
>  
> +	if (tc->hpd_num >= 0) {
> +		u32 lcnt_reg = tc->hpd_num == 0 ? INT_GP0_LCNT : INT_GP1_LCNT;
> +		u32 h_lc = INT_GPIO_H(tc->hpd_num) | INT_GPIO_LC(tc->hpd_num);
> +
> +		/* Set LCNT to 2ms */
> +		regmap_write(tc->regmap, lcnt_reg,
> +			     clk_get_rate(tc->refclk) * 2 / 1000);
> +		/* We need the "alternate" mode for HPD */
> +		regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_num));
> +
> +		if (tc->have_irq) {
> +			/* enable H & LC */
> +			regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc);
> +		}
> +	}
> +
>  	ret = tc_aux_link_setup(tc);
>  	if (ret)
>  		return ret;
> @@ -1377,12 +1507,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	if (ret)
>  		return ret;
>  
> -	ret = tc_get_display_props(tc);
> -	if (ret)
> -		goto err_unregister_aux;
> -
> -	tc_connector_set_polling(tc, &tc->connector);
> -
>  	tc->bridge.funcs = &tc_bridge_funcs;
>  	tc->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&tc->bridge);
> @@ -1390,9 +1514,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	i2c_set_clientdata(client, tc);
>  
>  	return 0;
> -err_unregister_aux:
> -	drm_dp_aux_unregister(&tc->aux);
> -	return ret;
>  }
>  
>  static int tc_remove(struct i2c_client *client)
Tomi Valkeinen April 15, 2019, 10:59 a.m. | #6
On 15/04/2019 13:42, Andrzej Hajda wrote:

> Ok we have here 4 combinations:
> 
> 1. noHPD + eDP.
> 
> 2. noHPD + DP.
> 
> 3. HPD + eDP.
> 
> 4. HPD + DP.
> 
> 
> Which ones do you want to support, disallow. It is not clear to me.

They all should work.

If there is HPD, we use it to return connected/disconnected.

If we don't have HPD:
- If there's a panel (i.e. eDP), we presume that it is always there, as
by definition it can't be disconnected.
- If there's no panel (i.e. DP), we can't know if the cable is connected
or not, so we return unknown. I think this could be improved by trying
to "ping" the monitor with an AUX transaction, but I didn't look at that.

> Moreover what connector_status_unknown should mean here for users?

The the driver does not know if there's a monitor or not.

>> +	if (tc->hpd_num >= 0 && tc->bridge.dev) {
>> +		bool h = val & INT_GPIO_H(tc->hpd_num);
>> +		bool lc = val & INT_GPIO_LC(tc->hpd_num);
>> +
>> +		dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num,
>> +			h ? "H" : "", lc ? "LC" : "");
> 
> 
> What does "h" and "lc" mean?

These are from the func spec. H means high, LC means low-counter. H is
triggered immediately when the HPD line goes high, LC is triggered when
the line has been low for the counter-specified time (the counter is
configured at probe time).

These could be used to implement a more elaborate DP HPD & interrupt
handling, but for the time being the driver just takes them as "HPD may
have changed".

 Tomi
Andrzej Hajda April 17, 2019, 7:32 a.m. | #7
On 15.04.2019 12:59, Tomi Valkeinen wrote:
> On 15/04/2019 13:42, Andrzej Hajda wrote:
>
>> Ok we have here 4 combinations:
>>
>> 1. noHPD + eDP.
>>
>> 2. noHPD + DP.
>>
>> 3. HPD + eDP.
>>
>> 4. HPD + DP.
>>
>>
>> Which ones do you want to support, disallow. It is not clear to me.
> They all should work.
>
> If there is HPD, we use it to return connected/disconnected.


OK, I though that eDP does not use HPD at all.


> If we don't have HPD:
> - If there's a panel (i.e. eDP), we presume that it is always there, as
> by definition it can't be disconnected.
> - If there's no panel (i.e. DP), we can't know if the cable is connected
> or not, so we return unknown. I think this could be improved by trying
> to "ping" the monitor with an AUX transaction, but I didn't look at that.
>
>> Moreover what connector_status_unknown should mean here for users?
> The the driver does not know if there's a monitor or not.

:)

More specifically, what user can do with such information.

OK, he can ensure monitor is connected and then force connected state.

But shall we expect existence of such configurations, I hoped we could
eliminate such combination (DP+noHPD) during probe.


>
>>> +	if (tc->hpd_num >= 0 && tc->bridge.dev) {
>>> +		bool h = val & INT_GPIO_H(tc->hpd_num);
>>> +		bool lc = val & INT_GPIO_LC(tc->hpd_num);
>>> +
>>> +		dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num,
>>> +			h ? "H" : "", lc ? "LC" : "");
>>
>> What does "h" and "lc" mean?
> These are from the func spec. H means high, LC means low-counter. H is
> triggered immediately when the HPD line goes high, LC is triggered when
> the line has been low for the counter-specified time (the counter is
> configured at probe time).


It would be good to add this info somewhere, it is hard to guess what lc
means.


Regards

Andrzej


>
> These could be used to implement a more elaborate DP HPD & interrupt
> handling, but for the time being the driver just takes them as "HPD may
> have changed".
>
>  Tomi
>
Tomi Valkeinen April 29, 2019, 9:27 a.m. | #8
Hi,

On 17/04/2019 10:32, Andrzej Hajda wrote:
> On 15.04.2019 12:59, Tomi Valkeinen wrote:
>> On 15/04/2019 13:42, Andrzej Hajda wrote:
>>
>>> Ok we have here 4 combinations:
>>>
>>> 1. noHPD + eDP.
>>>
>>> 2. noHPD + DP.
>>>
>>> 3. HPD + eDP.
>>>
>>> 4. HPD + DP.
>>>
>>>
>>> Which ones do you want to support, disallow. It is not clear to me.
>> They all should work.
>>
>> If there is HPD, we use it to return connected/disconnected.
> 
> 
> OK, I though that eDP does not use HPD at all.
> 
> 
>> If we don't have HPD:
>> - If there's a panel (i.e. eDP), we presume that it is always there, as
>> by definition it can't be disconnected.
>> - If there's no panel (i.e. DP), we can't know if the cable is connected
>> or not, so we return unknown. I think this could be improved by trying
>> to "ping" the monitor with an AUX transaction, but I didn't look at that.
>>
>>> Moreover what connector_status_unknown should mean here for users?
>> The the driver does not know if there's a monitor or not.
> 
> :)
> 
> More specifically, what user can do with such information.
> 
> OK, he can ensure monitor is connected and then force connected state.

Yes, something like that. I haven't really been testing that kind of
setups, but afaik, that's more about how DRM exposes and expects the
userspace to handle "unknown" connection status than what TC358767 does.

> But shall we expect existence of such configurations, I hoped we could
> eliminate such combination (DP+noHPD) during probe.

Eliminate how? Make HPD required for DP and fail if there's no HPD? I
guess that's an option.

Then again, the solutions the HW people come up with in the embedded
space never ceases to amaze me, so while I don't expect such
configurations, I would not be surprised to see one.

>>>> +	if (tc->hpd_num >= 0 && tc->bridge.dev) {
>>>> +		bool h = val & INT_GPIO_H(tc->hpd_num);
>>>> +		bool lc = val & INT_GPIO_LC(tc->hpd_num);
>>>> +
>>>> +		dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num,
>>>> +			h ? "H" : "", lc ? "LC" : "");
>>>
>>> What does "h" and "lc" mean?
>> These are from the func spec. H means high, LC means low-counter. H is
>> triggered immediately when the HPD line goes high, LC is triggered when
>> the line has been low for the counter-specified time (the counter is
>> configured at probe time).
> 
> 
> It would be good to add this info somewhere, it is hard to guess what lc
> means.

Ok.

 Tomi

Patch

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 8606de29c9b2..6978129428a8 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -71,6 +71,7 @@ 
 
 /* System */
 #define TC_IDREG		0x0500
+#define SYSSTAT			0x0508
 #define SYSCTRL			0x0510
 #define DP0_AUDSRC_NO_INPUT		(0 << 3)
 #define DP0_AUDSRC_I2S_RX		(1 << 3)
@@ -79,9 +80,16 @@ 
 #define DP0_VIDSRC_DPI_RX		(2 << 0)
 #define DP0_VIDSRC_COLOR_BAR		(3 << 0)
 #define GPIOM			0x0540
+#define GPIOC			0x0544
+#define GPIOO			0x0548
 #define GPIOI			0x054c
 #define INTCTL_G		0x0560
 #define INTSTS_G		0x0564
+
+#define INT_SYSERR		BIT(16)
+#define INT_GPIO_H(x)		(1 << (x == 0 ? 2 : 10))
+#define INT_GPIO_LC(x)		(1 << (x == 0 ? 3 : 11))
+
 #define INT_GP0_LCNT		0x0584
 #define INT_GP1_LCNT		0x0588
 
@@ -219,6 +227,12 @@  struct tc_data {
 	struct gpio_desc	*sd_gpio;
 	struct gpio_desc	*reset_gpio;
 	struct clk		*refclk;
+
+	/* do we have IRQ */
+	bool			have_irq;
+
+	/* HPD pin number (0 or 1) or -ENODEV */
+	int			hpd_num;
 };
 
 static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
@@ -1095,6 +1109,12 @@  static void tc_bridge_enable(struct drm_bridge *bridge)
 	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
 
+	ret = tc_get_display_props(tc);
+	if (ret < 0) {
+		dev_err(tc->dev, "failed to read display props: %d\n", ret);
+		return;
+	}
+
 	ret = tc_main_link_enable(tc);
 	if (ret < 0) {
 		dev_err(tc->dev, "main link enable error: %d\n", ret);
@@ -1200,19 +1220,42 @@  static int tc_connector_get_modes(struct drm_connector *connector)
 	return count;
 }
 
-static void tc_connector_set_polling(struct tc_data *tc,
-				     struct drm_connector *connector)
-{
-	/* TODO: add support for HPD */
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
-			    DRM_CONNECTOR_POLL_DISCONNECT;
-}
-
 static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
 	.get_modes = tc_connector_get_modes,
 };
 
+static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct tc_data *tc = connector_to_tc(connector);
+	bool conn;
+	u32 val;
+	int ret;
+
+	if (tc->hpd_num < 0) {
+		if (tc->panel)
+			return connector_status_connected;
+		else
+			return connector_status_unknown;
+	}
+
+	tc_read(GPIOI, &val);
+
+	conn = val & BIT(tc->hpd_num);
+
+	if (force && conn)
+		tc_get_display_props(tc);
+
+	if (conn)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
+
+err:
+	return connector_status_unknown;
+}
+
 static const struct drm_connector_funcs tc_connector_funcs = {
+	.detect = tc_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = drm_connector_cleanup,
 	.reset = drm_atomic_helper_connector_reset,
@@ -1227,7 +1270,7 @@  static int tc_bridge_attach(struct drm_bridge *bridge)
 	struct drm_device *drm = bridge->dev;
 	int ret;
 
-	/* Create eDP connector */
+	/* Create DP/eDP connector */
 	drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs);
 	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs,
 				 tc->panel ? DRM_MODE_CONNECTOR_eDP :
@@ -1235,6 +1278,15 @@  static int tc_bridge_attach(struct drm_bridge *bridge)
 	if (ret)
 		return ret;
 
+	/* Don't poll if don't have HPD connected */
+	if (tc->hpd_num >= 0) {
+		if (tc->have_irq)
+			tc->connector.polled = DRM_CONNECTOR_POLL_HPD;
+		else
+			tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
+					       DRM_CONNECTOR_POLL_DISCONNECT;
+	}
+
 	if (tc->panel)
 		drm_panel_attach(tc->panel, &tc->connector);
 
@@ -1301,6 +1353,43 @@  static const struct regmap_config tc_regmap_config = {
 	.val_format_endian = REGMAP_ENDIAN_LITTLE,
 };
 
+static irqreturn_t tc_irq_handler(int irq, void *arg)
+{
+	struct tc_data *tc = arg;
+	u32 val;
+	int r;
+
+	r = regmap_read(tc->regmap, INTSTS_G, &val);
+	if (r)
+		return IRQ_NONE;
+
+	if (!val)
+		return IRQ_NONE;
+
+	if (val & INT_SYSERR) {
+		u32 stat = 0;
+
+		regmap_read(tc->regmap, SYSSTAT, &stat);
+
+		dev_err(tc->dev, "syserr %x\n", stat);
+	}
+
+	if (tc->hpd_num >= 0 && tc->bridge.dev) {
+		bool h = val & INT_GPIO_H(tc->hpd_num);
+		bool lc = val & INT_GPIO_LC(tc->hpd_num);
+
+		dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num,
+			h ? "H" : "", lc ? "LC" : "");
+
+		if (h || lc)
+			drm_kms_helper_hotplug_event(tc->bridge.dev);
+	}
+
+	regmap_write(tc->regmap, INTSTS_G, val);
+
+	return IRQ_HANDLED;
+}
+
 static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
@@ -1352,6 +1441,31 @@  static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return ret;
 	}
 
+	ret = of_property_read_u32(dev->of_node, "hpd-num", &tc->hpd_num);
+	if (ret) {
+		tc->hpd_num = -ENODEV;
+	} else {
+		if (tc->hpd_num < 0 || tc->hpd_num > 1) {
+			dev_err(dev, "failed to parse HPD number\n");
+			return ret;
+		}
+	}
+
+	if (client->irq > 0) {
+		/* enable SysErr */
+		regmap_write(tc->regmap, INTCTL_G, INT_SYSERR);
+
+		ret = devm_request_threaded_irq(dev, client->irq,
+						NULL, tc_irq_handler, IRQF_ONESHOT,
+						"tc358767-irq", tc);
+		if (ret) {
+			dev_err(dev, "failed to register dp interrupt\n");
+			return ret;
+		}
+
+		tc->have_irq = true;
+	}
+
 	ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev);
 	if (ret) {
 		dev_err(tc->dev, "can not read device ID: %d\n", ret);
@@ -1365,6 +1479,22 @@  static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */
 
+	if (tc->hpd_num >= 0) {
+		u32 lcnt_reg = tc->hpd_num == 0 ? INT_GP0_LCNT : INT_GP1_LCNT;
+		u32 h_lc = INT_GPIO_H(tc->hpd_num) | INT_GPIO_LC(tc->hpd_num);
+
+		/* Set LCNT to 2ms */
+		regmap_write(tc->regmap, lcnt_reg,
+			     clk_get_rate(tc->refclk) * 2 / 1000);
+		/* We need the "alternate" mode for HPD */
+		regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_num));
+
+		if (tc->have_irq) {
+			/* enable H & LC */
+			regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc);
+		}
+	}
+
 	ret = tc_aux_link_setup(tc);
 	if (ret)
 		return ret;
@@ -1377,12 +1507,6 @@  static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (ret)
 		return ret;
 
-	ret = tc_get_display_props(tc);
-	if (ret)
-		goto err_unregister_aux;
-
-	tc_connector_set_polling(tc, &tc->connector);
-
 	tc->bridge.funcs = &tc_bridge_funcs;
 	tc->bridge.of_node = dev->of_node;
 	drm_bridge_add(&tc->bridge);
@@ -1390,9 +1514,6 @@  static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	i2c_set_clientdata(client, tc);
 
 	return 0;
-err_unregister_aux:
-	drm_dp_aux_unregister(&tc->aux);
-	return ret;
 }
 
 static int tc_remove(struct i2c_client *client)