| Message ID | 1506386727-16370-1-git-send-email-jun.li@nxp.com |
|---|---|
| Headers | show |
| Series | staging: typec: tcpci: move out of staging | expand |
On Tue, Sep 26, 2017 at 08:45:27AM +0800, Li Jun wrote: > Move TCPCI(Typec port controller interface) driver out of staging. > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > 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 | 637 ----------------------------------------- > drivers/staging/typec/tcpci.h | 133 --------- > drivers/usb/typec/Kconfig | 7 + > drivers/usb/typec/Makefile | 1 + > drivers/usb/typec/tcpci.c | 637 +++++++++++++++++++++++++++++++++++++++++ > drivers/usb/typec/tcpci.h | 133 +++++++++ > 11 files changed, 778 insertions(+), 793 deletions(-) Creating patches like this with "-M" to git format-patch is nice, that way we see the rename as just a rename, and not a big delete/add patch. Can you do that instead and resend this series? thanks, greg k-h -- 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: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Tuesday, September 26, 2017 2:58 PM > To: Jun Li <jun.li@nxp.com> > Cc: linux@roeck-us.net; robh+dt@kernel.org; mark.rutland@arm.com; > heikki.krogerus@linux.intel.com; yueyao@google.com; o_leveque@orange.fr; > Peter Chen <peter.chen@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>; linux- > usb@vger.kernel.org; devicetree@vger.kernel.org > Subject: Re: [PATCH 12/12] staging: typec: tcpci: move tcpci driver out of > staging > > On Tue, Sep 26, 2017 at 08:45:27AM +0800, Li Jun wrote: > > Move TCPCI(Typec port controller interface) driver out of staging. > > > > Signed-off-by: Li Jun <jun.li@nxp.com> > > --- > > 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 | 637 > > ----------------------------------------- > > drivers/staging/typec/tcpci.h | 133 --------- > > drivers/usb/typec/Kconfig | 7 + > > drivers/usb/typec/Makefile | 1 + > > drivers/usb/typec/tcpci.c | 637 > +++++++++++++++++++++++++++++++++++++++++ > > drivers/usb/typec/tcpci.h | 133 +++++++++ > > 11 files changed, 778 insertions(+), 793 deletions(-) > > > Creating patches like this with "-M" to git format-patch is nice, that way we see > the rename as just a rename, and not a big delete/add patch. > > Can you do that instead and resend this series? Thanks, I will do that and resend. Li Jun > > thanks, > > greg k-h -- 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 09/25/2017 05:45 PM, Li Jun wrote: > As we should only drive connected cc line to be the right state when > attached, and keeps the other one to be open, so update the set_cc > interface for that. > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/usb/typec/tcpm.c | 12 +++++++++++- > include/linux/usb/tcpm.h | 3 ++- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c > index 38a6223..c9966ee 100644 > --- a/drivers/usb/typec/tcpm.c > +++ b/drivers/usb/typec/tcpm.c > @@ -1857,9 +1857,15 @@ static bool tcpm_start_drp_toggling(struct tcpm_port *port, > > static void tcpm_set_cc(struct tcpm_port *port, enum typec_cc_status cc) > { > + bool attached; > + > tcpm_log(port, "cc:=%d", cc); > port->cc_req = cc; > - port->tcpc->set_cc(port->tcpc, cc); > + if (port->state == SRC_ATTACHED || port->state == SNK_ATTACHED) > + attached = true; > + else > + attached = false; > + port->tcpc->set_cc(port->tcpc, cc, attached, port->polarity); Polarity should be set with set_polarity(). Is it necessary to duplicate that ? > } > > static int tcpm_init_vbus(struct tcpm_port *port) > @@ -1913,6 +1919,8 @@ static int tcpm_src_attach(struct tcpm_port *port) > if (ret < 0) > return ret; > > + tcpm_set_cc(port, tcpm_rp_cc(port)); > + > ret = tcpm_set_roles(port, true, TYPEC_SOURCE, TYPEC_HOST); > if (ret < 0) > return ret; > @@ -2028,6 +2036,8 @@ static int tcpm_snk_attach(struct tcpm_port *port) > if (ret < 0) > return ret; > > + tcpm_set_cc(port, TYPEC_CC_RD); > + > ret = tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE); > if (ret < 0) > return ret; > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h > index a67cf77..a007e2a1 100644 > --- a/include/linux/usb/tcpm.h > +++ b/include/linux/usb/tcpm.h > @@ -159,7 +159,8 @@ struct tcpc_dev { > int (*init)(struct tcpc_dev *dev); > int (*get_vbus)(struct tcpc_dev *dev); > int (*get_current_limit)(struct tcpc_dev *dev); > - int (*set_cc)(struct tcpc_dev *dev, enum typec_cc_status cc); > + int (*set_cc)(struct tcpc_dev *dev, enum typec_cc_status cc, > + bool attached, enum typec_cc_polarity polarity); > int (*get_cc)(struct tcpc_dev *dev, enum typec_cc_status *cc1, > enum typec_cc_status *cc2); > int (*set_polarity)(struct tcpc_dev *dev, > -- 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 Tue, Sep 26, 2017 at 08:45:16AM +0800, Li Jun wrote: > This patch add 2 APIs to get port type and preferred role from firmware > description. > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/usb/typec/typec.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/usb/typec.h | 2 ++ > 2 files changed, 47 insertions(+) > > diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c > index 24e355b..0c77cc4 100644 > --- a/drivers/usb/typec/typec.c > +++ b/drivers/usb/typec/typec.c > @@ -12,6 +12,7 @@ > #include <linux/device.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/of.h> Not needed. > #include <linux/slab.h> > #include <linux/usb/typec.h> > > @@ -1249,6 +1250,50 @@ void typec_set_pwr_opmode(struct typec_port *port, > } > EXPORT_SYMBOL_GPL(typec_set_pwr_opmode); > > +/** > + * typec_get_port_type - Get the typec port type > + * @dev: Device to get the property of > + * > + * This routine is used by typec hardware driver to read property port type > + * from the device firmware description. > + * > + * Returns typec_port_type if success, otherwise negative error code. > + */ > +int typec_get_port_type(struct device *dev) > +{ > + const char *type_str; > + int ret; > + > + ret = device_property_read_string(dev, "port-type", &type_str); > + if (ret < 0) > + return ret; > + > + return match_string(typec_port_types, ARRAY_SIZE(typec_port_types), > + type_str); > +} > +EXPORT_SYMBOL_GPL(typec_get_port_type); > + > +/** > + * typec_get_power_role - Get the typec preferred role > + * @dev: Device to get the property of > + * > + * This routine is used by typec hardware driver to read property default role > + * from the device firmware description. > + * > + * Returns typec_role if success, otherwise negative error code. > + */ > +int typec_get_power_role(struct device *dev) > +{ > + const char *power_str; > + int ret; > + > + ret = device_property_read_string(dev, "default-role", &power_str); > + if (ret < 0) > + return ret; > + > + return match_string(typec_roles, ARRAY_SIZE(typec_roles), power_str); > +} > +EXPORT_SYMBOL_GPL(typec_get_power_role); > /* --------------------------------------- */ > > /** > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h > index ffe7487..bfac685 100644 > --- a/include/linux/usb/typec.h > +++ b/include/linux/usb/typec.h > @@ -243,5 +243,7 @@ void typec_set_data_role(struct typec_port *port, enum typec_data_role role); > void typec_set_pwr_role(struct typec_port *port, enum typec_role role); > void typec_set_vconn_role(struct typec_port *port, enum typec_role role); > void typec_set_pwr_opmode(struct typec_port *port, enum typec_pwr_opmode mode); > +int typec_get_port_type(struct device *dev); > +int typec_get_power_role(struct device *dev); > > #endif /* __LINUX_USB_TYPEC_H */ > -- > 2.6.6 Thanks,
> -----Original Message----- > From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com] > Sent: Tuesday, September 26, 2017 4:03 PM > To: Jun Li <jun.li@nxp.com> > Cc: gregkh@linuxfoundation.org; linux@roeck-us.net; robh+dt@kernel.org; > mark.rutland@arm.com; yueyao@google.com; o_leveque@orange.fr; Peter > Chen <peter.chen@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>; linux- > usb@vger.kernel.org; devicetree@vger.kernel.org > Subject: Re: [PATCH 01/12] usb: typec: add API to get port type and preferred > role > > Hi, > > On Tue, Sep 26, 2017 at 08:45:16AM +0800, Li Jun wrote: > > This patch add 2 APIs to get port type and preferred role from > > firmware description. > > > > Signed-off-by: Li Jun <jun.li@nxp.com> > > --- > > drivers/usb/typec/typec.c | 45 > > +++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/usb/typec.h | 2 ++ > > 2 files changed, 47 insertions(+) > > > > diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c > > index 24e355b..0c77cc4 100644 > > --- a/drivers/usb/typec/typec.c > > +++ b/drivers/usb/typec/typec.c > > @@ -12,6 +12,7 @@ > > #include <linux/device.h> > > #include <linux/module.h> > > #include <linux/mutex.h> > > +#include <linux/of.h> > > Not needed. I will change to #include <linux/property.h> > > > #include <linux/slab.h> > > #include <linux/usb/typec.h> > > > > @@ -1249,6 +1250,50 @@ void typec_set_pwr_opmode(struct typec_port > > *port, } EXPORT_SYMBOL_GPL(typec_set_pwr_opmode); > > > > +/** > > + * typec_get_port_type - Get the typec port type > > + * @dev: Device to get the property of > > + * > > + * This routine is used by typec hardware driver to read property > > +port type > > + * from the device firmware description. > > + * > > + * Returns typec_port_type if success, otherwise negative error code. > > + */ > > +int typec_get_port_type(struct device *dev) { > > + const char *type_str; > > + int ret; > > + > > + ret = device_property_read_string(dev, "port-type", &type_str); > > + if (ret < 0) > > + return ret; > > + > > + return match_string(typec_port_types, ARRAY_SIZE(typec_port_types), > > + type_str); > > +} > > +EXPORT_SYMBOL_GPL(typec_get_port_type); > > + > > +/** > > + * typec_get_power_role - Get the typec preferred role > > + * @dev: Device to get the property of > > + * > > + * This routine is used by typec hardware driver to read property > > +default role > > + * from the device firmware description. > > + * > > + * Returns typec_role if success, otherwise negative error code. > > + */ > > +int typec_get_power_role(struct device *dev) { > > + const char *power_str; > > + int ret; > > + > > + ret = device_property_read_string(dev, "default-role", &power_str); > > + if (ret < 0) > > + return ret; > > + > > + return match_string(typec_roles, ARRAY_SIZE(typec_roles), > > +power_str); } EXPORT_SYMBOL_GPL(typec_get_power_role); > > /* --------------------------------------- */ > > > > /** > > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h > > index ffe7487..bfac685 100644 > > --- a/include/linux/usb/typec.h > > +++ b/include/linux/usb/typec.h > > @@ -243,5 +243,7 @@ void typec_set_data_role(struct typec_port *port, > > enum typec_data_role role); void typec_set_pwr_role(struct typec_port > > *port, enum typec_role role); void typec_set_vconn_role(struct > > typec_port *port, enum typec_role role); void > > typec_set_pwr_opmode(struct typec_port *port, enum typec_pwr_opmode > > mode); > > +int typec_get_port_type(struct device *dev); int > > +typec_get_power_role(struct device *dev); > > > > #endif /* __LINUX_USB_TYPEC_H */ > > -- > > 2.6.6 > > Thanks, > > -- > heikki -- 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 09/26/2017 02:53 AM, Jun Li wrote: > Hi, > >> -----Original Message----- >> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter Roeck >> Sent: Tuesday, September 26, 2017 3:17 PM >> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; robh+dt@kernel.org; >> mark.rutland@arm.com; heikki.krogerus@linux.intel.com >> Cc: yueyao@google.com; o_leveque@orange.fr; Peter Chen >> <peter.chen@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>; linux- >> usb@vger.kernel.org; devicetree@vger.kernel.org >> Subject: Re: [PATCH 09/12] usb: typec: tcpm: only drives the connected cc line >> when attached >> >> On 09/25/2017 05:45 PM, Li Jun wrote: >>> As we should only drive connected cc line to be the right state when >>> attached, and keeps the other one to be open, so update the set_cc >>> interface for that. >>> >>> Signed-off-by: Li Jun <jun.li@nxp.com> >>> --- >>> drivers/usb/typec/tcpm.c | 12 +++++++++++- >>> include/linux/usb/tcpm.h | 3 ++- >>> 2 files changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index >>> 38a6223..c9966ee 100644 >>> --- a/drivers/usb/typec/tcpm.c >>> +++ b/drivers/usb/typec/tcpm.c >>> @@ -1857,9 +1857,15 @@ static bool tcpm_start_drp_toggling(struct >>> tcpm_port *port, >>> >>> static void tcpm_set_cc(struct tcpm_port *port, enum typec_cc_status cc) >>> { >>> + bool attached; >>> + >>> tcpm_log(port, "cc:=%d", cc); >>> port->cc_req = cc; >>> - port->tcpc->set_cc(port->tcpc, cc); >>> + if (port->state == SRC_ATTACHED || port->state == SNK_ATTACHED) >>> + attached = true; >>> + else >>> + attached = false; >>> + port->tcpc->set_cc(port->tcpc, cc, attached, port->polarity); >> >> Polarity should be set with set_polarity(). Is it necessary to duplicate that ? > > In tcpci case, set_polarity only sets Plug Orientation bit: > " 0b: When Vconn is enabled, apply it to the CC2 pin. Monitor the CC1 pin for > BMC communications if PD messaging is enabled. > 1b: When Vconn is enabled, apply it to the CC1 pin. Monitor the CC2 pin for > BMC communications if PD messaging is enabled." > Yes, but the driver should know about the flag already. I have two more concerns: Setting CC is logically independent from the state machine "active" state. I don't recall that the USB PD state machine talks about CC pin changes upon entering and exiting READY states. Why is it necessary to pass the state to the driver ? Also, your patch changes the API, but you don't change the driver, meaning there should be compile failures after this patch. You need to change the calling code in the same patch to keep the build clean. Guenter > There is no duplication for tcpci, or you think I should do this in set_polarity > of tcpci driver internally? looks better from my point view as I may don't > need change tcpm set_cc interface. > > thanks > Li Jun >> >>> } >>> >>> static int tcpm_init_vbus(struct tcpm_port *port) @@ -1913,6 +1919,8 >>> @@ static int tcpm_src_attach(struct tcpm_port *port) >>> if (ret < 0) >>> return ret; >>> >>> + tcpm_set_cc(port, tcpm_rp_cc(port)); >>> + >>> ret = tcpm_set_roles(port, true, TYPEC_SOURCE, TYPEC_HOST); >>> if (ret < 0) >>> return ret; >>> @@ -2028,6 +2036,8 @@ static int tcpm_snk_attach(struct tcpm_port *port) >>> if (ret < 0) >>> return ret; >>> >>> + tcpm_set_cc(port, TYPEC_CC_RD); >>> + >>> ret = tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE); >>> if (ret < 0) >>> return ret; >>> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index >>> a67cf77..a007e2a1 100644 >>> --- a/include/linux/usb/tcpm.h >>> +++ b/include/linux/usb/tcpm.h >>> @@ -159,7 +159,8 @@ struct tcpc_dev { >>> int (*init)(struct tcpc_dev *dev); >>> int (*get_vbus)(struct tcpc_dev *dev); >>> int (*get_current_limit)(struct tcpc_dev *dev); >>> - int (*set_cc)(struct tcpc_dev *dev, enum typec_cc_status cc); >>> + int (*set_cc)(struct tcpc_dev *dev, enum typec_cc_status cc, >>> + bool attached, enum typec_cc_polarity polarity); >>> int (*get_cc)(struct tcpc_dev *dev, enum typec_cc_status *cc1, >>> enum typec_cc_status *cc2); >>> int (*set_polarity)(struct tcpc_dev *dev, >>> > -- 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 09/25/2017 05:45 PM, Li Jun wrote: > This patch add 2 APIs to get port type and preferred role from firmware > description. > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/usb/typec/typec.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/usb/typec.h | 2 ++ > 2 files changed, 47 insertions(+) > > diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c > index 24e355b..0c77cc4 100644 > --- a/drivers/usb/typec/typec.c > +++ b/drivers/usb/typec/typec.c > @@ -12,6 +12,7 @@ > #include <linux/device.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/of.h> > #include <linux/slab.h> > #include <linux/usb/typec.h> > > @@ -1249,6 +1250,50 @@ void typec_set_pwr_opmode(struct typec_port *port, > } > EXPORT_SYMBOL_GPL(typec_set_pwr_opmode); > > +/** > + * typec_get_port_type - Get the typec port type > + * @dev: Device to get the property of > + * > + * This routine is used by typec hardware driver to read property port type > + * from the device firmware description. > + * > + * Returns typec_port_type if success, otherwise negative error code. > + */ > +int typec_get_port_type(struct device *dev) > +{ > + const char *type_str; > + int ret; > + > + ret = device_property_read_string(dev, "port-type", &type_str); > + if (ret < 0) > + return ret; > + > + return match_string(typec_port_types, ARRAY_SIZE(typec_port_types), > + type_str); > +} > +EXPORT_SYMBOL_GPL(typec_get_port_type); > + > +/** > + * typec_get_power_role - Get the typec preferred role > + * @dev: Device to get the property of > + * > + * This routine is used by typec hardware driver to read property default role > + * from the device firmware description. > + * > + * Returns typec_role if success, otherwise negative error code. > + */ > +int typec_get_power_role(struct device *dev) > +{ > + const char *power_str; > + int ret; > + > + ret = device_property_read_string(dev, "default-role", &power_str); > + if (ret < 0) > + return ret; > + > + return match_string(typec_roles, ARRAY_SIZE(typec_roles), power_str); > +} > +EXPORT_SYMBOL_GPL(typec_get_power_role); Add empty line > /* --------------------------------------- */ > > /** > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h > index ffe7487..bfac685 100644 > --- a/include/linux/usb/typec.h > +++ b/include/linux/usb/typec.h > @@ -243,5 +243,7 @@ void typec_set_data_role(struct typec_port *port, enum typec_data_role role); > void typec_set_pwr_role(struct typec_port *port, enum typec_role role); > void typec_set_vconn_role(struct typec_port *port, enum typec_role role); > void typec_set_pwr_opmode(struct typec_port *port, enum typec_pwr_opmode mode); > +int typec_get_port_type(struct device *dev); > +int typec_get_power_role(struct device *dev); > > #endif /* __LINUX_USB_TYPEC_H */ > -- 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 09/25/2017 05:45 PM, Li Jun wrote: > User can define the typec port properties in tcpci node to setup > the port config. > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/staging/typec/tcpci.c | 89 +++++++++++++++++++++++++++++++++++++++---- > include/linux/usb/tcpm.h | 6 +-- > 2 files changed, 85 insertions(+), 10 deletions(-) > > diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c > index 4636804..0119453 100644 > --- a/drivers/staging/typec/tcpci.c > +++ b/drivers/staging/typec/tcpci.c > @@ -425,17 +425,92 @@ 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, > -}; > - > +/* Populate struct tcpc_config from device-tree */ > static int tcpci_parse_config(struct tcpci *tcpci) > { > + struct tcpc_config *tcfg; > + int ret = -EINVAL; > + > tcpci->controls_vbus = true; /* XXX */ > > - /* TODO: Populate struct tcpc_config from ACPI/device-tree */ > - tcpci->tcpc.config = &tcpci_tcpc_config; > + tcpci->tcpc.config = devm_kzalloc(tcpci->dev, sizeof(*tcfg), > + GFP_KERNEL); > + if (!tcpci->tcpc.config) > + return -ENOMEM; > + > + tcfg = tcpci->tcpc.config; > + > + /* Get port-type */ > + ret = typec_get_port_type(tcpci->dev); > + if (ret < 0) { > + dev_err(tcpci->dev, "typec port type is NOT correct!\n"); Are all those exclamation marks necessary ? > + return ret; > + } > + tcfg->type = ret; > + > + if (tcfg->type == TYPEC_PORT_UFP) > + goto sink; > + > + /* Get source PDO */ > + tcfg->nr_src_pdo = device_property_read_u32_array(tcpci->dev, > + "src-pdos", NULL, 0); I personally prefer continuation line alignment to '(', but that is up to Greg to decide. > + if (tcfg->nr_src_pdo <= 0) { > + dev_err(tcpci->dev, "typec source pdo is missing!\n"); > + return -EINVAL; > + } > + > + tcfg->src_pdo = devm_kzalloc(tcpci->dev, > + sizeof(*tcfg->src_pdo) * tcfg->nr_src_pdo, GFP_KERNEL); devm_kcalloc > + if (!tcfg->src_pdo) > + return -ENOMEM; > + > + ret = device_property_read_u32_array(tcpci->dev, "src-pdos", > + tcfg->src_pdo, tcfg->nr_src_pdo); > + if (ret) { > + dev_err(tcpci->dev, "Failed to read src pdo!\n"); > + return -EINVAL; > + } > + > + if (tcfg->type == TYPEC_PORT_DFP) > + return 0; > + > + /* Get the preferred power role for drp */ > + ret = typec_get_power_role(tcpci->dev); Maybe this should be named typec_get_preferred_role(). The generic function names are a bit misleading; they suggest they would return the current role, and don't indicate that the data is from device properties. I am also not sure about the mix of using typec infra functions and direct device_property calls. > + if (ret < 0) { > + dev_err(tcpci->dev, "typec preferred role is wrong!\n"); Wrong or missing ? > + return ret; > + } > + tcfg->default_role = ret; > +sink: > + /* Get sink power capability */ > + tcfg->nr_snk_pdo = device_property_read_u32_array(tcpci->dev, > + "snk-pdos", NULL, 0); > + if (tcfg->nr_snk_pdo <= 0) { > + dev_err(tcpci->dev, "typec sink pdo is missing!\n"); > + return -EINVAL; > + } > + > + tcfg->snk_pdo = devm_kzalloc(tcpci->dev, > + sizeof(*tcfg->snk_pdo) * tcfg->nr_snk_pdo, GFP_KERNEL); > + if (!tcfg->snk_pdo) > + return -ENOMEM; > + > + ret = device_property_read_u32_array(tcpci->dev, "snk-pdos", > + tcfg->snk_pdo, tcfg->nr_snk_pdo); > + if (ret) { > + dev_err(tcpci->dev, "Failed to read sink pdo!\n"); There is a mix of "Failed to read" and "missing" messages. What is the difference ? > + return -EINVAL; > + } > + > + if (device_property_read_u32(tcpci->dev, "max-snk-mv", > + &tcfg->max_snk_mv) || > + device_property_read_u32(tcpci->dev, "max-snk-ma", > + &tcfg->max_snk_ma) || > + device_property_read_u32(tcpci->dev, "op-snk-mw", > + &tcfg->operating_snk_mw)) { > + dev_err(tcpci->dev, "Failed to read sink capability!\n"); > + return -EINVAL; > + } > > return 0; > } > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h > index 073197f..a67cf77 100644 > --- a/include/linux/usb/tcpm.h > +++ b/include/linux/usb/tcpm.h > @@ -76,10 +76,10 @@ enum tcpm_transmit_type { > * @alt_modes: List of supported alternate modes > */ > struct tcpc_config { > - const u32 *src_pdo; > + u32 *src_pdo; > unsigned int nr_src_pdo; > > - const u32 *snk_pdo; > + u32 *snk_pdo; > unsigned int nr_snk_pdo; > > const u32 *snk_vdo; > @@ -154,7 +154,7 @@ struct tcpc_mux_dev { > * @mux: Pointer to multiplexer data > */ > struct tcpc_dev { > - const struct tcpc_config *config; > + struct tcpc_config *config; > > int (*init)(struct tcpc_dev *dev); > int (*get_vbus)(struct tcpc_dev *dev); > -- 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 09/25/2017 05:45 PM, 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> > --- > drivers/staging/typec/tcpci.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c > index 0119453..6d608b4 100644 > --- a/drivers/staging/typec/tcpci.c > +++ b/drivers/staging/typec/tcpci.c > @@ -552,15 +552,14 @@ static int tcpci_probe(struct i2c_client *client, > /* Disable chip interrupts */ > tcpci_write16(tcpci, TCPC_ALERT_MASK, 0); > > - err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL, > + tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc); > + if (IS_ERR(tcpci->port)) > + return PTR_ERR(tcpci->port); > + > + return devm_request_threaded_irq(tcpci->dev, client->irq, NULL, > tcpci_irq, > IRQF_ONESHOT | IRQF_TRIGGER_LOW, > dev_name(tcpci->dev), tcpci); > - if (err < 0) > - return err; > - > - tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc); > - return PTR_ERR_OR_ZERO(tcpci->port); This leaves the port registered if registering the irq fails. > } > > static int tcpci_remove(struct i2c_client *client) > -- 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 09/25/2017 05:45 PM, Li Jun wrote: > TCPCI implementation may need SW to enable VBUS detection to generate > power status events. > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/staging/typec/tcpci.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c > index 6d608b4..851d026 100644 > --- a/drivers/staging/typec/tcpci.c > +++ b/drivers/staging/typec/tcpci.c > @@ -313,6 +313,26 @@ static int tcpci_pd_transmit(struct tcpc_dev *tcpc, > return 0; > } > > +static int tcpci_vbus_detect(struct tcpc_dev *tcpc, bool enable) > +{ > + struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > + int ret; > + > + if (enable) { > + ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > + TCPC_CMD_ENABLE_VBUS_DETECT); > + if (ret < 0) > + return ret; > + } else { > + ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > + TCPC_CMD_DISABLE_VBUS_DETECT); > + if (ret < 0) > + return ret; > + } This could be simplified to something like u8 cmd = enabled ? TCPC_CMD_ENABLE_VBUS_DETECT : TCPC_CMD_DISABLE_VBUS_DETECT; return regmap_write(tcpci->regmap, TCPC_COMMAND, cmd); though the question is why not just add a function named tcpci_vbus_detect_enable() since it is never disabled, at least not in this patch. > + > + return 0; > +} > + > static int tcpci_init(struct tcpc_dev *tcpc) > { > struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > @@ -344,6 +364,9 @@ static int tcpci_init(struct tcpc_dev *tcpc) > if (ret < 0) > return ret; > > + /* Enable Vbus detection */ > + tcpci_vbus_detect(tcpc, true); > + > reg = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_FAILED | > TCPC_ALERT_TX_DISCARDED | TCPC_ALERT_RX_STATUS | > TCPC_ALERT_RX_HARD_RST | TCPC_ALERT_CC_STATUS; > -- 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 09/25/2017 05:45 PM, Li Jun wrote: > As DRP port autonomously toggles the Rp/Rd need a start value to > begin with, so add one parameter for it in tcpm_start_drp_toggling. > It does have a starting value. The patch changes the starting value to TYPEC_CC_RD (from currently one of the RP states) when entering the SNK_UNATTACHED state. Please provide a matching description. > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/usb/typec/tcpm.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c > index 8483d3e..38a6223 100644 > --- a/drivers/usb/typec/tcpm.c > +++ b/drivers/usb/typec/tcpm.c > @@ -1839,15 +1839,15 @@ static int tcpm_set_charge(struct tcpm_port *port, bool charge) > return 0; > } > > -static bool tcpm_start_drp_toggling(struct tcpm_port *port) > +static bool tcpm_start_drp_toggling(struct tcpm_port *port, > + enum typec_cc_status cc) > { > int ret; > > if (port->tcpc->start_drp_toggling && > port->port_type == TYPEC_PORT_DRP) { > tcpm_log_force(port, "Start DRP toggling"); > - ret = port->tcpc->start_drp_toggling(port->tcpc, > - tcpm_rp_cc(port)); > + ret = port->tcpc->start_drp_toggling(port->tcpc, cc); > if (!ret) > return true; > } > @@ -2156,7 +2156,7 @@ static void run_state_machine(struct tcpm_port *port) > if (!port->non_pd_role_swap) > tcpm_swap_complete(port, -ENOTCONN); > tcpm_src_detach(port); > - if (tcpm_start_drp_toggling(port)) { > + if (tcpm_start_drp_toggling(port, tcpm_rp_cc(port))) { > tcpm_set_state(port, DRP_TOGGLING, 0); > break; > } > @@ -2328,7 +2328,7 @@ static void run_state_machine(struct tcpm_port *port) > if (!port->non_pd_role_swap) > tcpm_swap_complete(port, -ENOTCONN); > tcpm_snk_detach(port); > - if (tcpm_start_drp_toggling(port)) { > + if (tcpm_start_drp_toggling(port, TYPEC_CC_RD)) { > tcpm_set_state(port, DRP_TOGGLING, 0); > break; > } > -- 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 09/25/2017 05:45 PM, Li Jun wrote: > As we should keep the disconnected cc line to be open when attached, > so update the set_cc interface accordingly for it. > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/staging/typec/tcpci.c | 37 +++++++++++++++++++++---------------- > 1 file changed, 21 insertions(+), 16 deletions(-) > > diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c > index cea67f9..c7c45da 100644 > --- a/drivers/staging/typec/tcpci.c > +++ b/drivers/staging/typec/tcpci.c > @@ -57,10 +57,11 @@ static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val) > return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16)); > } > > -static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc) > +static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc, > + bool attached, enum typec_cc_polarity polarity) > { > struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > - unsigned int reg; > + unsigned int reg = 0, reg_cc1 = 0, reg_cc2 = 0; > int ret; > > switch (cc) { > @@ -69,26 +70,23 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc) > (TCPC_ROLE_CTRL_CC_RA << TCPC_ROLE_CTRL_CC2_SHIFT); > break; > case TYPEC_CC_RD: > - reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) | > - (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT); > + reg_cc1 = TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT; > + reg_cc2 = TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT; > break; > case TYPEC_CC_RP_DEF: > - reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) | > - (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT) | > - (TCPC_ROLE_CTRL_RP_VAL_DEF << > - TCPC_ROLE_CTRL_RP_VAL_SHIFT); > + reg_cc1 = TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT; > + reg_cc2 = TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT; > + reg = TCPC_ROLE_CTRL_RP_VAL_DEF << TCPC_ROLE_CTRL_RP_VAL_SHIFT; > break; > case TYPEC_CC_RP_1_5: > - reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) | > - (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT) | > - (TCPC_ROLE_CTRL_RP_VAL_1_5 << > - TCPC_ROLE_CTRL_RP_VAL_SHIFT); > + reg_cc1 = TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT; > + reg_cc2 = TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT; > + reg = TCPC_ROLE_CTRL_RP_VAL_1_5 << TCPC_ROLE_CTRL_RP_VAL_SHIFT; > break; > case TYPEC_CC_RP_3_0: > - reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) | > - (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT) | > - (TCPC_ROLE_CTRL_RP_VAL_3_0 << > - TCPC_ROLE_CTRL_RP_VAL_SHIFT); > + reg_cc1 = TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT; > + reg_cc2 = TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT; > + reg = TCPC_ROLE_CTRL_RP_VAL_3_0 << TCPC_ROLE_CTRL_RP_VAL_SHIFT; > break; > case TYPEC_CC_OPEN: > default: > @@ -97,6 +95,13 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc) > break; > } > > + if (!attached) > + reg |= reg_cc1 | reg_cc2; > + else if (polarity == TYPEC_POLARITY_CC1) > + reg |= reg_cc1; > + else > + reg |= reg_cc2; > + I think this is wrong. The value of CC pins should not depend on the state of the state machine. What if the state machine knows the polarity but is in a transient state ?. I am also not sure if the chip should ever set both CC lines at the same time. Guenter > ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg); > if (ret < 0) > return ret; > -- 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
> -----Original Message----- > From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter Roeck > Sent: Tuesday, September 26, 2017 9:20 PM > To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; robh+dt@kernel.org; > mark.rutland@arm.com; heikki.krogerus@linux.intel.com > Cc: yueyao@google.com; o_leveque@orange.fr; Peter Chen > <peter.chen@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>; linux- > usb@vger.kernel.org; devicetree@vger.kernel.org > Subject: Re: [PATCH 09/12] usb: typec: tcpm: only drives the connected cc line > when attached > > On 09/26/2017 02:53 AM, Jun Li wrote: > > Hi, > > > >> -----Original Message----- > >> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter > >> Roeck > >> Sent: Tuesday, September 26, 2017 3:17 PM > >> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; > >> robh+dt@kernel.org; mark.rutland@arm.com; > >> heikki.krogerus@linux.intel.com > >> Cc: yueyao@google.com; o_leveque@orange.fr; Peter Chen > >> <peter.chen@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>; linux- > >> usb@vger.kernel.org; devicetree@vger.kernel.org > >> Subject: Re: [PATCH 09/12] usb: typec: tcpm: only drives the > >> connected cc line when attached > >> > >> On 09/25/2017 05:45 PM, Li Jun wrote: > >>> As we should only drive connected cc line to be the right state when > >>> attached, and keeps the other one to be open, so update the set_cc > >>> interface for that. > >>> > >>> Signed-off-by: Li Jun <jun.li@nxp.com> > >>> --- > >>> drivers/usb/typec/tcpm.c | 12 +++++++++++- > >>> include/linux/usb/tcpm.h | 3 ++- > >>> 2 files changed, 13 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c > >>> index 38a6223..c9966ee 100644 > >>> --- a/drivers/usb/typec/tcpm.c > >>> +++ b/drivers/usb/typec/tcpm.c > >>> @@ -1857,9 +1857,15 @@ static bool tcpm_start_drp_toggling(struct > >>> tcpm_port *port, > >>> > >>> static void tcpm_set_cc(struct tcpm_port *port, enum typec_cc_status cc) > >>> { > >>> + bool attached; > >>> + > >>> tcpm_log(port, "cc:=%d", cc); > >>> port->cc_req = cc; > >>> - port->tcpc->set_cc(port->tcpc, cc); > >>> + if (port->state == SRC_ATTACHED || port->state == SNK_ATTACHED) > >>> + attached = true; > >>> + else > >>> + attached = false; > >>> + port->tcpc->set_cc(port->tcpc, cc, attached, port->polarity); > >> > >> Polarity should be set with set_polarity(). Is it necessary to duplicate that ? > > > > In tcpci case, set_polarity only sets Plug Orientation bit: > > " 0b: When Vconn is enabled, apply it to the CC2 pin. Monitor the CC1 > > pin for BMC communications if PD messaging is enabled. > > 1b: When Vconn is enabled, apply it to the CC1 pin. Monitor the CC2 > > pin for BMC communications if PD messaging is enabled." > > > Yes, but the driver should know about the flag already. > > I have two more concerns: > > Setting CC is logically independent from the state machine "active" state. > I don't recall that the USB PD state machine talks about CC pin changes upon > entering and exiting READY states. Why is it necessary to pass the state to the > driver ? Here the CC pin changes only for un-touched one between look for connection and attached, so when attach(before entering READY), the un-touch cc pin should be changed from Pd/Rp to open(e.g. see typec spec Table 4-6 Source Perspective). as my question below, I can adding it into tcpci_set_polarity() for tcpci case if it's reasonable, with that tcpm API don't need change. > > Also, your patch changes the API, but you don't change the driver, meaning > there should be compile failures after this patch. You need to change the calling > code in the same patch to keep the build clean. Sorry, I didn’t realize there is already a user of tcpm, I will update the calling driver accordingly for hanged tcpm API in next version. Li Jun > > Guenter > > > There is no duplication for tcpci, or you think I should do this in > > set_polarity of tcpci driver internally? looks better from my point > > view as I may don't need change tcpm set_cc interface. > > > > thanks > > Li Jun > >> > >>> } > >>> > >>> static int tcpm_init_vbus(struct tcpm_port *port) @@ -1913,6 > >>> +1919,8 @@ static int tcpm_src_attach(struct tcpm_port *port) > >>> if (ret < 0) > >>> return ret; > >>> > >>> + tcpm_set_cc(port, tcpm_rp_cc(port)); > >>> + > >>> ret = tcpm_set_roles(port, true, TYPEC_SOURCE, TYPEC_HOST); > >>> if (ret < 0) > >>> return ret; > >>> @@ -2028,6 +2036,8 @@ static int tcpm_snk_attach(struct tcpm_port > *port) > >>> if (ret < 0) > >>> return ret; > >>> > >>> + tcpm_set_cc(port, TYPEC_CC_RD); > >>> + > >>> ret = tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE); > >>> if (ret < 0) > >>> return ret; > >>> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h > >>> index > >>> a67cf77..a007e2a1 100644 > >>> --- a/include/linux/usb/tcpm.h > >>> +++ b/include/linux/usb/tcpm.h > >>> @@ -159,7 +159,8 @@ struct tcpc_dev { > >>> int (*init)(struct tcpc_dev *dev); > >>> int (*get_vbus)(struct tcpc_dev *dev); > >>> int (*get_current_limit)(struct tcpc_dev *dev); > >>> - int (*set_cc)(struct tcpc_dev *dev, enum typec_cc_status cc); > >>> + int (*set_cc)(struct tcpc_dev *dev, enum typec_cc_status cc, > >>> + bool attached, enum typec_cc_polarity polarity); > >>> int (*get_cc)(struct tcpc_dev *dev, enum typec_cc_status *cc1, > >>> enum typec_cc_status *cc2); > >>> int (*set_polarity)(struct tcpc_dev *dev, > >>> > >
Hi > -----Original Message----- > From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter Roeck > Sent: Tuesday, September 26, 2017 9:33 PM > To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; robh+dt@kernel.org; > mark.rutland@arm.com; heikki.krogerus@linux.intel.com > Cc: yueyao@google.com; o_leveque@orange.fr; Peter Chen > <peter.chen@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>; linux- > usb@vger.kernel.org; devicetree@vger.kernel.org > Subject: Re: [PATCH 04/12] staging: typec: tcpci: support port config passed via > dt > > On 09/25/2017 05:45 PM, Li Jun wrote: > > User can define the typec port properties in tcpci node to setup the > > port config. > > > > Signed-off-by: Li Jun <jun.li@nxp.com> > > --- > > drivers/staging/typec/tcpci.c | 89 > +++++++++++++++++++++++++++++++++++++++---- > > include/linux/usb/tcpm.h | 6 +-- > > 2 files changed, 85 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/staging/typec/tcpci.c > > b/drivers/staging/typec/tcpci.c index 4636804..0119453 100644 > > --- a/drivers/staging/typec/tcpci.c > > +++ b/drivers/staging/typec/tcpci.c > > @@ -425,17 +425,92 @@ 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, > > -}; > > - > > +/* Populate struct tcpc_config from device-tree */ > > static int tcpci_parse_config(struct tcpci *tcpci) > > { > > + struct tcpc_config *tcfg; > > + int ret = -EINVAL; > > + > > tcpci->controls_vbus = true; /* XXX */ > > > > - /* TODO: Populate struct tcpc_config from ACPI/device-tree */ > > - tcpci->tcpc.config = &tcpci_tcpc_config; > > + tcpci->tcpc.config = devm_kzalloc(tcpci->dev, sizeof(*tcfg), > > + GFP_KERNEL); > > + if (!tcpci->tcpc.config) > > + return -ENOMEM; > > + > > + tcfg = tcpci->tcpc.config; > > + > > + /* Get port-type */ > > + ret = typec_get_port_type(tcpci->dev); > > + if (ret < 0) { > > + dev_err(tcpci->dev, "typec port type is NOT correct!\n"); > > Are all those exclamation marks necessary ? I will remove it. > > > + return ret; > > + } > > + tcfg->type = ret; > > + > > + if (tcfg->type == TYPEC_PORT_UFP) > > + goto sink; > > + > > + /* Get source PDO */ > > + tcfg->nr_src_pdo = device_property_read_u32_array(tcpci->dev, > > + "src-pdos", NULL, 0); > > I personally prefer continuation line alignment to '(', but that is up to Greg to > decide. > > > + if (tcfg->nr_src_pdo <= 0) { > > + dev_err(tcpci->dev, "typec source pdo is missing!\n"); > > + return -EINVAL; > > + } > > + > > + tcfg->src_pdo = devm_kzalloc(tcpci->dev, > > + sizeof(*tcfg->src_pdo) * tcfg->nr_src_pdo, GFP_KERNEL); > > devm_kcalloc Will change. > > > + if (!tcfg->src_pdo) > > + return -ENOMEM; > > + > > + ret = device_property_read_u32_array(tcpci->dev, "src-pdos", > > + tcfg->src_pdo, tcfg->nr_src_pdo); > > + if (ret) { > > + dev_err(tcpci->dev, "Failed to read src pdo!\n"); > > + return -EINVAL; > > + } > > + > > + if (tcfg->type == TYPEC_PORT_DFP) > > + return 0; > > + > > + /* Get the preferred power role for drp */ > > + ret = typec_get_power_role(tcpci->dev); > > Maybe this should be named typec_get_preferred_role(). The generic function > names are a bit misleading; they suggest they would return the current role, and > don't indicate that the data is from device properties. Thanks, typec_get_preferred_role() looks more precise, I will change. > I am also not sure about the mix of using typec infra functions and direct > device_property calls. OK, I will try to also use typec infra functions for those device_property calls(some may be grouped). > > > + if (ret < 0) { > > + dev_err(tcpci->dev, "typec preferred role is wrong!\n"); > > Wrong or missing ? Both wrong and missing will return negative error code, I will refine it. > > > + return ret; > > + } > > + tcfg->default_role = ret; > > +sink: > > + /* Get sink power capability */ > > + tcfg->nr_snk_pdo = device_property_read_u32_array(tcpci->dev, > > + "snk-pdos", NULL, 0); > > + if (tcfg->nr_snk_pdo <= 0) { > > + dev_err(tcpci->dev, "typec sink pdo is missing!\n"); > > + return -EINVAL; > > + } > > + > > + tcfg->snk_pdo = devm_kzalloc(tcpci->dev, > > + sizeof(*tcfg->snk_pdo) * tcfg->nr_snk_pdo, GFP_KERNEL); > > + if (!tcfg->snk_pdo) > > + return -ENOMEM; > > + > > + ret = device_property_read_u32_array(tcpci->dev, "snk-pdos", > > + tcfg->snk_pdo, tcfg->nr_snk_pdo); > > + if (ret) { > > + dev_err(tcpci->dev, "Failed to read sink pdo!\n"); > > There is a mix of "Failed to read" and "missing" messages. What is the > difference ? I will refine the error message. > > > + return -EINVAL; > > + } > > + > > + if (device_property_read_u32(tcpci->dev, "max-snk-mv", > > + &tcfg->max_snk_mv) || > > + device_property_read_u32(tcpci->dev, "max-snk-ma", > > + &tcfg->max_snk_ma) || > > + device_property_read_u32(tcpci->dev, "op-snk-mw", > > + &tcfg->operating_snk_mw)) { > > + dev_err(tcpci->dev, "Failed to read sink capability!\n"); > > + return -EINVAL; > > + } > > > > return 0; > > } > > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index > > 073197f..a67cf77 100644 > > --- a/include/linux/usb/tcpm.h > > +++ b/include/linux/usb/tcpm.h > > @@ -76,10 +76,10 @@ enum tcpm_transmit_type { > > * @alt_modes: List of supported alternate modes > > */ > > struct tcpc_config { > > - const u32 *src_pdo; > > + u32 *src_pdo; > > unsigned int nr_src_pdo; > > > > - const u32 *snk_pdo; > > + u32 *snk_pdo; > > unsigned int nr_snk_pdo; > > > > const u32 *snk_vdo; > > @@ -154,7 +154,7 @@ struct tcpc_mux_dev { > > * @mux: Pointer to multiplexer data > > */ > > struct tcpc_dev { > > - const struct tcpc_config *config; > > + struct tcpc_config *config; > > > > int (*init)(struct tcpc_dev *dev); > > int (*get_vbus)(struct tcpc_dev *dev); > >
Hi > -----Original Message----- > From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter Roeck > Sent: Tuesday, September 26, 2017 9:34 PM > To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; robh+dt@kernel.org; > mark.rutland@arm.com; heikki.krogerus@linux.intel.com > Cc: yueyao@google.com; o_leveque@orange.fr; Peter Chen > <peter.chen@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>; linux- > usb@vger.kernel.org; devicetree@vger.kernel.org > Subject: Re: [PATCH 05/12] staging: typec: tcpci: register port before request irq > > On 09/25/2017 05:45 PM, 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> > > --- > > drivers/staging/typec/tcpci.c | 11 +++++------ > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/staging/typec/tcpci.c > > b/drivers/staging/typec/tcpci.c index 0119453..6d608b4 100644 > > --- a/drivers/staging/typec/tcpci.c > > +++ b/drivers/staging/typec/tcpci.c > > @@ -552,15 +552,14 @@ static int tcpci_probe(struct i2c_client *client, > > /* Disable chip interrupts */ > > tcpci_write16(tcpci, TCPC_ALERT_MASK, 0); > > > > - err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL, > > + tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc); > > + if (IS_ERR(tcpci->port)) > > + return PTR_ERR(tcpci->port); > > + > > + return devm_request_threaded_irq(tcpci->dev, client->irq, NULL, > > tcpci_irq, > > IRQF_ONESHOT | IRQF_TRIGGER_LOW, > > dev_name(tcpci->dev), tcpci); > > - if (err < 0) > > - return err; > > - > > - tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc); > > - return PTR_ERR_OR_ZERO(tcpci->port); > > This leaves the port registered if registering the irq fails. I will add an error handling with tcpm_unregister_port(). > > > } > > > > static int tcpci_remove(struct i2c_client *client) > >
Hi > -----Original Message----- > From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter Roeck > Sent: Tuesday, September 26, 2017 9:43 PM > To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; robh+dt@kernel.org; > mark.rutland@arm.com; heikki.krogerus@linux.intel.com > Cc: yueyao@google.com; o_leveque@orange.fr; Peter Chen > <peter.chen@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>; linux- > usb@vger.kernel.org; devicetree@vger.kernel.org > Subject: Re: [PATCH 07/12] typec: tcpm: add starting value for drp toggling > > On 09/25/2017 05:45 PM, Li Jun wrote: > > As DRP port autonomously toggles the Rp/Rd need a start value to begin > > with, so add one parameter for it in tcpm_start_drp_toggling. > > > > It does have a starting value. The patch changes the starting value to > TYPEC_CC_RD (from currently one of the RP states) when entering the > SNK_UNATTACHED state. > Please provide a matching description. Yes, actually it's a mismatch, when entering the SNK_UNATTACHED, we should start from Rd, see TCPCI spec: Figure 4-11. TCPC State Diagram before a Connection > > > Signed-off-by: Li Jun <jun.li@nxp.com> > > --- > > drivers/usb/typec/tcpm.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index > > 8483d3e..38a6223 100644 > > --- a/drivers/usb/typec/tcpm.c > > +++ b/drivers/usb/typec/tcpm.c > > @@ -1839,15 +1839,15 @@ static int tcpm_set_charge(struct tcpm_port > *port, bool charge) > > return 0; > > } > > > > -static bool tcpm_start_drp_toggling(struct tcpm_port *port) > > +static bool tcpm_start_drp_toggling(struct tcpm_port *port, > > + enum typec_cc_status cc) > > { > > int ret; > > > > if (port->tcpc->start_drp_toggling && > > port->port_type == TYPEC_PORT_DRP) { > > tcpm_log_force(port, "Start DRP toggling"); > > - ret = port->tcpc->start_drp_toggling(port->tcpc, > > - tcpm_rp_cc(port)); > > + ret = port->tcpc->start_drp_toggling(port->tcpc, cc); > > if (!ret) > > return true; > > } > > @@ -2156,7 +2156,7 @@ static void run_state_machine(struct tcpm_port > *port) > > if (!port->non_pd_role_swap) > > tcpm_swap_complete(port, -ENOTCONN); > > tcpm_src_detach(port); > > - if (tcpm_start_drp_toggling(port)) { > > + if (tcpm_start_drp_toggling(port, tcpm_rp_cc(port))) { > > tcpm_set_state(port, DRP_TOGGLING, 0); > > break; > > } > > @@ -2328,7 +2328,7 @@ static void run_state_machine(struct tcpm_port > *port) > > if (!port->non_pd_role_swap) > > tcpm_swap_complete(port, -ENOTCONN); > > tcpm_snk_detach(port); > > - if (tcpm_start_drp_toggling(port)) { > > + if (tcpm_start_drp_toggling(port, TYPEC_CC_RD)) { > > tcpm_set_state(port, DRP_TOGGLING, 0); > > break; > > } > >