[PATCHv3 22/23] drm/bridge: tc358767: add IRQ and HPD support

[Tomi Valkeinen]

On 21/05/2019 10:07, Andrzej Hajda wrote:

>> @@ -1214,19 +1234,43 @@ 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;
> unused var

Needed for tc_write/read... =( Cleaning these up will be the next step.

>> +
>> +	if (tc->hpd_pin < 0) {
>> +		if (!tc->panel)
>> +			return connector_status_unknown;
>> +
>> +		conn = true;
>> +	} else {
>> +		tc_read(GPIOI, &val);
>> +
>> +		conn = val & BIT(tc->hpd_pin);
>> +	}
>> +
>> +	if (force && conn)
>> +		tc_get_display_props(tc);
> 
> 
> Why do you call tc_get_display_props here? It is called already in .enable.
> 
> If you need it for get_modes you can call it there. Here it looks unrelated.

Yes, it's needed for get_modes. Or more specifically, for tc_mode_valid. I agree it 
doesn't quite feel right, but I wouldn't say it's unrelated, or that this is a wrong place.

Afaics, we need tc_get_display_props in bridge_enable, for the case where we don't have 
hpd. We could call tc_get_display_props in get_modes, but I don't know if we always get a 
get_modes call. Or maybe we get multiple get_modes calls, and we do unnecessary 
tc_get_display_props calls.

Now that I wrote the above, it makes me wonder whether the get_modes works in the current 
patches if we don't have hpd...

We could cache tc_get_display_props results, too, but I'm not sure when to clear the 
cache, especially if we don't have hpd.

DisplayPort spec talks about doing the display-props reading and EDID reading when 
handling HPD.

I think it would be best to change the code so that we read display props and EDID in HPD, 
but so that we also can read them later (when needed, probably bridge enable and 
get_modes) if we haven't done the reads already. I've had this in mind since I started the 
series, but as it didn't feel like a simple change, I left it for later.

> Removing the call from here should also simplify function flow:
> 
>      if (tc->hpd_pin < 0)
> 
>          return tc->panel ? connector_status_connected :
> connector_status_disconnected;
> 
>      tc_read(GPIOI, &val);
> 
>      return (val & BIT(tc->hpd_pin))? ? connector_status_connected :
> connector_status_disconnected;
> 
> 
>> +
>> +	if (conn)
>> +		return connector_status_connected;
>> +	else
>> +		return connector_status_disconnected;
>> +
>> +err:
> 
> 
> unused label/code?

Needed for tc_write/read too.

>> @@ -1249,6 +1293,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_pin >= 0) {
>> +		if (tc->have_irq)
>> +			tc->connector.polled = DRM_CONNECTOR_POLL_HPD;
>> +		else
>> +			tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
>> +					       DRM_CONNECTOR_POLL_DISCONNECT;
> 
> 
> Bikeshedding: wouldn't be more clear to use ?:  operator?

Depends on the reader, I guess. I like ?: when the parameters are relatively simple (say, 
a single variable). Here it's a bit so-and-so with the second case's bitwise-or.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 
0615521-4. Kotipaikka/Domicile: Helsinki