RE: [PATCH v2 10/12] dt-bindings: connector: add properties for typec power delivery

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi
> -----Original Message-----
> From: Heikki Krogerus [mailto:heikki.krogerus@xxxxxxxxxxxxxxx]
> Sent: 2018年3月6日 20:02
> To: Jun Li <jun.li@xxxxxxx>
> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
> robh+dt@xxxxxxxxxx; linux@xxxxxxxxxxxx; mark.rutland@xxxxxxx;
> yueyao@xxxxxxxxxx; Peter Chen <peter.chen@xxxxxxx>;
> garsilva@xxxxxxxxxxxxxx; o_leveque@xxxxxxxxx;
> shufan_lee@xxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> Subject: Re: [PATCH v2 10/12] dt-bindings: connector: add properties for
> typec power delivery
> 
> Hi guys,
> 
> On Tue, Mar 06, 2018 at 09:38:59AM +0000, Jun Li wrote:
> > > >>> diff --git
> > > >>> a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > >>> b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > >>> index e1463f1..242f6df 100644
> > > >>> ---
> > > >>> a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > >>> +++
> > > b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > >>> @@ -15,6 +15,30 @@ Optional properties:
> > > >>>  - type: size of the connector, should be specified in case of
> > > >>> USB-A,
> > > USB-B
> > > >>>    non-fullsize connectors: "mini", "micro".
> > > >>>
> > > >>> +Required properties for usb-c-connector with power delivery
> support:
> > > >>> +- port-type: should be one of "source", "sink" or "dual".
> > > >>> +- default-role: preferred power role if port-type is
> > > >>> +"dual"(drp), should be
> > > >>> +  "sink" or "source".
> > > >> Since port carries data and power, it would be better to
> > > >> explicitly mention it in names of properties which can be ambiguous.
> > > >> Maybe instead of 'port-type' you can use 'power-role', for example.
> > > > Port-type is align with the name of typec coding and ABI, 'power-role'
> > > > is more like for the current role rather than the port's ability.
> > >
> > > I am not sure what are you exactly mean by "coding and ABI", but I
> > > guess it is just about Power-Delivery part of USB-C. And if you want
> > > to put this property into USB node without mark it is related to PD
> > > part of USB connector, you risk confusion with possible USB data related
> properties.
> >
> > Understood your concern, I am following typec naming conventions, for
> > typec, port-type property has the same meaning as
> > /sys/class/typec/portx/port_type which is not only for power, also for
> > data, there are dedicated sys files power_role for power and data_role
> > for data.
> >
> > > Simple question, what if somebody wants to add property describing
> > > USB data capabilities (DFP, UFP, DRD), how should it be named?
> > >
> >
> > Then we may use data-role?
> >
> > > >
> > > >> Other thing is that default-role is required only in case of DRP,
> > > >> so maybe better would be to squash it in power-role, for example:
> > > >> ?????? power-role: should be on of "source", "sink",
> > > >> "dual-source", "dual-sink", in case of dual roles suffix determines
> preferred role.
> > > > I don't know how much this squash can benefit, "dual-source/sink"
> > > > is not directly showing its meaning by name.
> > >
> > > Some benefit is simpler binding, no conditionally-required property.
> > >
> >
> > There is already string definition for port type and preferred role
> > parse static const char * const typec_port_types[] = {
> >          [TYPEC_PORT_DFP] = "source",
> >          [TYPEC_PORT_UFP] = "sink",
> >          [TYPEC_PORT_DRP] = "dual",
> > };
> > And I think there will be other conditionally-required properties to
> > be extended later, are we going to name all of them by this way?
> > Either way is OK for me, I am not sure if there is principle like we
> > should avoid conditionally-required property, if we should, maybe
> > "dual-preferred-source/sink" is better.
> >
> > Hi Heikki,
> > Do you have any comments/preference here?
> 
> In the first versions of USB Type-C specification the data and power roles
> were in practice coupled together. There were no data role specific modes,
> just DFP, UFP and DRP. However, v1.2 of the spec.
> introduced separate modes for the data roles as well, and I have a patch for
> that (attached).
> 
> If you are asking my opinion, the data role must be handled as separate
> capability of the port, i.e. you probable want to have separate properties for
> the power role and data role, even if we did not support that yet in the class
> code. Don't use "port-type" name, just call them "power-role" and
> "data-role" from now on.
> 

OK, I will introduce power-role and data-role, meanwhile, I think the class code
should be changed accordingly(looks like your attached patch is doing that).

> The default-role should tell the state machines whether Try.SNK or Try.SRC
> states should be used or not. Perhaps you should have boolean property for
> both: "try-snk" and "try-src" (note: both can not be true).
> 

try-sink and try-source, both are optional, can't be true at the same time.

> Final note. I think it would make sense to clearly separate the USB Type-C
> specific properties from USB PD ones. Though it is unlikely that we will see
> USB PD being used with other connector types besides Type-C anymore, USB
> Type-C connectors will still definitely not always support USB PD, but when
> USB PD is not supported, we still need to know the data-role, power-role,
> default-role (or try-snk, try-src), etc.
> 

Agree, I think we can judge if the typec connector support PD by checking
if there are PD required properties(like PDO).

Thanks
Jun Li
> 
> Br,
> 
> --
> heikki
?韬{.n?????%??檩??w?{.n????z谵{???塄}?财??j:+v??????2??璀??摺?囤??z夸z罐?+?????w棹f




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux