Message ID | 1522253178-32414-1-git-send-email-jun.li@nxp.com |
---|---|
Headers | show |
Series | staging: typec: tcpci: move out of staging | expand |
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
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,
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
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
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, ®); > 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
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
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 >
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, ®); > > 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) > >
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