from:"Jun Li"

RE: [PATCH v4 01/13] dt-bindings: connector: add properties for typec

2018-05-01 Thread Jun Li

Hi
> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: 2018年4月30日 19:24
> To: Jun Li <jun...@nxp.com>
> Cc: robh...@kernel.org; gre...@linuxfoundation.org; li...@roeck-us.net;
> a.ha...@samsung.com; shufan_...@richtek.com; Peter Chen
> <peter.c...@nxp.com>; devicet...@vger.kernel.org;
> linux-...@vger.kernel.org; dl-linux-imx <linux-...@nxp.com>;
> de...@driverdev.osuosl.org
> Subject: Re: [PATCH v4 01/13] dt-bindings: connector: add properties for typec
> 
> Hi,
> 
> On Thu, Mar 29, 2018 at 12:06:06AM +0800, Li Jun wrote:
> > +Optional properties for usb-c-connector:
> > +- power-type: should be one of "source", "sink" or "dual"(DRP) if typec
> > +  connector has power support.
> > +- try-power-role: preferred power role if "dual"(DRP) can support Try.SNK
> > +  or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> > +- data-type: should be one of "host", "device", "dual"(DRD) if typec
> > +  connector supports USB data.
> 
> Please use "power-role" and "data-role".

OK. I will update these 2 property names as you suggested.

Li Jun
> 
> 
> Thanks,
> 
> --
> heikki
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v4 02/13] dt-bindings: usb: add documentation for typec port controller(TCPCI)

2018-05-01 Thread Jun Li

Hi,
> -Original Message-
> From: Mats Karrman [mailto:mats.dev.l...@gmail.com]
> Sent: 2018年4月30日 15:41
> To: Jun Li <jun...@nxp.com>
> Cc: robh...@kernel.org; gre...@linuxfoundation.org;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net; a.ha...@samsung.com;
> shufan_...@richtek.com; Peter Chen <peter.c...@nxp.com>;
> devicet...@vger.kernel.org; linux-...@vger.kernel.org; dl-linux-imx
> <linux-...@nxp.com>; de...@driverdev.osuosl.org
> Subject: Re: [PATCH v4 02/13] dt-bindings: usb: add documentation for typec
> port controller(TCPCI)
> 
> Hi Li Jun,
> 
> Are you working on an updated version of this patch series?
> I'm pondering other changes that builds on these patches (the documentation
> and the fwnode added to the tcpc_dev and tcpm primarily).

I am on a vacation and will be back tomorrow, I will post a new version soon.

> 
> Btw, there is a semi-colon missing in your example below.

Thanks, I will add it.

Li Jun
> 
> BR // Mats
> 
> On 2018-03-28 18:06, Li Jun wrote:
> 
> > TCPCI stands for typec port controller interface, its implementation
> > has full typec port control with power delivery support, it's a
> > standard i2c slave with GPIO input as irq interface, detail see spec
> > "Universal Serial Bus Type-C Port Controller Interface Specification
> > Revision 1.0, Version 1.1"
> >
> > Signed-off-by: Li Jun <jun...@nxp.com>
> > ---
> >   .../devicetree/bindings/usb/typec-tcpci.txt| 33
> ++
> >   1 file changed, 33 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> > b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> > new file mode 100644
> > index 000..7a7a8e0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> > @@ -0,0 +1,33 @@
> > +TCPCI(Typec port cotroller interface) binding
> > +-
> > +
> > +Required properties:
> > +- compatible:   should be "usb-tcpci,chip-specific-string".
> > +- reg:  the i2c slave address of typec port controller device.
> > +- interrupt-parent: the phandle to the interrupt controller which provides
> > +the interrupt.
> > +- interrupts:   interrupt specification for tcpci alert.
> > +
> > +Required sub-node:
> > +- connector: The "usb-c-connector" attached to the tcpci chip, the
> > +bindings
> > +  of connector node are specified in
> > +  Documentation/devicetree/bindings/connector/usb-connector.txt
> > +
> > +Example:
> > +
> > +ptn5110@50 {
> > +   compatible = "usb-tcpci,ptn5110";
> > +   reg = <0x50>;
> > +   interrupt-parent = <>;
> > +   interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> > +
> > +   usb_con: connector {
> > +   compatible = "usb-c-connector";
> > +   label = "USB-C";
> > +   port-type = "dual";
> > +   try-power-role = "sink"
> 
> Here!
> 
> > +   source-pdos = <0x380190c8>;
> > +   sink-pdos = <0x380190c8 0x3802d0c8>;
> > +   op-sink-microwatt-hours = <900>;
> > +   };
> > +};
> >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v4 02/13] dt-bindings: usb: add documentation for typec port controller(TCPCI)

