mbox series

[v4,00/13] staging: typec: tcpci: move out of staging

Message ID 1522253178-32414-1-git-send-email-jun.li@nxp.com
Headers show
Series staging: typec: tcpci: move out of staging | expand

Message

Jun Li March 28, 2018, 4:06 p.m. UTC 
This patch set attempts to move the tcpci driver out of staging by fix
some tcpci driver issues and define typec and power delivery device
properties, the changes are verified on NXP PTN5110, which is a standard
tcpci typec port controller device with power delivery support, tested
power source and sink with drp config.

Changes for v4:
- Remove max-sink-* properties as we will purge max_snk_* in tcpm,
  see patch set[4].
- Get typec power and data type value via name string(patch 5).
- Move finding typec and pd properties code from tcpci to tcpm(patch 6)
- Add a compatible string for nxp ptn5110 typec controller in tcpci driver.
  (patch 3)
- Add set cc for drp toggling without try.src/snk in tcpm(patch 10), then
  patch 11 can only update CCx bits for keep disconnect cc line open.
- Update op-sink-microwatt-hours example value to be the right value in
  micorwatts, and accordingly divide 1000 to get its miliwatts value
  in patch 6.
- Add Guenter's Reviewed-by for patch(8/9/12)

[4] https://www.spinics.net/lists/linux-usb/msg167261.html

Changes for v3:
- Use 2 properties to separate power and data capability of typec port:
  "power-type" and "data-type", this is based on Heikki's typec class code
  change[2]. use "try-power-role" to present if the typec port can support
  Try.SNK or Try.SRC.
- 4 sink properties(max_sink_mv/ma/mw and op_sink_mw) are kept because the
  counterpart code is back, see revert patch[3], meanwhile I post a patch
  to fix the reported problem of current source pdo select machinism(which
  completely ignored those 4 sink settings), to see if we can keep current
  code, once it was discussed and have conclusion I can update this
  accordingly.
- Use fwnode to get the connector node for dt setting parse.

Main changes for v2:
- 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.
- Add 2 infra APIs to get power sink and source config from dt.
- Don't change the set_cc api, to keep the uncontacted cc line open,
  set cc1/cc2 to be open in tcpci driver when set polarity.
- Directly enable vbus detect in tcpci driver rather than add a API.
- Details added in each patch.

[1] https://patchwork.kernel.org/patch/10231447/
[2] https://patchwork.kernel.org/patch/10276483/
[3] https://www.spinics.net/lists/linux-usb/msg166366.html

Li Jun (13):
  dt-bindings: connector: add properties for typec
  dt-bindings: usb: add documentation for typec port controller(TCPCI)
  staging: typec: tcpci: add compatible string for nxp ptn5110
  usb: typec: add fwnode to tcpc
  usb: typec: add API to get typec basic port power and data config
  usb: typec: tcpm: support get typec and pd config from device
    properties
  staging: typec: tcpci: register port before request irq
  staging: typec: tcpci: enable vbus detection
  typec: tcpm: add starting value for drp toggling
  usb: typec: tcpm: set cc for drp toggling attach
  staging: typec: tcpci: keep the not connecting cc line open
  staging: typec: tcpci: Only touch target bit when enable vconn
  staging: typec: tcpci: move tcpci driver out of staging

 .../bindings/connector/usb-connector.txt           |  39 ++
 .../devicetree/bindings/usb/typec-tcpci.txt        |  33 ++
 drivers/staging/Kconfig                            |   2 -
 drivers/staging/Makefile                           |   1 -
 drivers/staging/typec/Kconfig                      |  14 -
 drivers/staging/typec/Makefile                     |   1 -
 drivers/staging/typec/TODO                         |   5 -
 drivers/staging/typec/tcpci.c                      | 596 --------------------
 drivers/staging/typec/tcpci.h                      | 138 -----
 drivers/usb/typec/Kconfig                          |   7 +
 drivers/usb/typec/Makefile                         |   1 +
 drivers/usb/typec/class.c                          |  52 ++
 drivers/usb/typec/tcpci.c                          | 611 +++++++++++++++++++++
 drivers/usb/typec/tcpci.h                          | 138 +++++
 drivers/usb/typec/tcpm.c                           | 156 +++++-
 include/linux/usb/tcpm.h                           |   2 +
 include/linux/usb/typec.h                          |   3 +
 17 files changed, 1012 insertions(+), 787 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/typec-tcpci.txt
 delete mode 100644 drivers/staging/typec/Kconfig
 delete mode 100644 drivers/staging/typec/Makefile
 delete mode 100644 drivers/staging/typec/TODO
 delete mode 100644 drivers/staging/typec/tcpci.c
 delete mode 100644 drivers/staging/typec/tcpci.h
 create mode 100644 drivers/usb/typec/tcpci.c
 create mode 100644 drivers/usb/typec/tcpci.h

