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: 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.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.
> > 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)

> Rob
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��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