2018-04-19 Thread Jun Li

> -Original Message-
> From: Rob Herring [mailto:r...@kernel.org]
> Sent: 2018年4月16日 22:28
> To: Jun Li <jun...@nxp.com>
> Cc: gre...@linuxfoundation.org; heikki.kroge...@linux.intel.com;
> li...@roeck-us.net; a.ha...@samsung.com; shufan_...@richtek.com; Peter
> Chen <peter.c...@nxp.com>; devicet...@vger.kernel.org;
> linux-...@vger.kernel.org; dl-linux-imx <linux-...@nxp.com>;
> de...@driverdev.osuosl.org
> Subject: Re: [PATCH v4 02/13] dt-bindings: usb: add documentation for typec
> port controller(TCPCI)
> 
> On Mon, Apr 16, 2018 at 6:54 AM, Jun Li <jun...@nxp.com> wrote:
> > Hi
> >> -Original Message-
> >> From: Rob Herring [mailto:r...@kernel.org]
> >> Sent: 2018年4月10日 4:04
> >> To: Jun Li <jun...@nxp.com>
> >> Cc: gre...@linuxfoundation.org; heikki.kroge...@linux.intel.com;
> >> li...@roeck-us.net; a.ha...@samsung.com; shufan_...@richtek.com;
> >> Peter Chen <peter.c...@nxp.com>; devicet...@vger.kernel.org;
> >> linux-...@vger.kernel.org; dl-linux-imx <linux-...@nxp.com>;
> >> de...@driverdev.osuosl.org
> >> Subject: Re: [PATCH v4 02/13] dt-bindings: usb: add documentation for
> >> typec port controller(TCPCI)
> >>
> >> On Thu, Mar 29, 2018 at 12:06:07AM +0800, Li Jun wrote:
> 
> [...]
> 
> >> > +ptn5110@50 {
> >> > +   compatible = "usb-tcpci,ptn5110";
> >> > +   reg = <0x50>;
> >> > +   interrupt-parent = <>;
> >> > +   interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> >> > +
> >> > +   usb_con: connector {
> >>
> >> How is the OF graph done in this case? You need some link to the USB
> controller.
> >
> > The platform(i.MX8MQ EVK) for this is still on the way of start
> > upstream, I was Planning to add this part with enabling USB3 function,
> > as of how this will be done, I only have usb3 ss data(no display port or
> Sideband), is something like below OK?
> >
> > typec: ptn5110@50 {
> > compatible = "nxp,ptn5110";
> > ...
> >
> > usb_con: connector {
> > compatible = "usb-c-connector";
> > label = "USB-C";
> > ...
> >
> > ports {
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > port@1 {
> > reg = <1>;
> > usb_con_ss: endpoint {
> > remote-endpoint = <_phy_ss>;
> > };
> > };
> > };
> > };
> > };
> >
> > _phy0 {
> > status = "okay";
> >
> > port {
> > usb3_phy_ss: endpoint {
> 
> Normally, the graph connection would be to the USB controller, not the phy as
> the phy is just referred to with a "phys" property.

Understood, I will put this into a USB controller node. Thanks.

Jun
> 
> > remote-endpoint = <_con_ss>;
> > };
> > };
> > }
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v4 02/13] dt-bindings: usb: add documentation for typec port controller(TCPCI)

2018-04-16 Thread Jun Li

Hi
> -Original Message-
> From: Rob Herring [mailto:r...@kernel.org]
> Sent: 2018年4月10日 4:04
> To: Jun Li <jun...@nxp.com>
> Cc: gre...@linuxfoundation.org; heikki.kroge...@linux.intel.com;
> li...@roeck-us.net; a.ha...@samsung.com; shufan_...@richtek.com; Peter
> Chen <peter.c...@nxp.com>; devicet...@vger.kernel.org;
> linux-...@vger.kernel.org; dl-linux-imx <linux-...@nxp.com>;
> de...@driverdev.osuosl.org
> Subject: Re: [PATCH v4 02/13] dt-bindings: usb: add documentation for typec
> port controller(TCPCI)
> 
> On Thu, Mar 29, 2018 at 12:06:07AM +0800, Li Jun wrote:
> > TCPCI stands for typec port controller interface, its implementation
> > has full typec port control with power delivery support, it's a
> > standard i2c slave with GPIO input as irq interface, detail see spec
> > "Universal Serial Bus Type-C Port Controller Interface Specification
> > Revision 1.0, Version 1.1"
> >
> > Signed-off-by: Li Jun <jun...@nxp.com>
> > ---
> >  .../devicetree/bindings/usb/typec-tcpci.txt| 33
> ++
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> > b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> > new file mode 100644
> > index 000..7a7a8e0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> > @@ -0,0 +1,33 @@
> > +TCPCI(Typec port cotroller interface) binding
> > +-
> > +
> > +Required properties:
> > +- compatible:   should be "usb-tcpci,chip-specific-string".
> 
> Compatible strings should be in the form of ","
> 

OK, I will list the specific compatible string here and change
my example case to be "nxp,ptn5110".

> > +- reg:  the i2c slave address of typec port controller device.
> > +- interrupt-parent: the phandle to the interrupt controller which provides
> > +the interrupt.
> > +- interrupts:   interrupt specification for tcpci alert.
> > +
> > +Required sub-node:
> > +- connector: The "usb-c-connector" attached to the tcpci chip, the
> > +bindings
> > +  of connector node are specified in
> > +  Documentation/devicetree/bindings/connector/usb-connector.txt
> > +
> > +Example:
> > +
> > +ptn5110@50 {
> > +   compatible = "usb-tcpci,ptn5110";
> > +   reg = <0x50>;
> > +   interrupt-parent = <>;
> > +   interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> > +
> > +   usb_con: connector {
> 
> How is the OF graph done in this case? You need some link to the USB 
> controller.

The platform(i.MX8MQ EVK) for this is still on the way of start upstream, I was
Planning to add this part with enabling USB3 function, as of how this will be 
done,
I only have usb3 ss data(no display port or Sideband), is something like below 
OK?

typec: ptn5110@50 {
compatible = "nxp,ptn5110";
...

usb_con: connector {
compatible = "usb-c-connector";
label = "USB-C";
...

ports {
#address-cells = <1>;
#size-cells = <0>;

port@1 {
reg = <1>;
usb_con_ss: endpoint {
remote-endpoint = <_phy_ss>;
};
};
};
};
};

_phy0 {
status = "okay";

port {
usb3_phy_ss: endpoint {
remote-endpoint = <_con_ss>;
};
};
}

Thanks
Jun
> 
> > +   compatible = "usb-c-connector";
> > +   label = "USB-C";
> > +   port-type = "dual";
> > +   try-power-role = "sink"
> > +   source-pdos = <0x380190c8>;
> > +   sink-pdos = <0x380190c8 0x3802d0c8>;
> > +   op-sink-microwatt-hours = <900>;
> > +   };
> > +};
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree"
> > in the body of a message to majord...@vger.kernel.org More majordomo
> > info at
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger
> > .kernel.org%2Fmajordomo-info.html=02%7C01%7Cjun.li%40nxp.com%7C
> 86
> >
> a7c0da18204df434d208d59e550c27%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %
> >
> 7C0%7C636589010562193065=0%2FmoDrqWn9YghWGucWYnMd1YK0BO2
> dVgp%2Fa
> > KNZZZ%2BXE%3D=0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 01/13] dt-bindings: connector: add properties for typec