Comments

Dan Carpenter March 29, 2018, 10:52 a.m. UTC | #1 
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.chen@nxp.com>
> Signed-off-by: Li Jun <jun.li@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?  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, &val,
>  			       sizeof(u16));
>  	if (err < 0)
>  		return err;
>  
> +	chip->tcpci = tcpci_register_port(&client->dev, &chip->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.



> +
>  	err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>  					_tcpci_irq,
>  					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>  					dev_name(&client->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;


regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Heikki Krogerus March 29, 2018, 12:57 p.m. UTC | #2 
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.krogerus@linux.intel.com>
> Signed-off-by: Li Jun <jun.li@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 <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
> +#include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/usb/pd.h>
>  #include <linux/usb/tcpm.h>
> @@ -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 = &tcpci_tcpc_config;

That will break bisectablitity. tcpm.c is still accessing the config
at this point.

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


Thanks,
Mats Karrman March 29, 2018, 9:18 p.m. UTC | #3 
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.li@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"?|

>  }
>  
>  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


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck March 29, 2018, 10:49 p.m. UTC | #4 
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.

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

Guenter

> Signed-off-by: Li Jun <jun.li@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
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck March 30, 2018, 3:15 p.m. UTC | #5 
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 ?

Guenter

> Signed-off-by: Li Jun <jun.li@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, &reg);
>   	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)
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jun Li March 31, 2018, 3:17 a.m. UTC | #6 
Hi
> -----Original Message-----
> From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com]
> Sent: 2018年3月29日 20:58
> To: Jun Li <jun.li@nxp.com>
> Cc: robh+dt@kernel.org; gregkh@linuxfoundation.org; linux@roeck-us.net;
> a.hajda@samsung.com; shufan_lee@richtek.com; Peter Chen
> <peter.chen@nxp.com>; devicetree@vger.kernel.org;
> linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> devel@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.krogerus@linux.intel.com>
> > Signed-off-by: Li Jun <jun.li@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 <linux/module.h>
> >  #include <linux/i2c.h>
> >  #include <linux/interrupt.h>
> > +#include <linux/property.h>
> >  #include <linux/regmap.h>
> >  #include <linux/usb/pd.h>
> >  #include <linux/usb/tcpm.h>
> > @@ -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 = &tcpci_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
Jun Li March 31, 2018, 3:37 a.m. UTC | #7 
Hi
> -----Original Message-----
> From: Mats Karrman [mailto:mats.dev.list@gmail.com]
> Sent: 2018年3月30日 5:19
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; gregkh@linuxfoundation.org;
> heikki.krogerus@linux.intel.com; linux@roeck-us.net
> Cc: a.hajda@samsung.com; shufan_lee@richtek.com; Peter Chen
> <peter.chen@nxp.com>; devicetree@vger.kernel.org;
> linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> devel@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.li@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
>
Jun Li March 31, 2018, 4:49 a.m. UTC | #8 
Hi
> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter Roeck
> Sent: 2018年3月30日 23:16
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; gregkh@linuxfoundation.org;
> heikki.krogerus@linux.intel.com
> Cc: a.hajda@samsung.com; shufan_lee@richtek.com; Peter Chen
> <peter.chen@nxp.com>; devicetree@vger.kernel.org;
> linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> devel@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.li@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, &reg);
> >   	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)
> >
Dan Carpenter March 31, 2018, 8:01 a.m. UTC | #9 
On Sat, Mar 31, 2018 at 03:09:44AM +0000, Jun Li wrote:
> 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.

Fair enough.  That's reasonable.  Thanks!

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html