linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Stephen Boyd <swboyd@chromium.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Imre Deak <imre.deak@intel.com>,
	Jani Nikula <jani.nikula@linux.intel.com>,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	Lyude <lyude@redhat.com>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	pmalani@chromium.org, robh@kernel.org
Cc: Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	dri-devel@lists.freedesktop.org, linux-usb@vger.kernel.org
Subject: Re: [PATCH 8/8] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events
Date: Thu, 16 Sep 2021 15:17:56 +0200	[thread overview]
Message-ID: <1b477c65-d5cd-2ece-1f60-f9a7762f07b9@redhat.com> (raw)
In-Reply-To: <CAE-0n53cRs3USijgp5nKy0KoykrNAOs90K-pMk9QBkT+qOgyHw@mail.gmail.com>

Hi,

On 9/16/21 5:20 AM, Stephen Boyd wrote:
> Quoting Hans de Goede (2021-08-17 14:52:01)
>> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
>> index aa669b9cf70e..c1d8c23baa39 100644
>> --- a/drivers/usb/typec/altmodes/displayport.c
>> +++ b/drivers/usb/typec/altmodes/displayport.c
>> @@ -125,6 +129,7 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 con)
>>  static int dp_altmode_status_update(struct dp_altmode *dp)
>>  {
>>         bool configured = !!DP_CONF_GET_PIN_ASSIGN(dp->data.conf);
>> +       bool hpd = !!(dp->data.status & DP_STATUS_HPD_STATE);
>>         u8 con = DP_STATUS_CONNECTION(dp->data.status);
>>         int ret = 0;
>>
>> @@ -137,6 +142,11 @@ static int dp_altmode_status_update(struct dp_altmode *dp)
>>                 ret = dp_altmode_configure(dp, con);
>>                 if (!ret)
>>                         dp->state = DP_STATE_CONFIGURE;
>> +       } else {
>> +               if (dp->hpd != hpd) {
>> +                       drm_connector_oob_hotplug_event(dp->connector_fwnode);
>> +                       dp->hpd = hpd;
>> +               }
>>         }
>>
>>         return ret;
>> @@ -512,6 +522,7 @@ static const struct attribute_group dp_altmode_group = {
>>  int dp_altmode_probe(struct typec_altmode *alt)
>>  {
>>         const struct typec_altmode *port = typec_altmode_get_partner(alt);
>> +       struct fwnode_handle *fwnode;
>>         struct dp_altmode *dp;
>>         int ret;
>>
>> @@ -540,6 +551,11 @@ int dp_altmode_probe(struct typec_altmode *alt)
>>         alt->desc = "DisplayPort";
>>         alt->ops = &dp_altmode_ops;
>>
>> +       fwnode = dev_fwnode(alt->dev.parent->parent); /* typec_port fwnode */
>> +       dp->connector_fwnode = fwnode_find_reference(fwnode, "displayport", 0);
> 
> I'm trying to figure out how to translate this over to DT bindings.

First of all let me say that it is great that people are looking
into using this functionality outside of the niche application
for which I wrote it.

> Is
> there a binding document for this fwnode reference? If not, can you
> please update
> Documentation/devicetree/bindings/connector/usb-connector.yaml with this
> property?

My use case is some Intel Cherry Trail based mini-laptops which use a
Intel Whiskey Cove PMIC with a FUSB302 Type-C controller + a
PI3USB30532 USB switch, specifically the GPD Win and GPD Pocket
(first generation of each).

These are ACPI/X86 devices so devicetree is not used there, the
connector_fwnode checked here is actually sw_node, this sw_node
reference gets setup here:

drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c

The setup/use of a sw_node reference for this was actually
designed by Heikki (in the Cc already), so he might be a better
person to answer your questions.

With that said, as to your question if I can document this in
usb-connector.yaml, no I cannot (sorry). Since this is a sw_node
setup by X86 code and not a normal devicetree fwnode reference,
atm this is purely a kernel internal API/binding really and the
DT-binding maintainers have repeatedly told me that I should NOT
submit DT-binding updates for these. Only once a real devicetree
user for this shows up will the accept DT-bindings patches for this.

The good news here though is, that if this turns out to be
non-ideal for the devicetree case we can still change things
as long as drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c
also gets updated so as to not break things.

> I think this means that the type-c node would have a 'displayport =
> <&some_phandle>' property in it that points to the display port hardware
> device that's pumping out the DisplayPort data?

It points to a fwnode belonging to the drm-connector for the DisplayPort
connector/pins to which the Type-C data-lines mux/switch is connected, see:

https://cgit.freedesktop.org/drm-misc/commit/?id=48c429c6d18db115c277b75000152d8fa4cd35d0
https://cgit.freedesktop.org/drm-misc/commit/?id=3d3f7c1e68691574c1d87cd0f9f2348323bc0199
https://cgit.freedesktop.org/drm-intel/commit/?id=a481d0e80eabbc3fed666103744aeaf47f63e708

So this get working with devicetree you would need something like this:

1. Have devicetree-node-s describing video-output connectors, similar
to how we have usb-connector nodes to describe usb-connectors

2. Have your drm/kms driver lookup these video-output-connector of_node-s
and assign them to drm_connector.fwnode, at least for the DP video-output
used.

3. Have the usb-connector node for your Type-C connector (the same of_node
as which has the source-pdos, sink-pdos, etc props) have a 
'displayport = <&some_phandle>' property pointing to the of_node for
the DP video-output used.

I hope this helps.

> 
>> +       if (IS_ERR(dp->connector_fwnode))
>> +               dp->connector_fwnode = NULL;
>> +
>>         typec_altmode_set_drvdata(alt, dp);
>>
>>         dp->state = DP_STATE_ENTER;
>> @@ -555,6 +571,13 @@ void dp_altmode_remove(struct typec_altmode *alt)
>>
>>         sysfs_remove_group(&alt->dev.kobj, &dp_altmode_group);
>>         cancel_work_sync(&dp->work);
>> +
>> +       if (dp->connector_fwnode) {
>> +               if (dp->hpd)
>> +                       drm_connector_oob_hotplug_event(dp->connector_fwnode);
> 
> I was hoping that we could make a type-c connector into a drm_bridge.
> I'm thinking that it would be a DP-to-panel bridge. Then a panel could
> be created as well on the end of the type-c connector and the bridge
> would report hpd whenever the type-c logic figures out the cable has
> been connected and hpd is asserted. The actual DisplayPort hardware
> that's encoding data would then find the bridge through the graph
> binding connected to the output node.
> 
> I'm not sure how MST is handled though. In that scenario maybe there's
> more than one panel?

Yeah, given that MST over DP over Type-C is very much possible my
first instinct is that this drm_bridge + bridge-to-panel idea is
not going to work very well. Also the output could be anything, it
could be a projector, or the DP input of a video-grabber, or ...
so modelling this as a panel feels wrong.

Regards,

Hans


  reply	other threads:[~2021-09-16 13:18 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-17 21:51 [PATCH 0/8] drm + usb-type-c: Add support for out-of-band hotplug notification (v4 resend) Hans de Goede
2021-08-17 21:51 ` [PATCH 1/8] drm/connector: Give connector sysfs devices there own device_type Hans de Goede
2021-08-18 21:35   ` Lyude Paul
2021-08-17 21:51 ` [PATCH 2/8] drm/connector: Add a fwnode pointer to drm_connector and register with ACPI (v2) Hans de Goede
2021-08-17 21:51 ` [PATCH 3/8] drm/connector: Add drm_connector_find_by_fwnode() function (v3) Hans de Goede
2021-08-17 21:51 ` [PATCH 4/8] drm/connector: Add support for out-of-band hotplug notification (v3) Hans de Goede
2021-08-17 21:51 ` [PATCH 5/8] drm/i915: Associate ACPI connector nodes with connector entries (v2) Hans de Goede
2021-08-17 21:51 ` [PATCH 6/8] drm/i915/dp: Add support for out-of-bound hotplug events Hans de Goede
2021-08-17 21:52 ` [PATCH 7/8] usb: typec: altmodes/displayport: Make dp_altmode_notify() more generic Hans de Goede
2021-08-17 21:52 ` [PATCH 8/8] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events Hans de Goede
2021-09-16  3:20   ` Stephen Boyd
2021-09-16 13:17     ` Hans de Goede [this message]
2021-09-24 23:29       ` Stephen Boyd
2021-08-18 22:03 ` [PATCH 0/8] drm + usb-type-c: Add support for out-of-band hotplug notification (v4 resend) Lyude Paul
  -- strict thread matches above, loose matches on Subject: below --
2021-06-04 19:48 [PATCH 0/8] drm + usb-type-c: Add support for out-of-band hotplug notification (v4) Hans de Goede
2021-06-04 19:48 ` [PATCH 8/8] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events Hans de Goede
2021-06-15  7:40   ` Greg Kroah-Hartman
2021-05-05 16:24 [PATCH 0/8] drm + usb-type-c: Add support for out-of-band hotplug notification (v3) Hans de Goede
2021-05-05 16:24 ` [PATCH 8/8] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events Hans de Goede
2021-05-11  6:56   ` Heikki Krogerus

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1b477c65-d5cd-2ece-1f60-f9a7762f07b9@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=airlied@linux.ie \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=imre.deak@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=jani.nikula@linux.intel.com \
    --cc=joonas.lahtinen@linux.intel.com \
    --cc=linux-usb@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=lyude@redhat.com \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mripard@kernel.org \
    --cc=pmalani@chromium.org \
    --cc=robh@kernel.org \
    --cc=rodrigo.vivi@intel.com \
    --cc=swboyd@chromium.org \
    --cc=tzimmermann@suse.de \
    --subject='Re: [PATCH 8/8] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).