2018-04-13 Thread Jun Li

2018-04-03 16:29 GMT+08:00 Andrzej Hajda :
>
> On 28.03.2018 18:06, Li Jun wrote:
> > Add bingdings supported by current typec driver, so user can pass
> > all those properties via dt.
> >
> > Signed-off-by: Li Jun 
> > ---
> >  .../bindings/connector/usb-connector.txt   | 39 
> > ++
> >  1 file changed, 39 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt 
> > b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > index e1463f1..922f22b 100644
> > --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > @@ -15,6 +15,29 @@ Optional properties:
> >  - type: size of the connector, should be specified in case of USB-A, USB-B
> >non-fullsize connectors: "mini", "micro".
> >
> > +Optional properties for usb-c-connector:
> > +- power-type: should be one of "source", "sink" or "dual"(DRP) if typec
> > +  connector has power support.
> > +- try-power-role: preferred power role if "dual"(DRP) can support Try.SNK
> > +  or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> > +- data-type: should be one of "host", "device", "dual"(DRD) if typec
> > +  connector supports USB data.
> > +
> > +Required properties for usb-c-connector with power delivery support:
> > +- source-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.
> > +- sink-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.
> > +- op-sink-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.
>
> I have lurked into specs and I am not sure what is the relation of this
> field with PD protocol, there is Minimum Operating Power, Maximum
> Operating Power and Operating Power present in different RDOs, there are
> also similar fields in sink PDOs.
> I guess it can be one of them, which one?

Per current usage of op-sink-microwatt(operating_snk_mw in tcpm),
this property is actually for max operating power, which is only used to
judge capability mismatch, i.e. the required max current/power is larger
than the selected source PDO can provide.

After checking the PD spec about this, basically I think this property also
can be removed, we can judge the capability mismatch by compare the
selected source pdo and sink pdo, considering there are existing users
of this field(operating_snk_mw in tcpm), I will check how this can be done.

I am removing the other 3 max_snk_*, see [1]

[1] https://www.spinics.net/lists/linux-usb/msg167262.html

> why other ones are not
> provided? What if this or any other property can be calculated only in
> runtime?
> I am not sure if any of them should be marked "required".
>
> > +
> >  Required nodes:
> >  - any data bus to the connector should be modeled using the OF graph 
> > bindings
> >specified in bindings/graph.txt, unless the bus is between parent node 
> > and
> > @@ -73,3 +96,19 @@ ccic: s2mm005@33 {
> >   };
> >   };
> >  };
> > +
> > +3. USB-C connector attached to a typec port controller(ptn5110), which has
> > +power delivery support and enables drp.
> > +
> > +typec: ptn5110@50 {
> > + ...
> > + usb_con: connector {
> > + compatible = "usb-c-connector";
> > + label = "USB-C";
> > + power-type = "dual";
> > + try-power-role = "sink";
> > + source-pdos = <0x380190c8>;
>
> I understand from DT specification point of view cryptic numbers are OK,
> but for sake of readability I strongly suggest use/define macros similar
> to the ones present already in kernel:
> source-pdos = < PDO_FIXED(5000, 400, PDO_FIXED_FLAGS) >;

I agree it's easier to read, so with the proposed way, we need maintain
a copy of those macros for dts(e.g. under dt-binding/usb/pd.h), right?

thanks
Jun
>
> It much less error prone, and makes review process much more easier.
>
> Regards
> Andrzej
>
> > + sink-pdos = <0x380190c8 0x3802d0c8>;
> > + op-sink-microwatt-hours = <900>;
> > + };
> > +};
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org

RE: [PATCH v4 11/13] staging: typec: tcpci: keep the not connecting cc line open

2018-03-30 Thread Jun Li

Hi
> -Original Message-
> From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter Roeck
> Sent: 2018年3月30日 23:16
> To: Jun Li <jun...@nxp.com>; robh...@kernel.org; gre...@linuxfoundation.org;
> heikki.kroge...@linux.intel.com
> Cc: a.ha...@samsung.com; shufan_...@richtek.com; Peter Chen
> <peter.c...@nxp.com>; devicet...@vger.kernel.org;
> linux-...@vger.kernel.org; dl-linux-imx <linux-...@nxp.com>;
> de...@driverdev.osuosl.org
> Subject: Re: [PATCH v4 11/13] staging: typec: tcpci: keep the not connecting 
> cc
> line open
> 
> On 03/28/2018 09:06 AM, Li Jun wrote:
> > While set polarity, we should keep the not connecting cc line to be
> > open.
> >
> 
> The more I look at this code, the more I am confused by it.
> 
> The original code doesn't touch the CC lines. This function only sets the 
> polarity.
> Is it really appropriate to touch the CC lines in the same function ?
> 

Yes, I didn't find a more proper place to do this, either I change the
tcpc->set_cc() interface with orientation/polarity parameter to know
which cc line I should keep it open, or do it in low level driver like
this, do you have any suggestion how this can be done?(I guess
both cc lines have the same state after attached with current code
of all tcpm users, but this should be resolved as it's break PD compliance
test) 

thanks
Li Jun

> Guenter
> 
> > Signed-off-by: Li Jun <jun...@nxp.com>
> > ---
> >   drivers/staging/typec/tcpci.c | 18 ++
> >   1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/typec/tcpci.c
> > b/drivers/staging/typec/tcpci.c index d5b4e4e..b58bd59 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -185,15 +185,25 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
> >   enum typec_cc_polarity polarity)
> >   {
> > struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > +   unsigned int reg;
> > int ret;
> >
> > -   ret = regmap_write(tcpci->regmap, TCPC_TCPC_CTRL,
> > -  (polarity == TYPEC_POLARITY_CC2) ?
> > -  TCPC_TCPC_CTRL_ORIENTATION : 0);
> > +   /* Keep the disconnect cc line open */
> > +   ret = regmap_read(tcpci->regmap, TCPC_ROLE_CTRL, );
> > if (ret < 0)
> > return ret;
> >
> > -   return 0;
> > +   if (polarity == TYPEC_POLARITY_CC2)
> > +   reg |= TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC1_SHIFT;
> > +   else
> > +   reg |= TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC2_SHIFT;
> > +   ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   return regmap_write(tcpci->regmap, TCPC_TCPC_CTRL,
> > +  (polarity == TYPEC_POLARITY_CC2) ?
> > +  TCPC_TCPC_CTRL_ORIENTATION : 0);
> >   }
> >
> >   static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool enable)
> >

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach

2018-03-30 Thread Jun Li



> -Original Message-
> From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter Roeck
> Sent: 2018年3月30日 6:49
> To: Jun Li <jun...@nxp.com>
> Cc: robh...@kernel.org; gre...@linuxfoundation.org;
> heikki.kroge...@linux.intel.com; a.ha...@samsung.com;
> shufan_...@richtek.com; Peter Chen <peter.c...@nxp.com>;
> devicet...@vger.kernel.org; linux-...@vger.kernel.org; dl-linux-imx
> <linux-...@nxp.com>; de...@driverdev.osuosl.org
> Subject: Re: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach
> 
> On Thu, Mar 29, 2018 at 12:06:15AM +0800, Li Jun wrote:
> > In case of drp toggling, we may need set correct cc value for role
> > control after attach as it may never been set.
> >
> 
> Isn't CC set by the lower level driver in this case ? In other words, is it 
> ever
> necessary to call back into the low level driver to set CC again ? Doing that 
> in
> attached state seems a bit late.

In question case(drp without try.src or try.snk), you can see tcpm never call
tcpm_set_cc() from drp toggling to attached state, start_drp_toggling
set cc at the beginning in this case, but the value is the *start* value, may 
not
the right value to match its final role.

Per tcpci spec
Figure 4-16. DRP Initialization and Connection Detection
After debounce, tcpm should set cc(again, but may different value) and polarity.

ShuFan encountered this case as I understood on his setup like below:
- Tcpc start drp toggling with Rp/Rp,
- Connect to adapter with also Rp/Rp
- After connection resolved, the tcpc becomes a sink(HW present Rd), but
role control register value is still Rp/Rp.
- This should be corrected to set cc for keep the un-contacted cc line open.

> 
> It may make more sense to update port->cc_req when the state machine leaves
> DRP_TOGGLING state, ie in _tcpm_cc_change(), and to do it without callback 
> into
> the low level driver (it should not be necessary).

Currently there is no user of cc_req flag, so I can't catch the real intention
of this flag, as of now it's only set in tcpm_set_cc(), so I understood it's
for if tcpm already set tcpc a specific role.

Li Jun
> 
> Guenter
> 
> > Signed-off-by: Li Jun <jun...@nxp.com>
> > ---
> >  drivers/usb/typec/tcpm.c | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index
> > 218c230..72d4232 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -2126,6 +2126,7 @@ static void tcpm_reset_port(struct tcpm_port *port)
> > tcpm_set_attached_state(port, false);
> > port->try_src_count = 0;
> > port->try_snk_count = 0;
> > +   port->cc_req = 0;
> >  }
> >
> >  static void tcpm_detach(struct tcpm_port *port) @@ -2361,6 +2362,8 @@
> > static void run_state_machine(struct tcpm_port *port)
> > break;
> >
> > case SRC_ATTACHED:
> > +   if (!port->cc_req)
> > +   tcpm_set_cc(port, tcpm_rp_cc(port));
> > ret = tcpm_src_attach(port);
> > tcpm_set_state(port, SRC_UNATTACHED,
> >ret < 0 ? 0 : PD_T_PS_SOURCE_ON); @@ -2531,6 
> > +2534,8
> @@
> > static void run_state_machine(struct tcpm_port *port)
> > tcpm_set_state(port, SNK_UNATTACHED, PD_T_PD_DEBOUNCE);
> > break;
> > case SNK_ATTACHED:
> > +   if (!port->cc_req)
> > +   tcpm_set_cc(port, TYPEC_CC_RD);
> > ret = tcpm_snk_attach(port);
> > if (ret < 0)
> > tcpm_set_state(port, SNK_UNATTACHED, 0);
> > --
> > 2.7.4
> >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v4 04/13] usb: typec: add fwnode to tcpc

