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]

 



> -----Original Message-----
> From: Jun Li
> Sent: 2018年3月5日 15:52
> To: Rob Herring <robh@xxxxxxxxxx>; Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; heikki.krogerus@xxxxxxxxxxxxxxx;
> 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
> 
> 
> > -----Original Message-----
> > From: Rob Herring [mailto:robh@xxxxxxxxxx]
> > Sent: 2018年3月3日 6:39
> > To: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> > Cc: Jun Li <jun.li@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
> > heikki.krogerus@xxxxxxxxxxxxxxx; 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
> >
> > On Tue, Feb 27, 2018 at 09:41:19AM +0100, Andrzej Hajda wrote:
> > > On 26.02.2018 12:49, Li Jun wrote:
> > > > In case of usb-c-connector with power delivery support, add
> > > > bingdings supported by current typec driver, so user can pass all
> > > > those properties via dt.
> > > >
> > > > Signed-off-by: Li Jun <jun.li@xxxxxxx>
> > > > ---
> > > > Changes for v2:
> > > > - Added typec properties are based on general usb connector
> bindings[1]
> > > >   proposed by Andrzej Hajda.
> > > > - Use the standard unit suffixes as defined in property-units.txt.
> > > >
> > > > [1]
> > > >
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fp
> > > >
> >
> atchwork.kernel.org%2Fpatch%2F10231447%2F&data=02%7C01%7Cjun.li%
> > 40nx
> > > >
> >
> p.com%7Ccea32589f3314488e18f08d5808e5aae%7C686ea1d3bc2b4c6fa92
> > cd99c5
> > > >
> >
> c301635%7C0%7C1%7C636556271266469012&sdata=ANr1jeW8x8cy6CG6tz
> > ACgj%2B
> > > > FgNKl9DJbRpervFwaQKM%3D&reserved=0
> > > >
> > > >  .../bindings/connector/usb-connector.txt           | 43
> > ++++++++++++++++++++++
> > > >  1 file changed, 43 insertions(+)
> > > >
> > > > 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.tx
> > > > +++ t
> > > > @@ -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.
> > > 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.
> > >
> > >
> > > > +- src-pdos: An array of u32 with each entry providing supported
> > > > +power
> > > > +  source data object(PDO), the detailed bit definitions of PDO
> > > > +can be found
> > > > +  in "Universal Serial Bus Power Delivery Specification" chapter
> > > > +6.4.1.2
> > > > +  Source_Capabilities Message, the order of each entry(PDO)
> > > > +should follow
> > > > +  the PD spec chapter 6.4.1. Required for power source and power
> > > > +dual
> > role.
> > > > +- snk-pdos: An array of u32 with each entry providing supported
> > > > +power
> > > > +  sink data object(PDO), the detailed bit definitions of PDO can
> > > > +be found in
> > > > +  "Universal Serial Bus Power Delivery Specification" chapter
> > > > +6.4.1.3 Sink
> > > > +  Capabilities Message, the order of each entry(PDO) should
> > > > +follow the PD
> > > > +  spec chapter 6.4.1. Required for power sink and power dual role.
> > >
> > > We should avoid magic numbers, magic numbers are bad :)
> >
> > I don't mind if there's a defined format for the data.
> >
> 
> Yes, there is defined format in spec for this data.
> 
> > > If we really need to use PDOs here, I think we can re-use PDO_*
> > > macros [1] - DTC should be able to parse it, maybe some lifting will be
> necessary.
> > >
> > > [1]:
> > >
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Feli
> > >
> >
> xir.bootlin.com%2Flinux%2Fv4.16-rc3%2Fsource%2Finclude%2Flinux%2Fusb
> > %2
> > >
> >
> Fpd.h%23L161&data=02%7C01%7Cjun.li%40nxp.com%7Ccea32589f3314488
> > e18f08d
> > >
> >
> 5808e5aae%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636556
> > 271266469
> > >
> >
> 012&sdata=rebFCafzTpqI7FyYk9ZgW2nIhJ0ca5IB%2BBUa7WC05lM%3D&res
> > erved=0
> > > > +- max-snk-microvolt: The max voltage the sink can support in
> > > > +micro volts,
> > > > +  required for power sink and power dual role.
> > > > +- max-snk-microamp: The max current the sink can support in micro
> > > > +amps,
> > > > +  required for power sink and power dual role.
> > > > +- max-snk-microwatt-hours: The max power the sink can support in
> > > > +micro
> > > > +  Watt-hours, required for power sink and power dual role.
> > > > +- op-snk-microwatt-hours: Sink required operating power in micro
> > > > +Watt-hours,
> > > > +  if source offered power is less then it, Capability Mismatch is
> > > > +set,
> > > > +  required for power sink and power dual role.
> > >
> > > What is the relation between above properties and PDOs specified
> earlier?
> > > Are you sure all these props are required?
> > >
> > > And general remark regarding all above properties. For me, most of
> > > them are specific not to the port but to devices responsible for
> > > power
> > > management: chargers, pmics,....
> > > In many cases these properties are redundant with devices capabilities.
> > > On the other side I guess in many cases it may be convenient to put
> > > them here, so I am not sure what is better :)
> >
> > I debated that too, but decided if you had 2 connectors connected to a
> > single power controller, then you'd want these in the connector.
> >
> 
> The standard typec port controller(tcpci) can only manage one typec port
> (connector), so in my case, those props should be applied to the typec
> controller, if only for power, connector child node is not required.
> I will move those typec props descriptions to a separate file:
> (Documentation/devicetree/bindings/usb/tyepc.txt)

After clarify with typec subsystem maintainer Heikki, we aligned and
correctly understand your idea that typec props should be in usb connector
node in all cases, I will keep this, sorry for the noise.

Jun Li
> 
> > Rob
��.n��������+%������w��{.n�����{���)��jg��������ݢj����G�������j:+v���w�m������w�������h�����٥




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux