| Message ID | 20191018195719.94634-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] usb: typec: tcpm: Remove tcpc_config configuration mechanism | expand |
On 10/18/19 12:57 PM, Hans de Goede wrote: > Add support for configuring display-port altmode through device-properties. > > We could try to add a generic mechanism for describing altmodes in > device-properties, but various altmodes will likely need altmode specific > configuration. E.g. the display-port altmode needs some way to describe > which set of DP pins on the GPU is connected to the USB Type-C connector. > > As such it is better to have a separate set of altmode specific properties > per altmode and this commit adds a property for basic display-port altmode > support. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > .../bindings/connector/usb-connector.txt | 3 ++ > drivers/usb/typec/tcpm/tcpm.c | 33 +++++++++++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt > index d357987181ee..7bae3cc9c76a 100644 > --- a/Documentation/devicetree/bindings/connector/usb-connector.txt > +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt > @@ -38,6 +38,9 @@ Optional properties for usb-c-connector: > or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC. > - data-role: should be one of "host", "device", "dual"(DRD) if typec > connector supports USB data. > +- displayport-vdo: The presenence of this property indicates that the From a DT perspective, I wonder if the vdo properties should be listed explicitly (capabilities, signaling, receptacle etc) or if it is ok to list a single value. Either case, I wonder if the VDO should be explained in more detail. > + usb-connector supports displayport-altmode (svid 0xff01), the value of > + this property is an u32 with the vdo value for the displayport-altmode, > The added property will require approval by a DT maintainer. > Required properties for usb-c-connector with power delivery support: > - source-pdos: An array of u32 with each entry providing supported power > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index c5fa18759f8e..2e3096657e96 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -28,6 +28,7 @@ > #include <linux/usb/role.h> > #include <linux/usb/tcpm.h> > #include <linux/usb/typec_altmode.h> > +#include <linux/usb/typec_dp.h> > #include <linux/workqueue.h> > > #define FOREACH_STATE(S) \ > @@ -281,6 +282,7 @@ struct tcpm_port { > unsigned int nr_snk_pdo; > u32 snk_vdo[VDO_MAX_OBJECTS]; > unsigned int nr_snk_vdo; > + u32 displayport_vdo; > > unsigned int operating_snk_mw; > bool update_sink_caps; > @@ -4433,6 +4435,9 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, > port->nr_snk_pdo)) > return -EINVAL; > > + fwnode_property_read_u32(fwnode, "displayport-vdo", > + &port->displayport_vdo); > + > if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0) > return -EINVAL; > port->operating_snk_mw = mw / 1000; > @@ -4667,6 +4672,28 @@ static int devm_tcpm_psy_register(struct tcpm_port *port) > return PTR_ERR_OR_ZERO(port->psy); > } > > +static int tcpm_register_port_altmodes(struct tcpm_port *port) > +{ > + struct typec_altmode_desc desc; > + struct typec_altmode *alt; > + int index = 0; > + > + if (port->displayport_vdo) { > + desc.svid = USB_TYPEC_DP_SID; > + desc.mode = USB_TYPEC_DP_MODE; > + desc.vdo = port->displayport_vdo; > + alt = typec_port_register_altmode(port->typec_port, &desc); > + if (IS_ERR(alt)) > + return PTR_ERR(alt); > + typec_altmode_set_drvdata(alt, port); > + alt->ops = &tcpm_altmode_ops; > + port->port_altmode[index] = alt; > + index++; > + } > + /* Future support for further altmodes goes here */ > + return 0; > +} > + > struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc) > { > struct tcpm_port *port; > @@ -4736,6 +4763,10 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc) > goto out_role_sw_put; > } > > + err = tcpm_register_port_altmodes(port); > + if (err) > + goto out_unregister_port; > + > mutex_lock(&port->lock); > tcpm_init(port); > mutex_unlock(&port->lock); > @@ -4743,6 +4774,8 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc) > tcpm_log(port, "%s: registered", dev_name(dev)); > return port; > > +out_unregister_port: > + typec_unregister_port(port->typec_port); > out_role_sw_put: > usb_role_switch_put(port->role_sw); > out_destroy_wq: >
Hi Hans, On Fri, Oct 18, 2019 at 09:57:18PM +0200, Hans de Goede wrote: > Add support for configuring display-port altmode through device-properties. > > We could try to add a generic mechanism for describing altmodes in > device-properties, but various altmodes will likely need altmode specific > configuration. E.g. the display-port altmode needs some way to describe > which set of DP pins on the GPU is connected to the USB Type-C connector. > > As such it is better to have a separate set of altmode specific properties > per altmode and this commit adds a property for basic display-port altmode > support. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > .../bindings/connector/usb-connector.txt | 3 ++ > drivers/usb/typec/tcpm/tcpm.c | 33 +++++++++++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt > index d357987181ee..7bae3cc9c76a 100644 > --- a/Documentation/devicetree/bindings/connector/usb-connector.txt > +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt > @@ -38,6 +38,9 @@ Optional properties for usb-c-connector: > or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC. > - data-role: should be one of "host", "device", "dual"(DRD) if typec > connector supports USB data. > +- displayport-vdo: The presenence of this property indicates that the > + usb-connector supports displayport-altmode (svid 0xff01), the value of > + this property is an u32 with the vdo value for the displayport-altmode, No, let's not take this approach. Every alternate mode a connector supports will need to have its own "sub-fwnode" under the connector fwnode. I thought we agreed this earlier? In any case, those sub-nodes will have default device properties named "svid" and "vdo". If the alternate mode still needs some other details, it can have other device properties that are specific to it, but note that displayport alt mode does not need anything extra. The "vdo" will already tells which pin configurations the connector supports and that is all that the driver needs to know. After we have the sub-nodes, it's not a big deal to walk through the child-nodes the port has during port registration and register the port alternate modes at the same time. That we can do in typec_register_port(), so we do not need to do it in every driver separately. thanks,
Hi, On 21-10-2019 08:55, Heikki Krogerus wrote: > Hi Hans, > > On Fri, Oct 18, 2019 at 09:57:18PM +0200, Hans de Goede wrote: >> Add support for configuring display-port altmode through device-properties. >> >> We could try to add a generic mechanism for describing altmodes in >> device-properties, but various altmodes will likely need altmode specific >> configuration. E.g. the display-port altmode needs some way to describe >> which set of DP pins on the GPU is connected to the USB Type-C connector. >> >> As such it is better to have a separate set of altmode specific properties >> per altmode and this commit adds a property for basic display-port altmode >> support. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> .../bindings/connector/usb-connector.txt | 3 ++ >> drivers/usb/typec/tcpm/tcpm.c | 33 +++++++++++++++++++ >> 2 files changed, 36 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt >> index d357987181ee..7bae3cc9c76a 100644 >> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt >> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt >> @@ -38,6 +38,9 @@ Optional properties for usb-c-connector: >> or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC. >> - data-role: should be one of "host", "device", "dual"(DRD) if typec >> connector supports USB data. >> +- displayport-vdo: The presenence of this property indicates that the >> + usb-connector supports displayport-altmode (svid 0xff01), the value of >> + this property is an u32 with the vdo value for the displayport-altmode, > > No, let's not take this approach. > > Every alternate mode a connector supports will need to have its own > "sub-fwnode" under the connector fwnode. I thought we agreed this > earlier? > > In any case, those sub-nodes will have default device properties named > "svid" and "vdo". If the alternate mode still needs some other > details, it can have other device properties that are specific to it, > but note that displayport alt mode does not need anything extra. The > "vdo" will already tells which pin configurations the connector > supports and that is all that the driver needs to know. > > After we have the sub-nodes, it's not a big deal to walk through the > child-nodes the port has during port registration and register the > port alternate modes at the same time. That we can do in > typec_register_port(), so we do not need to do it in every driver > separately. Yes we did agree to do the sub-fwnode thingie. But since this is a hobby project I do not have a whole lot of time to work on this. So when I started working on this, I though that the approach from this patch-set would be more KISS and IMHO it works out well. But the sub-fwnode approach is probably more future proof. Anyways as said I do not have a whole lot of time to work on this, if you want to go the sub-fwnode route, perhaps you can do a PoC patch series for this? I would be happy to test this and if necessary work it into something which works for the DP case. Doing the port alternate modes registration from typec_register_port() does sound like a good idea. The first patch in this series is independent of this and IMHO it would be good to get that upstream regardless of this alt-mode registration stuff, so I will resend that as a standalone patch. Regards, Hans
On Thu, Nov 14, 2019 at 12:16:09PM +0100, Hans de Goede wrote: > > Hi, > > On 21-10-2019 08:55, Heikki Krogerus wrote: > > Hi Hans, > > > > On Fri, Oct 18, 2019 at 09:57:18PM +0200, Hans de Goede wrote: > > > Add support for configuring display-port altmode through device-properties. > > > > > > We could try to add a generic mechanism for describing altmodes in > > > device-properties, but various altmodes will likely need altmode specific > > > configuration. E.g. the display-port altmode needs some way to describe > > > which set of DP pins on the GPU is connected to the USB Type-C connector. > > > > > > As such it is better to have a separate set of altmode specific properties > > > per altmode and this commit adds a property for basic display-port altmode > > > support. > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > .../bindings/connector/usb-connector.txt | 3 ++ > > > drivers/usb/typec/tcpm/tcpm.c | 33 +++++++++++++++++++ > > > 2 files changed, 36 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt > > > index d357987181ee..7bae3cc9c76a 100644 > > > --- a/Documentation/devicetree/bindings/connector/usb-connector.txt > > > +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt > > > @@ -38,6 +38,9 @@ Optional properties for usb-c-connector: > > > or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC. > > > - data-role: should be one of "host", "device", "dual"(DRD) if typec > > > connector supports USB data. > > > +- displayport-vdo: The presenence of this property indicates that the > > > + usb-connector supports displayport-altmode (svid 0xff01), the value of > > > + this property is an u32 with the vdo value for the displayport-altmode, > > > > No, let's not take this approach. > > > > Every alternate mode a connector supports will need to have its own > > "sub-fwnode" under the connector fwnode. I thought we agreed this > > earlier? > > > > In any case, those sub-nodes will have default device properties named > > "svid" and "vdo". If the alternate mode still needs some other > > details, it can have other device properties that are specific to it, > > but note that displayport alt mode does not need anything extra. The > > "vdo" will already tells which pin configurations the connector > > supports and that is all that the driver needs to know. > > > > After we have the sub-nodes, it's not a big deal to walk through the > > child-nodes the port has during port registration and register the > > port alternate modes at the same time. That we can do in > > typec_register_port(), so we do not need to do it in every driver > > separately. > > Yes we did agree to do the sub-fwnode thingie. But since this is a hobby > project I do not have a whole lot of time to work on this. > > So when I started working on this, I though that the approach from this > patch-set would be more KISS and IMHO it works out well. But the sub-fwnode > approach is probably more future proof. > > Anyways as said I do not have a whole lot of time to work on this, > if you want to go the sub-fwnode route, perhaps you can do a PoC > patch series for this? I would be happy to test this and if necessary > work it into something which works for the DP case. Sure, I'll prepare something for that once I have some spare time. > Doing the port alternate modes registration from typec_register_port() > does sound like a good idea. > > The first patch in this series is independent of this and IMHO it > would be good to get that upstream regardless of this alt-mode > registration stuff, so I will resend that as a standalone patch. OK, thanks,
diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt index d357987181ee..7bae3cc9c76a 100644 --- a/Documentation/devicetree/bindings/connector/usb-connector.txt +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt @@ -38,6 +38,9 @@ Optional properties for usb-c-connector: or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC. - data-role: should be one of "host", "device", "dual"(DRD) if typec connector supports USB data. +- displayport-vdo: The presenence of this property indicates that the + usb-connector supports displayport-altmode (svid 0xff01), the value of + this property is an u32 with the vdo value for the displayport-altmode, Required properties for usb-c-connector with power delivery support: - source-pdos: An array of u32 with each entry providing supported power diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index c5fa18759f8e..2e3096657e96 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -28,6 +28,7 @@ #include <linux/usb/role.h> #include <linux/usb/tcpm.h> #include <linux/usb/typec_altmode.h> +#include <linux/usb/typec_dp.h> #include <linux/workqueue.h> #define FOREACH_STATE(S) \ @@ -281,6 +282,7 @@ struct tcpm_port { unsigned int nr_snk_pdo; u32 snk_vdo[VDO_MAX_OBJECTS]; unsigned int nr_snk_vdo; + u32 displayport_vdo; unsigned int operating_snk_mw; bool update_sink_caps; @@ -4433,6 +4435,9 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, port->nr_snk_pdo)) return -EINVAL; + fwnode_property_read_u32(fwnode, "displayport-vdo", + &port->displayport_vdo); + if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0) return -EINVAL; port->operating_snk_mw = mw / 1000; @@ -4667,6 +4672,28 @@ static int devm_tcpm_psy_register(struct tcpm_port *port) return PTR_ERR_OR_ZERO(port->psy); } +static int tcpm_register_port_altmodes(struct tcpm_port *port) +{ + struct typec_altmode_desc desc; + struct typec_altmode *alt; + int index = 0; + + if (port->displayport_vdo) { + desc.svid = USB_TYPEC_DP_SID; + desc.mode = USB_TYPEC_DP_MODE; + desc.vdo = port->displayport_vdo; + alt = typec_port_register_altmode(port->typec_port, &desc); + if (IS_ERR(alt)) + return PTR_ERR(alt); + typec_altmode_set_drvdata(alt, port); + alt->ops = &tcpm_altmode_ops; + port->port_altmode[index] = alt; + index++; + } + /* Future support for further altmodes goes here */ + return 0; +} + struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc) { struct tcpm_port *port; @@ -4736,6 +4763,10 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc) goto out_role_sw_put; } + err = tcpm_register_port_altmodes(port); + if (err) + goto out_unregister_port; + mutex_lock(&port->lock); tcpm_init(port); mutex_unlock(&port->lock); @@ -4743,6 +4774,8 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc) tcpm_log(port, "%s: registered", dev_name(dev)); return port; +out_unregister_port: + typec_unregister_port(port->typec_port); out_role_sw_put: usb_role_switch_put(port->role_sw); out_destroy_wq:
Add support for configuring display-port altmode through device-properties. We could try to add a generic mechanism for describing altmodes in device-properties, but various altmodes will likely need altmode specific configuration. E.g. the display-port altmode needs some way to describe which set of DP pins on the GPU is connected to the USB Type-C connector. As such it is better to have a separate set of altmode specific properties per altmode and this commit adds a property for basic display-port altmode support. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../bindings/connector/usb-connector.txt | 3 ++ drivers/usb/typec/tcpm/tcpm.c | 33 +++++++++++++++++++ 2 files changed, 36 insertions(+)