2018-03-30 Thread Jun Li

Hi
> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: 2018年3月29日 20:58
> To: Jun Li <jun...@nxp.com>
> Cc: robh...@kernel.org; gre...@linuxfoundation.org; li...@roeck-us.net;
> a.ha...@samsung.com; shufan_...@richtek.com; Peter Chen
> <peter.c...@nxp.com>; devicet...@vger.kernel.org;
> linux-...@vger.kernel.org; dl-linux-imx <linux-...@nxp.com>;
> de...@driverdev.osuosl.org
> Subject: Re: [PATCH v4 04/13] usb: typec: add fwnode to tcpc
> 
> Hi,
> 
> On Thu, Mar 29, 2018 at 12:06:09AM +0800, Li Jun wrote:
> > Add fwnode handle to get the fwnode so we can get typec configs it
> > contains.
> >
> > Suggested-by: Heikki Krogerus <heikki.kroge...@linux.intel.com>
> > Signed-off-by: Li Jun <jun...@nxp.com>
> > ---
> >  drivers/staging/typec/tcpci.c | 14 +++---
> >  drivers/usb/typec/tcpm.c  |  1 +
> >  include/linux/usb/tcpm.h  |  2 ++
> >  3 files changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/typec/tcpci.c
> > b/drivers/staging/typec/tcpci.c index ed76327..4f7ad10 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -10,6 +10,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -463,17 +464,16 @@ static const struct regmap_config
> tcpci_regmap_config = {
> > .max_register = 0x7F, /* 0x80 .. 0xFF are vendor defined */  };
> >
> > -static const struct tcpc_config tcpci_tcpc_config = {
> > -   .type = TYPEC_PORT_DFP,
> > -   .default_role = TYPEC_SINK,
> > -};
> > -
> >  static int tcpci_parse_config(struct tcpci *tcpci)  {
> > tcpci->controls_vbus = true; /* XXX */
> >
> > -   /* TODO: Populate struct tcpc_config from ACPI/device-tree */
> > -   tcpci->tcpc.config = _tcpc_config;
> 
> That will break bisectablitity. tcpm.c is still accessing the config at this 
> point.
> 

Yes, good catch.

> Just leave those untouched in here, and clean-up in separate patch that comes
> after the patch that prepares tcpm.c.

I will change in next version, thanks.

Li Jun
> 
> 
> Thanks,
> 
> --
> heikki
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach

2018-03-30 Thread Jun Li

Hi
> -Original Message-
> From: Mats Karrman [mailto:mats.dev.l...@gmail.com]
> Sent: 2018年3月30日 5:19
> To: Jun Li <jun...@nxp.com>; robh...@kernel.org; gre...@linuxfoundation.org;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net
> Cc: a.ha...@samsung.com; shufan_...@richtek.com; Peter Chen
> <peter.c...@nxp.com>; devicet...@vger.kernel.org;
> linux-...@vger.kernel.org; dl-linux-imx <linux-...@nxp.com>;
> de...@driverdev.osuosl.org
> Subject: Re: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach
> 
> Hi Li,
> 
> On 03/28/2018 06:06 PM, Li Jun wrote:
> 
> > In case of drp toggling, we may need set correct cc value for role
> > control after attach as it may never been set.
> >
> > Signed-off-by: Li Jun <jun...@nxp.com>
> > ---
> >  drivers/usb/typec/tcpm.c | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index
> > 218c230..72d4232 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -2126,6 +2126,7 @@ static void tcpm_reset_port(struct tcpm_port *port)
> > tcpm_set_attached_state(port, false);
> > port->try_src_count = 0;
> > port->try_snk_count = 0;
> > +   port->cc_req = 0;
> 
> I don't think it's OK to use "0" here. cc_req is an enum so why not use
> "|TYPEC_CC_OPEN"?|
> 

I will change to be TYPEC_CC_OPEN, also other place.

Li Jun
> >  }
> >
> >  static void tcpm_detach(struct tcpm_port *port) @@ -2361,6 +2362,8 @@
> > static void run_state_machine(struct tcpm_port *port)
> > break;
> >
> > case SRC_ATTACHED:
> > +   if (!port->cc_req)
> 
>           if (port->cc_req == |TYPEC_CC_OPEN)|
> 
> > +   tcpm_set_cc(port, tcpm_rp_cc(port));
> > ret = tcpm_src_attach(port);
> > tcpm_set_state(port, SRC_UNATTACHED,
> >ret < 0 ? 0 : PD_T_PS_SOURCE_ON); @@ -2531,6 
> > +2534,8
> @@
> > static void run_state_machine(struct tcpm_port *port)
> > tcpm_set_state(port, SNK_UNATTACHED, PD_T_PD_DEBOUNCE);
> > break;
> > case SNK_ATTACHED:
> > +   if (!port->cc_req)
> 
> Ditto.
> 
> > +   tcpm_set_cc(port, TYPEC_CC_RD);
> > ret = tcpm_snk_attach(port);
> > if (ret < 0)
> > tcpm_set_state(port, SNK_UNATTACHED, 0);
> 
> // Mats
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v4 01/13] dt-bindings: connector: add properties for typec

