Re: [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification (v2)

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Tue, May 04, 2021 at 05:35:49PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 5/4/21 5:10 PM, Heikki Krogerus wrote:
> >> +/**
> >> + * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to connector
> >> + * @connector: connector to report the event on
> >> + * @data: data related to the event
> >> + *
> >> + * On some hardware a hotplug event notification may come from outside the display
> >> + * driver / device. An example of this is some USB Type-C setups where the hardware
> >> + * muxes the DisplayPort data and aux-lines but does not pass the altmode HPD
> >> + * status bit to the GPU's DP HPD pin.
> >> + *
> >> + * This function can be used to report these out-of-band events after obtaining
> >> + * a drm_connector reference through calling drm_connector_find_by_fwnode().
> >> + */
> >> +void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
> >> +				     struct drm_connector_oob_hotplug_event_data *data)
> >> +{
> >> +	struct drm_connector *connector;
> >> +
> >> +	connector = drm_connector_find_by_fwnode(connector_fwnode);
> >> +	if (IS_ERR(connector))
> >> +		return;
> >> +
> >> +	if (connector->funcs->oob_hotplug_event)
> >> +		connector->funcs->oob_hotplug_event(connector, data);
> >> +
> >> +	drm_connector_put(connector);
> >> +}
> >> +EXPORT_SYMBOL(drm_connector_oob_hotplug_event);
> > 
> > So it does looks like the "data" parameter is not needed at all:
> 
> Well Imre did indicate that having the number of lanes is useful, so
> for the next version I'll drop the orientation but I plan to keep
> the number of lanes if that is ok with you.
> 
> Not having passing along this info was one of the reasons why my
> previous attempt at this was nacked, so dropping it all together
> feels wrong.

If you need to pass any information to the function, then you need to
pass all the information that we have. Don't start with abstraction.
First create a dedicated API, and then, only if we really have another
user for it, we can add an abstraction layer that both users can use.
All cases are going to be different. We don't know how the abstraction
/ generalisation should look like before we have at least two real
users (ideally more than two, actually). Right now we can not even say
for sure that generalising the API is even possible.

I would not make a huge deal out of this unless I wasn't myself being
told by guys like Greg KH in the past to drop my attempts to
"generalize" things from the beginning when I only had a single user.
By doing so you'll not only force all ends, the source of the data
(the typec drivers in this case) as well as the consumer (i915), to be
always changed together, it will also confuse things. We are not
always going to be able to tell the lane count for example like we can
with USB Type-C, so i915 can't really rely on that information.

Right now we also don't know what exact details i915 (or what ever GPU
driver) needs. We can only say for sure that some details are going to
be needed. Trying to guess and cherry-pick the details now does not
makes sense because of that reason too.

So just make this API USB Type-C DP Alt Mode specific API first, and
supply it everything we have.


thanks,

-- 
heikki