2018-03-30 Thread Jun Li

Hi
> -Original Message-
> From: Mats Karrman [mailto:mats.dev.l...@gmail.com]
> Sent: 2018年3月30日 3:54
> To: Jun Li <jun...@nxp.com>; robh...@kernel.org; gre...@linuxfoundation.org;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net
> Cc: a.ha...@samsung.com; shufan_...@richtek.com; Peter Chen
> <peter.c...@nxp.com>; devicet...@vger.kernel.org;
> linux-...@vger.kernel.org; dl-linux-imx <linux-...@nxp.com>;
> de...@driverdev.osuosl.org
> Subject: Re: [PATCH v4 01/13] dt-bindings: connector: add properties for typec
> 
> Hi Li,
> 
> On 03/28/2018 06:06 PM, Li Jun wrote:
> 
> > Add bingdings supported by current typec driver, so user can pass all
> > those properties via dt.
> >
> > Signed-off-by: Li Jun <jun...@nxp.com>
> > ---
> >  .../bindings/connector/usb-connector.txt   | 39
> ++
> >  1 file changed, 39 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > index e1463f1..922f22b 100644
> > --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > @@ -15,6 +15,29 @@ Optional properties:
> >  - type: size of the connector, should be specified in case of USB-A, USB-B
> >non-fullsize connectors: "mini", "micro".
> >
> > +Optional properties for usb-c-connector:
> > +- power-type: should be one of "source", "sink" or "dual"(DRP) if
> > +typec
> > +  connector has power support.
> > +- try-power-role: preferred power role if "dual"(DRP) can support
> > +Try.SNK
> > +  or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> > +- data-type: should be one of "host", "device", "dual"(DRD) if typec
> > +  connector supports USB data.
> > +
> > +Required properties for usb-c-connector with power delivery support:
> > +- source-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.
> > +- sink-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.
> > +- op-sink-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.
> 
> This doesn't make sense. The unit of power is watt (W), watt-hour on the other
> hand is a measurement of energy. I think "op-sink-microwatt" is what we want
> here.

Yes, you are right, microwatt is what I should use here. I will change.

Li Jun
> 
> // Mats
> 
> > +
> >  Required nodes:
> >  - any data bus to the connector should be modeled using the OF graph
> bindings
> >specified in bindings/graph.txt, unless the bus is between parent
> > node and @@ -73,3 +96,19 @@ ccic: s2mm005@33 {
> > };
> > };
> >  };
> > +
> > +3. USB-C connector attached to a typec port controller(ptn5110),
> > +which has power delivery support and enables drp.
> > +
> > +typec: ptn5110@50 {
> > +   ...
> > +   usb_con: connector {
> > +   compatible = "usb-c-connector";
> > +   label = "USB-C";
> > +   power-type = "dual";
> > +   try-power-role = "sink";
> > +   source-pdos = <0x380190c8>;
> > +   sink-pdos = <0x380190c8 0x3802d0c8>;
> > +   op-sink-microwatt-hours = <900>;
> > +   };
> > +};
> >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v4 07/13] staging: typec: tcpci: register port before request irq

2018-03-30 Thread Jun Li

Hi
> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: 2018年3月29日 18:52
> To: Jun Li <jun...@nxp.com>
> Cc: robh...@kernel.org; gre...@linuxfoundation.org;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net;
> de...@driverdev.osuosl.org; devicet...@vger.kernel.org; Peter Chen
> <peter.c...@nxp.com>; linux-...@vger.kernel.org; a.ha...@samsung.com;
> dl-linux-imx <linux-...@nxp.com>; shufan_...@richtek.com
> Subject: Re: [PATCH v4 07/13] staging: typec: tcpci: register port before 
> request
> irq
> 
> On Thu, Mar 29, 2018 at 12:06:12AM +0800, Li Jun wrote:
> > With that we can clear any pending events and the port is registered
> > so driver can be ready to handle typec events once we request irq.
> >
> > Signed-off-by: Peter Chen <peter.c...@nxp.com>
> > Signed-off-by: Li Jun <jun...@nxp.com>
> 
> These sign offs aren't clear.
> 
> Sign offs mean that you handled the patch but didn't include any of SCO's
> copyrighted UNIX code into it.  Normally they're in the order of who touched
> the code.  So Peter touched the code first.  Should he get authorship credit?

I will change the patch author to be Peter as he touched the code first.

> How did he touch the code first if he didn't write the code?  It doesn't make
> sense.
> 
> > ---
> >  drivers/staging/typec/tcpci.c | 15 ---
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/typec/tcpci.c
> > b/drivers/staging/typec/tcpci.c index 4f7ad10..9e0014b 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -537,25 +537,26 @@ static int tcpci_probe(struct i2c_client *client,
> > if (IS_ERR(chip->data.regmap))
> > return PTR_ERR(chip->data.regmap);
> >
> > +   i2c_set_clientdata(client, chip);
> > +
> > /* Disable chip interrupts before requesting irq */
> > err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, ,
> >sizeof(u16));
> > if (err < 0)
> > return err;
> >
> > +   chip->tcpci = tcpci_register_port(>dev, >data);
> > +   if (PTR_ERR_OR_ZERO(chip->tcpci))
> > +   return PTR_ERR(chip->tcpci);
> 
> When a function returns both error pointers and NULL that means that NULL is a
> secial case of success.  Like for example:
> 
>   p->my_feature = get_optional_feature();
> 
> If it returns NULL that means the optional feature isn't there, but it's fine 
> because
> it's optional.  But if it returns an error pointer that means the feature is 
> there
> but the hardware is buggy or something so we shouldn't continue.
> 
> If you return PTR_ERR(NULL) that means success.
> 
> I don't think this code makes sense just from looking at it and also when I
> checked tcpci_register_port() doesn't return NULL.

This patch is to change the sequence of register port and request irq,
if error code checking of original code has the problem, I think that
should be another patch to fix it, I can do that later.

> 
> 
> 
> > +
> > err = devm_request_threaded_irq(>dev, client->irq, NULL,
> > _tcpci_irq,
> > IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> > dev_name(>dev), chip);
> > if (err < 0)
> > -   return err;
> > +   tcpci_unregister_port(chip->tcpci);
> 
> Can you put the "return err;" back, because that's better style.  It's better 
> to
> keep the error path and success path separate if you can.
> 
>   if (err < 0) {
>   tcpci_unregister_port(chip->tcpci);
>   return err;
>   }
> 
>   return 0;
> 

OK, I will change as you suggested, thanks.

Li Jun
> 
> regards,
> dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v4 01/13] dt-bindings: connector: add properties for typec

RE: [PATCH v4 02/13] dt-bindings: usb: add documentation for typec port controller(TCPCI)

RE: [PATCH v4 02/13] dt-bindings: usb: add documentation for typec port controller(TCPCI)

RE: [PATCH v4 02/13] dt-bindings: usb: add documentation for typec port controller(TCPCI)

Re: [PATCH v4 01/13] dt-bindings: connector: add properties for typec

RE: [PATCH v4 11/13] staging: typec: tcpci: keep the not connecting cc line open

RE: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach

RE: [PATCH v4 04/13] usb: typec: add fwnode to tcpc

RE: [PATCH v4 10/13] usb: typec: tcpm: set cc for drp toggling attach

RE: [PATCH v4 01/13] dt-bindings: connector: add properties for typec

RE: [PATCH v4 07/13] staging: typec: tcpci: register port before request irq

11 matches

Site Navigation

Mail list logo

Footer information