| Message ID | 1525307094-27402-6-git-send-email-jun.li@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Li Jun, On 2018-05-03 02:24, Li Jun wrote: > This patch adds 3 APIs to get the typec port power and data type, > and preferred power role by its name string. > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/usb/typec/class.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/usb/typec.h | 3 +++ > 2 files changed, 55 insertions(+) > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c > index 53df10d..5981e18 100644 > --- a/drivers/usb/typec/class.c > +++ b/drivers/usb/typec/class.c > @@ -9,6 +9,7 @@ > #include <linux/device.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/property.h> > #include <linux/slab.h> > #include <linux/usb/typec.h> > #include <linux/usb/typec_mux.h> > @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = { > [TYPEC_PORT_DRP] = "dual", > }; > > +static const char * const typec_data_types[] = { > + [TYPEC_PORT_DFP] = "host", > + [TYPEC_PORT_UFP] = "device", > + [TYPEC_PORT_DRD] = "dual", > +}; > + > static const char * const typec_port_types_drp[] = { > [TYPEC_PORT_SRC] = "dual [source] sink", > [TYPEC_PORT_SNK] = "dual source [sink]", > @@ -1252,6 +1259,51 @@ void typec_set_pwr_opmode(struct typec_port *port, > } > EXPORT_SYMBOL_GPL(typec_set_pwr_opmode); > > +/** > + * typec_find_power_type - Get the typec port power type Why is this function called typec_find_power_type() and not typec_find_port_type()? It's called port_type in sysfs, having different names just adds confusion. (Otherwise I agree power_type is a better name but...) > + * @name: port type string > + * > + * This routine is used to find the typec_port_type by its string name. > + * > + * Returns typec_port_type if success, otherwise negative error code. > + */ > +int typec_find_power_type(const char *name) > +{ > + return match_string(typec_port_types, ARRAY_SIZE(typec_port_types), > + name); > +} > +EXPORT_SYMBOL_GPL(typec_find_power_type); > + > +/** > + * typec_find_preferred_role - Find the typec drp port preferred power role Why typec_find_preferred_role()? Could be used for any power_role so why not typec_find_power_role()? BR // Mats > + * @name: power role string > + * > + * This routine is used to find the typec_role by its string name of > + * preferred power role(Try.SRC or Try.SNK). > + * > + * Returns typec_role if success, otherwise negative error code. > + */ > +int typec_find_preferred_role(const char *name) > +{ > + return match_string(typec_roles, ARRAY_SIZE(typec_roles), name); > +} > +EXPORT_SYMBOL_GPL(typec_find_preferred_role); > + > +/** > + * typec_find_data_type - Get the typec port data capability > + * @name: data type string > + * > + * This routine is used to find the typec_port_data by its string name. > + * > + * Returns typec_port_data if success, otherwise negative error code. > + */ > +int typec_find_data_type(const char *name) > +{ > + return match_string(typec_data_types, ARRAY_SIZE(typec_data_types), > + name); > +} > +EXPORT_SYMBOL_GPL(typec_find_data_type); > + > /* ------------------------------------------ */ > /* API for Multiplexer/DeMultiplexer Switches */ > > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h > index 672b39b..00c93e7 100644 > --- a/include/linux/usb/typec.h > +++ b/include/linux/usb/typec.h > @@ -267,4 +267,7 @@ int typec_set_orientation(struct typec_port *port, > enum typec_orientation orientation); > int typec_set_mode(struct typec_port *port, int mode); > > +int typec_find_power_type(const char *name); > +int typec_find_preferred_role(const char *name); > +int typec_find_data_type(const char *name); > #endif /* __LINUX_USB_TYPEC_H */ > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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: Mats Karrman [mailto:mats.dev.list@gmail.com] > Sent: 2018年5月12日 3:56 > 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; cw00.choi@samsung.com; > shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>; > gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power > and data config > > Hi Li Jun, > > On 2018-05-03 02:24, Li Jun wrote: > > > This patch adds 3 APIs to get the typec port power and data type, and > > preferred power role by its name string. > > > > Signed-off-by: Li Jun <jun.li@nxp.com> > > --- > > drivers/usb/typec/class.c | 52 > +++++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/usb/typec.h | 3 +++ > > 2 files changed, 55 insertions(+) > > > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c > > index 53df10d..5981e18 100644 > > --- a/drivers/usb/typec/class.c > > +++ b/drivers/usb/typec/class.c > > @@ -9,6 +9,7 @@ > > #include <linux/device.h> > > #include <linux/module.h> > > #include <linux/mutex.h> > > +#include <linux/property.h> > > #include <linux/slab.h> > > #include <linux/usb/typec.h> > > #include <linux/usb/typec_mux.h> > > @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = { > > [TYPEC_PORT_DRP] = "dual", > > }; > > > > +static const char * const typec_data_types[] = { > > + [TYPEC_PORT_DFP] = "host", > > + [TYPEC_PORT_UFP] = "device", > > + [TYPEC_PORT_DRD] = "dual", > > +}; > > + > > static const char * const typec_port_types_drp[] = { > > [TYPEC_PORT_SRC] = "dual [source] sink", > > [TYPEC_PORT_SNK] = "dual source [sink]", @@ -1252,6 +1259,51 > @@ > > void typec_set_pwr_opmode(struct typec_port *port, > > } > > EXPORT_SYMBOL_GPL(typec_set_pwr_opmode); > > > > +/** > > + * typec_find_power_type - Get the typec port power type > > Why is this function called typec_find_power_type() and not > typec_find_port_type()? > It's called port_type in sysfs, having different names just adds confusion. > (Otherwise I agree power_type is a better name but...) We have "port type" before the power and data role separation, this API name's intention is to reflect the power cap, anyway I leave this to be decided by Heikki then. > > > + * @name: port type string > > + * > > + * This routine is used to find the typec_port_type by its string name. > > + * > > + * Returns typec_port_type if success, otherwise negative error code. > > + */ > > +int typec_find_power_type(const char *name) { > > + return match_string(typec_port_types, ARRAY_SIZE(typec_port_types), > > + name); > > +} > > +EXPORT_SYMBOL_GPL(typec_find_power_type); > > + > > +/** > > + * typec_find_preferred_role - Find the typec drp port preferred > > +power role > > Why typec_find_preferred_role()? Could be used for any power_role so why not > typec_find_power_role()? I am not sure if I catch your point of this comment. For preferred role(if support try.sink or try.src) the only allowed power roles are "sink" "source" But for power role, the allowed type are "sink" "source" "dual" Thanks Li Jun > > BR // Mats > > > + * @name: power role string > > + * > > + * This routine is used to find the typec_role by its string name of > > + * preferred power role(Try.SRC or Try.SNK). > > + * > > + * Returns typec_role if success, otherwise negative error code. > > + */ > > +int typec_find_preferred_role(const char *name) { > > + return match_string(typec_roles, ARRAY_SIZE(typec_roles), name); } > > +EXPORT_SYMBOL_GPL(typec_find_preferred_role); > > + > > +/** > > + * typec_find_data_type - Get the typec port data capability > > + * @name: data type string > > + * > > + * This routine is used to find the typec_port_data by its string name. > > + * > > + * Returns typec_port_data if success, otherwise negative error code. > > + */ > > +int typec_find_data_type(const char *name) { > > + return match_string(typec_data_types, ARRAY_SIZE(typec_data_types), > > + name); > > +} > > +EXPORT_SYMBOL_GPL(typec_find_data_type); > > + > > /* ------------------------------------------ */ > > /* API for Multiplexer/DeMultiplexer Switches */ > > > > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h > > index 672b39b..00c93e7 100644 > > --- a/include/linux/usb/typec.h > > +++ b/include/linux/usb/typec.h > > @@ -267,4 +267,7 @@ int typec_set_orientation(struct typec_port *port, > > enum typec_orientation orientation); > > int typec_set_mode(struct typec_port *port, int mode); > > > > +int typec_find_power_type(const char *name); int > > +typec_find_preferred_role(const char *name); int > > +typec_find_data_type(const char *name); > > #endif /* __LINUX_USB_TYPEC_H */ > >
Hi, On 05/14/2018 11:36 AM, Jun Li wrote: > Hi >> -----Original Message----- >> From: Mats Karrman [mailto:mats.dev.list@gmail.com] >> Sent: 2018年5月12日 3:56 >> 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; cw00.choi@samsung.com; >> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>; >> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org; >> dl-linux-imx <linux-imx@nxp.com> >> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power >> and data config >> >> Hi Li Jun, >> >> On 2018-05-03 02:24, Li Jun wrote: >> >>> This patch adds 3 APIs to get the typec port power and data type, and >>> preferred power role by its name string. >>> >>> Signed-off-by: Li Jun <jun.li@nxp.com> >>> --- >>> drivers/usb/typec/class.c | 52 >> +++++++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/usb/typec.h | 3 +++ >>> 2 files changed, 55 insertions(+) >>> >>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c >>> index 53df10d..5981e18 100644 >>> --- a/drivers/usb/typec/class.c >>> +++ b/drivers/usb/typec/class.c >>> @@ -9,6 +9,7 @@ >>> #include <linux/device.h> >>> #include <linux/module.h> >>> #include <linux/mutex.h> >>> +#include <linux/property.h> >>> #include <linux/slab.h> >>> #include <linux/usb/typec.h> >>> #include <linux/usb/typec_mux.h> >>> @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = { >>> [TYPEC_PORT_DRP] = "dual", >>> }; >>> >>> +static const char * const typec_data_types[] = { >>> + [TYPEC_PORT_DFP] = "host", >>> + [TYPEC_PORT_UFP] = "device", >>> + [TYPEC_PORT_DRD] = "dual", >>> +}; >>> + >>> static const char * const typec_port_types_drp[] = { >>> [TYPEC_PORT_SRC] = "dual [source] sink", >>> [TYPEC_PORT_SNK] = "dual source [sink]", @@ -1252,6 +1259,51 >> @@ >>> void typec_set_pwr_opmode(struct typec_port *port, >>> } >>> EXPORT_SYMBOL_GPL(typec_set_pwr_opmode); >>> >>> +/** >>> + * typec_find_power_type - Get the typec port power type >> Why is this function called typec_find_power_type() and not >> typec_find_port_type()? >> It's called port_type in sysfs, having different names just adds confusion. >> (Otherwise I agree power_type is a better name but...) > We have "port type" before the power and data role separation, > this API name's intention is to reflect the power cap, anyway I > leave this to be decided by Heikki then. >>> + * @name: port type string >>> + * >>> + * This routine is used to find the typec_port_type by its string name. >>> + * >>> + * Returns typec_port_type if success, otherwise negative error code. >>> + */ >>> +int typec_find_power_type(const char *name) { >>> + return match_string(typec_port_types, ARRAY_SIZE(typec_port_types), >>> + name); >>> +} >>> +EXPORT_SYMBOL_GPL(typec_find_power_type); >>> + >>> +/** >>> + * typec_find_preferred_role - Find the typec drp port preferred >>> +power role >> Why typec_find_preferred_role()? Could be used for any power_role so why not >> typec_find_power_role()? > I am not sure if I catch your point of this comment. > For preferred role(if support try.sink or try.src) the only allowed power roles are > "sink" > "source" > But for power role, the allowed type are > "sink" > "source" > "dual" Uhm, typing too fast again, I am. A better name would be just typec_find_role(). What I mean is that the function could be used for any situation when someone wants to map a string to a TYPEC_{SOURCE,SINK} constant so it is unnecessary to limit its usage to just preferred role. // Mats > Thanks > Li Jun >> BR // Mats >> >>> + * @name: power role string >>> + * >>> + * This routine is used to find the typec_role by its string name of >>> + * preferred power role(Try.SRC or Try.SNK). >>> + * >>> + * Returns typec_role if success, otherwise negative error code. >>> + */ >>> +int typec_find_preferred_role(const char *name) { >>> + return match_string(typec_roles, ARRAY_SIZE(typec_roles), name); } >>> +EXPORT_SYMBOL_GPL(typec_find_preferred_role); >>> + >>> +/** >>> + * typec_find_data_type - Get the typec port data capability >>> + * @name: data type string >>> + * >>> + * This routine is used to find the typec_port_data by its string name. >>> + * >>> + * Returns typec_port_data if success, otherwise negative error code. >>> + */ >>> +int typec_find_data_type(const char *name) { >>> + return match_string(typec_data_types, ARRAY_SIZE(typec_data_types), >>> + name); >>> +} >>> +EXPORT_SYMBOL_GPL(typec_find_data_type); >>> + >>> /* ------------------------------------------ */ >>> /* API for Multiplexer/DeMultiplexer Switches */ >>> >>> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h >>> index 672b39b..00c93e7 100644 >>> --- a/include/linux/usb/typec.h >>> +++ b/include/linux/usb/typec.h >>> @@ -267,4 +267,7 @@ int typec_set_orientation(struct typec_port *port, >>> enum typec_orientation orientation); >>> int typec_set_mode(struct typec_port *port, int mode); >>> >>> +int typec_find_power_type(const char *name); int >>> +typec_find_preferred_role(const char *name); int >>> +typec_find_data_type(const char *name); >>> #endif /* __LINUX_USB_TYPEC_H */ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi guys, On Tue, May 15, 2018 at 10:52:57PM +0200, Mats Karrman wrote: > Hi, > > On 05/14/2018 11:36 AM, Jun Li wrote: > > > Hi > >> -----Original Message----- > >> From: Mats Karrman [mailto:mats.dev.list@gmail.com] > >> Sent: 2018???5???12??? 3:56 > >> 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; cw00.choi@samsung.com; > >> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>; > >> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org; > >> dl-linux-imx <linux-imx@nxp.com> > >> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power > >> and data config > >> > >> Hi Li Jun, > >> > >> On 2018-05-03 02:24, Li Jun wrote: > >> > >>> This patch adds 3 APIs to get the typec port power and data type, and > >>> preferred power role by its name string. > >>> > >>> Signed-off-by: Li Jun <jun.li@nxp.com> > >>> --- > >>> drivers/usb/typec/class.c | 52 > >> +++++++++++++++++++++++++++++++++++++++++++++++ > >>> include/linux/usb/typec.h | 3 +++ > >>> 2 files changed, 55 insertions(+) > >>> > >>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c > >>> index 53df10d..5981e18 100644 > >>> --- a/drivers/usb/typec/class.c > >>> +++ b/drivers/usb/typec/class.c > >>> @@ -9,6 +9,7 @@ > >>> #include <linux/device.h> > >>> #include <linux/module.h> > >>> #include <linux/mutex.h> > >>> +#include <linux/property.h> I don't think you need that anymore. > >>> #include <linux/slab.h> > >>> #include <linux/usb/typec.h> > >>> #include <linux/usb/typec_mux.h> > >>> @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = { > >>> [TYPEC_PORT_DRP] = "dual", > >>> }; > >>> > >>> +static const char * const typec_data_types[] = { > >>> + [TYPEC_PORT_DFP] = "host", > >>> + [TYPEC_PORT_UFP] = "device", > >>> + [TYPEC_PORT_DRD] = "dual", > >>> +}; > >>> + > >>> static const char * const typec_port_types_drp[] = { > >>> [TYPEC_PORT_SRC] = "dual [source] sink", > >>> [TYPEC_PORT_SNK] = "dual source [sink]", @@ -1252,6 +1259,51 > >> @@ > >>> void typec_set_pwr_opmode(struct typec_port *port, > >>> } > >>> EXPORT_SYMBOL_GPL(typec_set_pwr_opmode); > >>> > >>> +/** > >>> + * typec_find_power_type - Get the typec port power type > >> Why is this function called typec_find_power_type() and not > >> typec_find_port_type()? > >> It's called port_type in sysfs, having different names just adds confusion. > >> (Otherwise I agree power_type is a better name but...) > > We have "port type" before the power and data role separation, > > this API name's intention is to reflect the power cap, anyway I > > leave this to be decided by Heikki then. I really hate the "*_type" naming. It was understandable when there was no separate power and data roles defined in the specification, but now that there are, it's just confusing. IMO we should not use it anywhere. So to me typec_find_type() is just as bad as typec_find_power_type() because it has the "type" in it. I wonder if this function is necessary at all? If it is, then perhaps we can think of some better name for it, name that gives a better hint what it is used for. > >>> + * @name: port type string > >>> + * > >>> + * This routine is used to find the typec_port_type by its string name. > >>> + * > >>> + * Returns typec_port_type if success, otherwise negative error code. > >>> + */ > >>> +int typec_find_power_type(const char *name) { > >>> + return match_string(typec_port_types, ARRAY_SIZE(typec_port_types), > >>> + name); > >>> +} > >>> +EXPORT_SYMBOL_GPL(typec_find_power_type); > >>> + > >>> +/** > >>> + * typec_find_preferred_role - Find the typec drp port preferred > >>> +power role > >> Why typec_find_preferred_role()? Could be used for any power_role so why not > >> typec_find_power_role()? > > I am not sure if I catch your point of this comment. > > For preferred role(if support try.sink or try.src) the only allowed power roles are > > "sink" > > "source" > > But for power role, the allowed type are > > "sink" > > "source" > > "dual" > > Uhm, typing too fast again, I am. A better name would be just typec_find_role(). > What I mean is that the function could be used for any situation when > someone wants to map a string to a TYPEC_{SOURCE,SINK} constant so it > is unnecessary to limit its usage to just preferred role. That sounds reasonable to me. Thanks,
Hi, On 05/16/2018 02:25 PM, Heikki Krogerus wrote: > Hi guys, > > On Tue, May 15, 2018 at 10:52:57PM +0200, Mats Karrman wrote: >> Hi, >> >> On 05/14/2018 11:36 AM, Jun Li wrote: >> >>> Hi >>>> -----Original Message----- >>>> From: Mats Karrman [mailto:mats.dev.list@gmail.com] >>>> Sent: 2018???5???12??? 3:56 >>>> 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; cw00.choi@samsung.com; >>>> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>; >>>> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org; >>>> dl-linux-imx <linux-imx@nxp.com> >>>> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power >>>> and data config >>>> >>>> Hi Li Jun, >>>> >>>> On 2018-05-03 02:24, Li Jun wrote: >>>> >>>>> This patch adds 3 APIs to get the typec port power and data type, and >>>>> preferred power role by its name string. >>>>> >>>>> Signed-off-by: Li Jun <jun.li@nxp.com> >>>>> --- >>>>> drivers/usb/typec/class.c | 52 >>>> +++++++++++++++++++++++++++++++++++++++++++++++ >>>>> include/linux/usb/typec.h | 3 +++ >>>>> 2 files changed, 55 insertions(+) >>>>> >>>>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c >>>>> index 53df10d..5981e18 100644 >>>>> --- a/drivers/usb/typec/class.c >>>>> +++ b/drivers/usb/typec/class.c >>>>> @@ -9,6 +9,7 @@ >>>>> #include <linux/device.h> >>>>> #include <linux/module.h> >>>>> #include <linux/mutex.h> >>>>> +#include <linux/property.h> > I don't think you need that anymore. > >>>>> #include <linux/slab.h> >>>>> #include <linux/usb/typec.h> >>>>> #include <linux/usb/typec_mux.h> >>>>> @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = { >>>>> [TYPEC_PORT_DRP] = "dual", >>>>> }; >>>>> >>>>> +static const char * const typec_data_types[] = { >>>>> + [TYPEC_PORT_DFP] = "host", >>>>> + [TYPEC_PORT_UFP] = "device", >>>>> + [TYPEC_PORT_DRD] = "dual", >>>>> +}; >>>>> + >>>>> static const char * const typec_port_types_drp[] = { >>>>> [TYPEC_PORT_SRC] = "dual [source] sink", >>>>> [TYPEC_PORT_SNK] = "dual source [sink]", @@ -1252,6 +1259,51 >>>> @@ >>>>> void typec_set_pwr_opmode(struct typec_port *port, >>>>> } >>>>> EXPORT_SYMBOL_GPL(typec_set_pwr_opmode); >>>>> >>>>> +/** >>>>> + * typec_find_power_type - Get the typec port power type >>>> Why is this function called typec_find_power_type() and not >>>> typec_find_port_type()? >>>> It's called port_type in sysfs, having different names just adds confusion. >>>> (Otherwise I agree power_type is a better name but...) >>> We have "port type" before the power and data role separation, >>> this API name's intention is to reflect the power cap, anyway I >>> leave this to be decided by Heikki then. > I really hate the "*_type" naming. It was understandable when there > was no separate power and data roles defined in the specification, but > now that there are, it's just confusing. IMO we should not use it > anywhere. > > So to me typec_find_type() is just as bad as typec_find_power_type() > because it has the "type" in it. I wonder if this function is > necessary at all? If it is, then perhaps we can think of some better > name for it, name that gives a better hint what it is used for. I reread this patch and tried to see it more in the context of the other patches and the existing code. The naming of the existing string tables doesn't help in getting this right, however I have a proposal: typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD typec_find_power_role() to get to TYPEC_SINK/SOURCE and sometime, if the use should arise typec_find_data_role() to get to TYPEC_DEVICE/HOST I think it is fairly comprehensible, *_port_* concerns a capability and without *_port_* it is an actual state. Plus it matches the names of the constants. BR // Mats >>>>> + * @name: port type string >>>>> + * >>>>> + * This routine is used to find the typec_port_type by its string name. >>>>> + * >>>>> + * Returns typec_port_type if success, otherwise negative error code. >>>>> + */ >>>>> +int typec_find_power_type(const char *name) { >>>>> + return match_string(typec_port_types, ARRAY_SIZE(typec_port_types), >>>>> + name); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(typec_find_power_type); >>>>> + >>>>> +/** >>>>> + * typec_find_preferred_role - Find the typec drp port preferred >>>>> +power role >>>> Why typec_find_preferred_role()? Could be used for any power_role so why not >>>> typec_find_power_role()? >>> I am not sure if I catch your point of this comment. >>> For preferred role(if support try.sink or try.src) the only allowed power roles are >>> "sink" >>> "source" >>> But for power role, the allowed type are >>> "sink" >>> "source" >>> "dual" >> Uhm, typing too fast again, I am. A better name would be just typec_find_role(). >> What I mean is that the function could be used for any situation when >> someone wants to map a string to a TYPEC_{SOURCE,SINK} constant so it >> is unnecessary to limit its usage to just preferred role. > That sounds reasonable to me. > > > Thanks, > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 16, 2018 at 10:55:36PM +0200, Mats Karrman wrote: > Hi, > > On 05/16/2018 02:25 PM, Heikki Krogerus wrote: > > > Hi guys, > > > > On Tue, May 15, 2018 at 10:52:57PM +0200, Mats Karrman wrote: > >> Hi, > >> > >> On 05/14/2018 11:36 AM, Jun Li wrote: > >> > >>> Hi > >>>> -----Original Message----- > >>>> From: Mats Karrman [mailto:mats.dev.list@gmail.com] > >>>> Sent: 2018???5???12??? 3:56 > >>>> 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; cw00.choi@samsung.com; > >>>> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>; > >>>> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org; > >>>> dl-linux-imx <linux-imx@nxp.com> > >>>> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power > >>>> and data config > >>>> > >>>> Hi Li Jun, > >>>> > >>>> On 2018-05-03 02:24, Li Jun wrote: > >>>> > >>>>> This patch adds 3 APIs to get the typec port power and data type, and > >>>>> preferred power role by its name string. > >>>>> > >>>>> Signed-off-by: Li Jun <jun.li@nxp.com> > >>>>> --- > >>>>> drivers/usb/typec/class.c | 52 > >>>> +++++++++++++++++++++++++++++++++++++++++++++++ > >>>>> include/linux/usb/typec.h | 3 +++ > >>>>> 2 files changed, 55 insertions(+) > >>>>> > >>>>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c > >>>>> index 53df10d..5981e18 100644 > >>>>> --- a/drivers/usb/typec/class.c > >>>>> +++ b/drivers/usb/typec/class.c > >>>>> @@ -9,6 +9,7 @@ > >>>>> #include <linux/device.h> > >>>>> #include <linux/module.h> > >>>>> #include <linux/mutex.h> > >>>>> +#include <linux/property.h> > > I don't think you need that anymore. > > > >>>>> #include <linux/slab.h> > >>>>> #include <linux/usb/typec.h> > >>>>> #include <linux/usb/typec_mux.h> > >>>>> @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = { > >>>>> [TYPEC_PORT_DRP] = "dual", > >>>>> }; > >>>>> > >>>>> +static const char * const typec_data_types[] = { > >>>>> + [TYPEC_PORT_DFP] = "host", > >>>>> + [TYPEC_PORT_UFP] = "device", > >>>>> + [TYPEC_PORT_DRD] = "dual", > >>>>> +}; > >>>>> + > >>>>> static const char * const typec_port_types_drp[] = { > >>>>> [TYPEC_PORT_SRC] = "dual [source] sink", > >>>>> [TYPEC_PORT_SNK] = "dual source [sink]", @@ -1252,6 +1259,51 > >>>> @@ > >>>>> void typec_set_pwr_opmode(struct typec_port *port, > >>>>> } > >>>>> EXPORT_SYMBOL_GPL(typec_set_pwr_opmode); > >>>>> > >>>>> +/** > >>>>> + * typec_find_power_type - Get the typec port power type > >>>> Why is this function called typec_find_power_type() and not > >>>> typec_find_port_type()? > >>>> It's called port_type in sysfs, having different names just adds confusion. > >>>> (Otherwise I agree power_type is a better name but...) > >>> We have "port type" before the power and data role separation, > >>> this API name's intention is to reflect the power cap, anyway I > >>> leave this to be decided by Heikki then. > > I really hate the "*_type" naming. It was understandable when there > > was no separate power and data roles defined in the specification, but > > now that there are, it's just confusing. IMO we should not use it > > anywhere. > > > > So to me typec_find_type() is just as bad as typec_find_power_type() > > because it has the "type" in it. I wonder if this function is > > necessary at all? If it is, then perhaps we can think of some better > > name for it, name that gives a better hint what it is used for. > > I reread this patch and tried to see it more in the context of the other > patches and the existing code. The naming of the existing string tables > doesn't help in getting this right, however I have a proposal: > > typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP > typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD > typec_find_power_role() to get to TYPEC_SINK/SOURCE > > and sometime, if the use should arise > > typec_find_data_role() to get to TYPEC_DEVICE/HOST > > I think it is fairly comprehensible, *_port_* concerns a capability and > without *_port_* it is an actual state. Plus it matches the names of the > constants. Well, there are now four things to consider: 1) the roles (power and data) the port is capable of supporting 2) Try.SRC and Try.SNK, i.e. the preferred role 3) the current roles 4) the role(s) the user want's to limit the use of a port with DRP ports The last one I don't know if it's relevant with these functions. It's not information that we would get for example from firmware. I also don't think we need to use these functions with the current roles the port is in. If the preferred role is "sink" or "source", so just like the power role, we don't need separate function for it here. So isn't two functions all we need here: one for the power and one for data role? Thanks,
Hi Mats, > > Uhm, typing too fast again, I am. A better name would be just > typec_find_role(). > What I mean is that the function could be used for any situation when > someone wants to map a string to a TYPEC_{SOURCE,SINK} constant so it is > unnecessary to limit its usage to just preferred role. Get your idea, that will be a more general API for typec class. Thanks Li Jun
SGkNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogSGVpa2tpIEtyb2dlcnVz IFttYWlsdG86aGVpa2tpLmtyb2dlcnVzQGxpbnV4LmludGVsLmNvbV0NCj4gU2VudDogMjAxOMTq NdTCMTbI1SAyMDoyNQ0KPiBUbzogSnVuIExpIDxqdW4ubGlAbnhwLmNvbT47IE1hdHMgS2Fycm1h biA8bWF0cy5kZXYubGlzdEBnbWFpbC5jb20+DQo+IENjOiByb2JoK2R0QGtlcm5lbC5vcmc7IGdy ZWdraEBsaW51eGZvdW5kYXRpb24ub3JnOyBsaW51eEByb2Vjay11cy5uZXQ7DQo+IGEuaGFqZGFA c2Ftc3VuZy5jb207IGN3MDAuY2hvaUBzYW1zdW5nLmNvbTsgc2h1ZmFuX2xlZUByaWNodGVrLmNv bTsNCj4gUGV0ZXIgQ2hlbiA8cGV0ZXIuY2hlbkBueHAuY29tPjsgZ3NvbWxvQGdtYWlsLmNvbTsN Cj4gZGV2aWNldHJlZUB2Z2VyLmtlcm5lbC5vcmc7IGxpbnV4LXVzYkB2Z2VyLmtlcm5lbC5vcmc7 IGRsLWxpbnV4LWlteA0KPiA8bGludXgtaW14QG54cC5jb20+DQo+IFN1YmplY3Q6IFJlOiBbUEFU Q0ggdjUgMDUvMTRdIHVzYjogdHlwZWM6IGFkZCBBUEkgdG8gZ2V0IHR5cGVjIGJhc2ljIHBvcnQg cG93ZXINCj4gYW5kIGRhdGEgY29uZmlnDQo+IA0KPiBIaSBndXlzLA0KPiANCj4gT24gVHVlLCBN YXkgMTUsIDIwMTggYXQgMTA6NTI6NTdQTSArMDIwMCwgTWF0cyBLYXJybWFuIHdyb3RlOg0KPiA+ IEhpLA0KPiA+DQo+ID4gT24gMDUvMTQvMjAxOCAxMTozNiBBTSwgSnVuIExpIHdyb3RlOg0KPiA+ DQo+ID4gPiBIaQ0KPiA+ID4+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+ID4gPj4gRnJv bTogTWF0cyBLYXJybWFuIFttYWlsdG86bWF0cy5kZXYubGlzdEBnbWFpbC5jb21dDQo+ID4gPj4g U2VudDogMjAxOD8/PzU/Pz8xMj8/PyAzOjU2DQo+ID4gPj4gVG86IEp1biBMaSA8anVuLmxpQG54 cC5jb20+OyByb2JoK2R0QGtlcm5lbC5vcmc7DQo+ID4gPj4gZ3JlZ2toQGxpbnV4Zm91bmRhdGlv bi5vcmc7IGhlaWtraS5rcm9nZXJ1c0BsaW51eC5pbnRlbC5jb207DQo+ID4gPj4gbGludXhAcm9l Y2stdXMubmV0DQo+ID4gPj4gQ2M6IGEuaGFqZGFAc2Ftc3VuZy5jb207IGN3MDAuY2hvaUBzYW1z dW5nLmNvbTsNCj4gPiA+PiBzaHVmYW5fbGVlQHJpY2h0ZWsuY29tOyBQZXRlciBDaGVuIDxwZXRl ci5jaGVuQG54cC5jb20+Ow0KPiA+ID4+IGdzb21sb0BnbWFpbC5jb207IGRldmljZXRyZWVAdmdl ci5rZXJuZWwub3JnOw0KPiA+ID4+IGxpbnV4LXVzYkB2Z2VyLmtlcm5lbC5vcmc7IGRsLWxpbnV4 LWlteCA8bGludXgtaW14QG54cC5jb20+DQo+ID4gPj4gU3ViamVjdDogUmU6IFtQQVRDSCB2NSAw NS8xNF0gdXNiOiB0eXBlYzogYWRkIEFQSSB0byBnZXQgdHlwZWMNCj4gPiA+PiBiYXNpYyBwb3J0 IHBvd2VyIGFuZCBkYXRhIGNvbmZpZw0KPiA+ID4+DQo+ID4gPj4gSGkgTGkgSnVuLA0KPiA+ID4+ DQo+ID4gPj4gT24gMjAxOC0wNS0wMyAwMjoyNCwgTGkgSnVuIHdyb3RlOg0KPiA+ID4+DQo+ID4g Pj4+IFRoaXMgcGF0Y2ggYWRkcyAzIEFQSXMgdG8gZ2V0IHRoZSB0eXBlYyBwb3J0IHBvd2VyIGFu ZCBkYXRhIHR5cGUsDQo+ID4gPj4+IGFuZCBwcmVmZXJyZWQgcG93ZXIgcm9sZSBieSBpdHMgbmFt ZSBzdHJpbmcuDQo+ID4gPj4+DQo+ID4gPj4+IFNpZ25lZC1vZmYtYnk6IExpIEp1biA8anVuLmxp QG54cC5jb20+DQo+ID4gPj4+IC0tLQ0KPiA+ID4+PiAgIGRyaXZlcnMvdXNiL3R5cGVjL2NsYXNz LmMgfCA1Mg0KPiA+ID4+ICsrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr KysrKysrDQo+ID4gPj4+ICAgaW5jbHVkZS9saW51eC91c2IvdHlwZWMuaCB8ICAzICsrKw0KPiA+ ID4+PiAgIDIgZmlsZXMgY2hhbmdlZCwgNTUgaW5zZXJ0aW9ucygrKQ0KPiA+ID4+Pg0KPiA+ID4+ PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy91c2IvdHlwZWMvY2xhc3MuYyBiL2RyaXZlcnMvdXNiL3R5 cGVjL2NsYXNzLmMNCj4gPiA+Pj4gaW5kZXggNTNkZjEwZC4uNTk4MWUxOCAxMDA2NDQNCj4gPiA+ Pj4gLS0tIGEvZHJpdmVycy91c2IvdHlwZWMvY2xhc3MuYw0KPiA+ID4+PiArKysgYi9kcml2ZXJz L3VzYi90eXBlYy9jbGFzcy5jDQo+ID4gPj4+IEBAIC05LDYgKzksNyBAQA0KPiA+ID4+PiAgICNp bmNsdWRlIDxsaW51eC9kZXZpY2UuaD4NCj4gPiA+Pj4gICAjaW5jbHVkZSA8bGludXgvbW9kdWxl Lmg+DQo+ID4gPj4+ICAgI2luY2x1ZGUgPGxpbnV4L211dGV4Lmg+DQo+ID4gPj4+ICsjaW5jbHVk ZSA8bGludXgvcHJvcGVydHkuaD4NCj4gDQo+IEkgZG9uJ3QgdGhpbmsgeW91IG5lZWQgdGhhdCBh bnltb3JlLg0KDQpJIHdpbGwgcmVtb3ZlIGl0Lg0KDQo+IA0KPiA+ID4+PiAgICNpbmNsdWRlIDxs aW51eC9zbGFiLmg+DQo+ID4gPj4+ICAgI2luY2x1ZGUgPGxpbnV4L3VzYi90eXBlYy5oPg0KPiA+ ID4+PiAgICNpbmNsdWRlIDxsaW51eC91c2IvdHlwZWNfbXV4Lmg+IEBAIC04MDIsNiArODAzLDEy IEBAIHN0YXRpYw0KPiA+ID4+PiBjb25zdCBjaGFyICogY29uc3QgdHlwZWNfcG9ydF90eXBlc1td ID0gew0KPiA+ID4+PiAgIAlbVFlQRUNfUE9SVF9EUlBdID0gImR1YWwiLA0KPiA+ID4+PiAgIH07 DQo+ID4gPj4+DQo+ID4gPj4+ICtzdGF0aWMgY29uc3QgY2hhciAqIGNvbnN0IHR5cGVjX2RhdGFf dHlwZXNbXSA9IHsNCj4gPiA+Pj4gKwlbVFlQRUNfUE9SVF9ERlBdID0gImhvc3QiLA0KPiA+ID4+ PiArCVtUWVBFQ19QT1JUX1VGUF0gPSAiZGV2aWNlIiwNCj4gPiA+Pj4gKwlbVFlQRUNfUE9SVF9E UkRdID0gImR1YWwiLA0KPiA+ID4+PiArfTsNCj4gPiA+Pj4gKw0KPiA+ID4+PiAgIHN0YXRpYyBj b25zdCBjaGFyICogY29uc3QgdHlwZWNfcG9ydF90eXBlc19kcnBbXSA9IHsNCj4gPiA+Pj4gICAJ W1RZUEVDX1BPUlRfU1JDXSA9ICJkdWFsIFtzb3VyY2VdIHNpbmsiLA0KPiA+ID4+PiAgIAlbVFlQ RUNfUE9SVF9TTktdID0gImR1YWwgc291cmNlIFtzaW5rXSIsIEBAIC0xMjUyLDYgKzEyNTksNTEN Cj4gPiA+PiBAQA0KPiA+ID4+PiB2b2lkIHR5cGVjX3NldF9wd3Jfb3Btb2RlKHN0cnVjdCB0eXBl Y19wb3J0ICpwb3J0LA0KPiA+ID4+PiAgIH0NCj4gPiA+Pj4gICBFWFBPUlRfU1lNQk9MX0dQTCh0 eXBlY19zZXRfcHdyX29wbW9kZSk7DQo+ID4gPj4+DQo+ID4gPj4+ICsvKioNCj4gPiA+Pj4gKyAq IHR5cGVjX2ZpbmRfcG93ZXJfdHlwZSAtIEdldCB0aGUgdHlwZWMgcG9ydCBwb3dlciB0eXBlDQo+ ID4gPj4gV2h5IGlzIHRoaXMgZnVuY3Rpb24gY2FsbGVkIHR5cGVjX2ZpbmRfcG93ZXJfdHlwZSgp IGFuZCBub3QNCj4gPiA+PiB0eXBlY19maW5kX3BvcnRfdHlwZSgpPw0KPiA+ID4+IEl0J3MgY2Fs bGVkIHBvcnRfdHlwZSBpbiBzeXNmcywgaGF2aW5nIGRpZmZlcmVudCBuYW1lcyBqdXN0IGFkZHMg Y29uZnVzaW9uLg0KPiA+ID4+IChPdGhlcndpc2UgSSBhZ3JlZSBwb3dlcl90eXBlIGlzIGEgYmV0 dGVyIG5hbWUgYnV0Li4uKQ0KPiA+ID4gV2UgaGF2ZSAicG9ydCB0eXBlIiBiZWZvcmUgdGhlIHBv d2VyIGFuZCBkYXRhIHJvbGUgc2VwYXJhdGlvbiwgdGhpcw0KPiA+ID4gQVBJIG5hbWUncyBpbnRl bnRpb24gaXMgdG8gcmVmbGVjdCB0aGUgcG93ZXIgY2FwLCBhbnl3YXkgSSBsZWF2ZQ0KPiA+ID4g dGhpcyB0byBiZSBkZWNpZGVkIGJ5IEhlaWtraSB0aGVuLg0KPiANCj4gSSByZWFsbHkgaGF0ZSB0 aGUgIipfdHlwZSIgbmFtaW5nLiBJdCB3YXMgdW5kZXJzdGFuZGFibGUgd2hlbiB0aGVyZSB3YXMg bm8NCj4gc2VwYXJhdGUgcG93ZXIgYW5kIGRhdGEgcm9sZXMgZGVmaW5lZCBpbiB0aGUgc3BlY2lm aWNhdGlvbiwgYnV0IG5vdyB0aGF0IHRoZXJlDQo+IGFyZSwgaXQncyBqdXN0IGNvbmZ1c2luZy4g SU1PIHdlIHNob3VsZCBub3QgdXNlIGl0IGFueXdoZXJlLg0KDQpPSywga25vdyB5b3VyIHByZWZl cmVuY2Ugbm93Lg0KDQo+IA0KPiBTbyB0byBtZSB0eXBlY19maW5kX3R5cGUoKSBpcyBqdXN0IGFz IGJhZCBhcyB0eXBlY19maW5kX3Bvd2VyX3R5cGUoKSBiZWNhdXNlIGl0DQo+IGhhcyB0aGUgInR5 cGUiIGluIGl0LiBJIHdvbmRlciBpZiB0aGlzIGZ1bmN0aW9uIGlzIG5lY2Vzc2FyeSBhdCBhbGw/ IElmIGl0IGlzLCB0aGVuDQo+IHBlcmhhcHMgd2UgY2FuIHRoaW5rIG9mIHNvbWUgYmV0dGVyIG5h bWUgZm9yIGl0LCBuYW1lIHRoYXQgZ2l2ZXMgYSBiZXR0ZXIgaGludA0KPiB3aGF0IGl0IGlzIHVz ZWQgZm9yLg0KDQpXZSBuZWVkIGEgd2F5IHRvIG1hdGNoIGEgc3RyaW5nIHRvIGEgdmFsdWUgd2hp Y2ggaGFzIHRvIGJlIGRvbmUgdmlhIHR5cGVjDQpjbGFzcy4NCiANCj4gDQo+ID4gPj4+ICsgKiBA bmFtZTogcG9ydCB0eXBlIHN0cmluZw0KPiA+ID4+PiArICoNCj4gPiA+Pj4gKyAqIFRoaXMgcm91 dGluZSBpcyB1c2VkIHRvIGZpbmQgdGhlIHR5cGVjX3BvcnRfdHlwZSBieSBpdHMgc3RyaW5nIG5h bWUuDQo+ID4gPj4+ICsgKg0KPiA+ID4+PiArICogUmV0dXJucyB0eXBlY19wb3J0X3R5cGUgaWYg c3VjY2Vzcywgb3RoZXJ3aXNlIG5lZ2F0aXZlIGVycm9yIGNvZGUuDQo+ID4gPj4+ICsgKi8NCj4g PiA+Pj4gK2ludCB0eXBlY19maW5kX3Bvd2VyX3R5cGUoY29uc3QgY2hhciAqbmFtZSkgew0KPiA+ ID4+PiArCXJldHVybiBtYXRjaF9zdHJpbmcodHlwZWNfcG9ydF90eXBlcywNCj4gQVJSQVlfU0la RSh0eXBlY19wb3J0X3R5cGVzKSwNCj4gPiA+Pj4gKwkJCSAgICBuYW1lKTsNCj4gPiA+Pj4gK30N Cj4gPiA+Pj4gK0VYUE9SVF9TWU1CT0xfR1BMKHR5cGVjX2ZpbmRfcG93ZXJfdHlwZSk7DQo+ID4g Pj4+ICsNCj4gPiA+Pj4gKy8qKg0KPiA+ID4+PiArICogdHlwZWNfZmluZF9wcmVmZXJyZWRfcm9s ZSAtIEZpbmQgdGhlIHR5cGVjIGRycCBwb3J0IHByZWZlcnJlZA0KPiA+ID4+PiArcG93ZXIgcm9s ZQ0KPiA+ID4+IFdoeSB0eXBlY19maW5kX3ByZWZlcnJlZF9yb2xlKCk/IENvdWxkIGJlIHVzZWQg Zm9yIGFueSBwb3dlcl9yb2xlDQo+ID4gPj4gc28gd2h5IG5vdCB0eXBlY19maW5kX3Bvd2VyX3Jv bGUoKT8NCj4gPiA+IEkgYW0gbm90IHN1cmUgaWYgSSBjYXRjaCB5b3VyIHBvaW50IG9mIHRoaXMg Y29tbWVudC4NCj4gPiA+IEZvciBwcmVmZXJyZWQgcm9sZShpZiBzdXBwb3J0IHRyeS5zaW5rIG9y IHRyeS5zcmMpIHRoZSBvbmx5IGFsbG93ZWQNCj4gPiA+IHBvd2VyIHJvbGVzIGFyZSAic2luayIN Cj4gPiA+ICJzb3VyY2UiDQo+ID4gPiBCdXQgZm9yIHBvd2VyIHJvbGUsIHRoZSBhbGxvd2VkIHR5 cGUgYXJlICJzaW5rIg0KPiA+ID4gInNvdXJjZSINCj4gPiA+ICJkdWFsIg0KPiA+DQo+ID4gVWht LCB0eXBpbmcgdG9vIGZhc3QgYWdhaW4sIEkgYW0uIEEgYmV0dGVyIG5hbWUgd291bGQgYmUganVz dA0KPiB0eXBlY19maW5kX3JvbGUoKS4NCj4gPiBXaGF0IEkgbWVhbiBpcyB0aGF0IHRoZSBmdW5j dGlvbiBjb3VsZCBiZSB1c2VkIGZvciBhbnkgc2l0dWF0aW9uIHdoZW4NCj4gPiBzb21lb25lIHdh bnRzIHRvIG1hcCBhIHN0cmluZyB0byBhIFRZUEVDX3tTT1VSQ0UsU0lOS30gY29uc3RhbnQgc28g aXQNCj4gPiBpcyB1bm5lY2Vzc2FyeSB0byBsaW1pdCBpdHMgdXNhZ2UgdG8ganVzdCBwcmVmZXJy ZWQgcm9sZS4NCj4gDQo+IFRoYXQgc291bmRzIHJlYXNvbmFibGUgdG8gbWUuDQoNCkFncmVlZA0K DQp0aGFua3MNCkxpIEp1bg0KPiANCj4gDQo+IFRoYW5rcywNCj4gDQo+IC0tDQo+IGhlaWtraQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mats > I reread this patch and tried to see it more in the context of the other patches > and the existing code. The naming of the existing string tables doesn't help in > getting this right, however I have a proposal: > > typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP > typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD > typec_find_power_role() to get to TYPEC_SINK/SOURCE > > and sometime, if the use should arise > > typec_find_data_role() to get to TYPEC_DEVICE/HOST > > I think it is fairly comprehensible, *_port_* concerns a capability and without > *_port_* it is an actual state. Plus it matches the names of the constants. > Reasonable to me, I was using "type" to concern a capability but it was not preferred, if Heikki agree this proposal, I will change the API names like this and include 4 APIs above. Thanks Li Jun
Hi Heikki, > > I reread this patch and tried to see it more in the context of the > > other patches and the existing code. The naming of the existing string > > tables doesn't help in getting this right, however I have a proposal: > > > > typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP > > typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD > > typec_find_power_role() to get to TYPEC_SINK/SOURCE > > > > and sometime, if the use should arise > > > > typec_find_data_role() to get to TYPEC_DEVICE/HOST > > > > I think it is fairly comprehensible, *_port_* concerns a capability > > and without *_port_* it is an actual state. Plus it matches the names > > of the constants. > > Well, there are now four things to consider: > > 1) the roles (power and data) the port is capable of supporting > 2) Try.SRC and Try.SNK, i.e. the preferred role > 3) the current roles > 4) the role(s) the user want's to limit the use of a port with DRP ports > > The last one I don't know if it's relevant with these functions. It's not > information that we would get for example from firmware. > > I also don't think we need to use these functions with the current roles the port > is in. > > If the preferred role is "sink" or "source", so just like the power role, we don't > need separate function for it here. > > So isn't two functions all we need here: one for the power and one for data > role? Take power as an example, can we use one function to only look up typec_port_types[]? as capability(typec_port_type) and state(typec_role) are different enum, and the allowed strings are different. static const char * const typec_roles[] = { [TYPEC_SINK] = "sink", [TYPEC_SOURCE] = "source", }; static const char * const typec_port_types[] = { [TYPEC_PORT_SRC] = "source", [TYPEC_PORT_SNK] = "sink", [TYPEC_PORT_DRP] = "dual", }; Thanks Li Jun > > > Thanks, > > -- > heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 17, 2018 at 02:05:41PM +0000, Jun Li wrote: > Hi Heikki, > > > > I reread this patch and tried to see it more in the context of the > > > other patches and the existing code. The naming of the existing string > > > tables doesn't help in getting this right, however I have a proposal: > > > > > > typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP > > > typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD > > > typec_find_power_role() to get to TYPEC_SINK/SOURCE > > > > > > and sometime, if the use should arise > > > > > > typec_find_data_role() to get to TYPEC_DEVICE/HOST > > > > > > I think it is fairly comprehensible, *_port_* concerns a capability > > > and without *_port_* it is an actual state. Plus it matches the names > > > of the constants. > > > > Well, there are now four things to consider: > > > > 1) the roles (power and data) the port is capable of supporting > > 2) Try.SRC and Try.SNK, i.e. the preferred role > > 3) the current roles > > 4) the role(s) the user want's to limit the use of a port with DRP ports > > > > The last one I don't know if it's relevant with these functions. It's not > > information that we would get for example from firmware. > > > > I also don't think we need to use these functions with the current roles the port > > is in. > > > > If the preferred role is "sink" or "source", so just like the power role, we don't > > need separate function for it here. > > > > So isn't two functions all we need here: one for the power and one for data > > role? > > Take power as an example, can we use one function to only look up > typec_port_types[]? as capability(typec_port_type) and state(typec_role) > are different enum, and the allowed strings are different. > > static const char * const typec_roles[] = { > [TYPEC_SINK] = "sink", > [TYPEC_SOURCE] = "source", > }; > > static const char * const typec_port_types[] = { > [TYPEC_PORT_SRC] = "source", > [TYPEC_PORT_SNK] = "sink", > [TYPEC_PORT_DRP] = "dual", > }; Where out side the class code we need to convert the current role, the "state", with these functions? Thanks,
Hi > -----Original Message----- > From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com] > Sent: 2018年5月17日 22:24 > To: Jun Li <jun.li@nxp.com> > Cc: Mats Karrman <mats.dev.list@gmail.com>; robh+dt@kernel.org; > gregkh@linuxfoundation.org; linux@roeck-us.net; a.hajda@samsung.com; > cw00.choi@samsung.com; shufan_lee@richtek.com; Peter Chen > <peter.chen@nxp.com>; gsomlo@gmail.com; devicetree@vger.kernel.org; > linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power > and data config > > On Thu, May 17, 2018 at 02:05:41PM +0000, Jun Li wrote: > > Hi Heikki, > > > > > > I reread this patch and tried to see it more in the context of the > > > > other patches and the existing code. The naming of the existing > > > > string tables doesn't help in getting this right, however I have a proposal: > > > > > > > > typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP > > > > typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD > > > > typec_find_power_role() to get to TYPEC_SINK/SOURCE > > > > > > > > and sometime, if the use should arise > > > > > > > > typec_find_data_role() to get to TYPEC_DEVICE/HOST > > > > > > > > I think it is fairly comprehensible, *_port_* concerns a > > > > capability and without *_port_* it is an actual state. Plus it > > > > matches the names of the constants. > > > > > > Well, there are now four things to consider: > > > > > > 1) the roles (power and data) the port is capable of supporting > > > 2) Try.SRC and Try.SNK, i.e. the preferred role > > > 3) the current roles > > > 4) the role(s) the user want's to limit the use of a port with DRP > > > ports > > > > > > The last one I don't know if it's relevant with these functions. > > > It's not information that we would get for example from firmware. > > > > > > I also don't think we need to use these functions with the current > > > roles the port is in. > > > > > > If the preferred role is "sink" or "source", so just like the power > > > role, we don't need separate function for it here. > > > > > > So isn't two functions all we need here: one for the power and one > > > for data role? > > > > Take power as an example, can we use one function to only look up > > typec_port_types[]? as capability(typec_port_type) and > > state(typec_role) are different enum, and the allowed strings are different. > > > > static const char * const typec_roles[] = { > > [TYPEC_SINK] = "sink", > > [TYPEC_SOURCE] = "source", > > }; > > > > static const char * const typec_port_types[] = { > > [TYPEC_PORT_SRC] = "source", > > [TYPEC_PORT_SNK] = "sink", > > [TYPEC_PORT_DRP] = "dual", > > }; > > Where out side the class code we need to convert the current role, the "state", > with these functions? Currently, the only call site is getting the preferred power role from firmware. Thanks Li Jun > > > Thanks, > > -- > heikki
Hi Jun, Sorry for the delay. On Thu, May 17, 2018 at 02:41:31PM +0000, Jun Li wrote: > Hi > > -----Original Message----- > > From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com] > > Sent: 2018??5??17?? 22:24 > > To: Jun Li <jun.li@nxp.com> > > Cc: Mats Karrman <mats.dev.list@gmail.com>; robh+dt@kernel.org; > > gregkh@linuxfoundation.org; linux@roeck-us.net; a.hajda@samsung.com; > > cw00.choi@samsung.com; shufan_lee@richtek.com; Peter Chen > > <peter.chen@nxp.com>; gsomlo@gmail.com; devicetree@vger.kernel.org; > > linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > > Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power > > and data config > > > > On Thu, May 17, 2018 at 02:05:41PM +0000, Jun Li wrote: > > > Hi Heikki, > > > > > > > > I reread this patch and tried to see it more in the context of the > > > > > other patches and the existing code. The naming of the existing > > > > > string tables doesn't help in getting this right, however I have a proposal: > > > > > > > > > > typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP > > > > > typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD > > > > > typec_find_power_role() to get to TYPEC_SINK/SOURCE > > > > > > > > > > and sometime, if the use should arise > > > > > > > > > > typec_find_data_role() to get to TYPEC_DEVICE/HOST > > > > > > > > > > I think it is fairly comprehensible, *_port_* concerns a > > > > > capability and without *_port_* it is an actual state. Plus it > > > > > matches the names of the constants. > > > > > > > > Well, there are now four things to consider: > > > > > > > > 1) the roles (power and data) the port is capable of supporting > > > > 2) Try.SRC and Try.SNK, i.e. the preferred role > > > > 3) the current roles > > > > 4) the role(s) the user want's to limit the use of a port with DRP > > > > ports > > > > > > > > The last one I don't know if it's relevant with these functions. > > > > It's not information that we would get for example from firmware. > > > > > > > > I also don't think we need to use these functions with the current > > > > roles the port is in. > > > > > > > > If the preferred role is "sink" or "source", so just like the power > > > > role, we don't need separate function for it here. > > > > > > > > So isn't two functions all we need here: one for the power and one > > > > for data role? > > > > > > Take power as an example, can we use one function to only look up > > > typec_port_types[]? as capability(typec_port_type) and > > > state(typec_role) are different enum, and the allowed strings are different. > > > > > > static const char * const typec_roles[] = { > > > [TYPEC_SINK] = "sink", > > > [TYPEC_SOURCE] = "source", > > > }; > > > > > > static const char * const typec_port_types[] = { > > > [TYPEC_PORT_SRC] = "source", > > > [TYPEC_PORT_SNK] = "sink", > > > [TYPEC_PORT_DRP] = "dual", > > > }; > > > > Where out side the class code we need to convert the current role, the "state", > > with these functions? > > Currently, the only call site is getting the preferred power role from firmware. My point was that if we used enum typec_port_type with the prefered role, we could use the helper for power role with prefered role. But never mind. Thanks,
Hi Heikki, > -----Original Message----- > From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com] > Sent: 2018年5月21日 21:13 > To: Jun Li <jun.li@nxp.com> > Cc: Mats Karrman <mats.dev.list@gmail.com>; robh+dt@kernel.org; > gregkh@linuxfoundation.org; linux@roeck-us.net; a.hajda@samsung.com; > cw00.choi@samsung.com; shufan_lee@richtek.com; Peter Chen > <peter.chen@nxp.com>; gsomlo@gmail.com; devicetree@vger.kernel.org; > linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power > and data config > > Hi Jun, > > Sorry for the delay. > > On Thu, May 17, 2018 at 02:41:31PM +0000, Jun Li wrote: > > Hi > > > -----Original Message----- > > > From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com] > > > Sent: 2018??5??17?? 22:24 > > > To: Jun Li <jun.li@nxp.com> > > > Cc: Mats Karrman <mats.dev.list@gmail.com>; robh+dt@kernel.org; > > > gregkh@linuxfoundation.org; linux@roeck-us.net; a.hajda@samsung.com; > > > cw00.choi@samsung.com; shufan_lee@richtek.com; Peter Chen > > > <peter.chen@nxp.com>; gsomlo@gmail.com; devicetree@vger.kernel.org; > > > linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > > > Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic > > > port power and data config > > > > > > On Thu, May 17, 2018 at 02:05:41PM +0000, Jun Li wrote: > > > > Hi Heikki, > > > > > > > > > > I reread this patch and tried to see it more in the context of > > > > > > the other patches and the existing code. The naming of the > > > > > > existing string tables doesn't help in getting this right, however I have > a proposal: > > > > > > > > > > > > typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP > > > > > > typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD > > > > > > typec_find_power_role() to get to TYPEC_SINK/SOURCE > > > > > > > > > > > > and sometime, if the use should arise > > > > > > > > > > > > typec_find_data_role() to get to TYPEC_DEVICE/HOST > > > > > > > > > > > > I think it is fairly comprehensible, *_port_* concerns a > > > > > > capability and without *_port_* it is an actual state. Plus it > > > > > > matches the names of the constants. > > > > > > > > > > Well, there are now four things to consider: > > > > > > > > > > 1) the roles (power and data) the port is capable of supporting > > > > > 2) Try.SRC and Try.SNK, i.e. the preferred role > > > > > 3) the current roles > > > > > 4) the role(s) the user want's to limit the use of a port with > > > > > DRP ports > > > > > > > > > > The last one I don't know if it's relevant with these functions. > > > > > It's not information that we would get for example from firmware. > > > > > > > > > > I also don't think we need to use these functions with the > > > > > current roles the port is in. > > > > > > > > > > If the preferred role is "sink" or "source", so just like the > > > > > power role, we don't need separate function for it here. > > > > > > > > > > So isn't two functions all we need here: one for the power and > > > > > one for data role? > > > > > > > > Take power as an example, can we use one function to only look up > > > > typec_port_types[]? as capability(typec_port_type) and > > > > state(typec_role) are different enum, and the allowed strings are > different. > > > > > > > > static const char * const typec_roles[] = { > > > > [TYPEC_SINK] = "sink", > > > > [TYPEC_SOURCE] = "source", > > > > }; > > > > > > > > static const char * const typec_port_types[] = { > > > > [TYPEC_PORT_SRC] = "source", > > > > [TYPEC_PORT_SNK] = "sink", > > > > [TYPEC_PORT_DRP] = "dual", > > > > }; > > > > > > Where out side the class code we need to convert the current role, > > > the "state", with these functions? > > > > Currently, the only call site is getting the preferred power role from firmware. > > My point was that if we used enum typec_port_type with the prefered role, we > could use the helper for power role with prefered role. But never mind. Got your point. So let's follow Mats's suggestion on this, I will update a new version. Thanks Li Jun > > > Thanks, > > -- > heikki
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c index 53df10d..5981e18 100644 --- a/drivers/usb/typec/class.c +++ b/drivers/usb/typec/class.c @@ -9,6 +9,7 @@ #include <linux/device.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/property.h> #include <linux/slab.h> #include <linux/usb/typec.h> #include <linux/usb/typec_mux.h> @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = { [TYPEC_PORT_DRP] = "dual", }; +static const char * const typec_data_types[] = { + [TYPEC_PORT_DFP] = "host", + [TYPEC_PORT_UFP] = "device", + [TYPEC_PORT_DRD] = "dual", +}; + static const char * const typec_port_types_drp[] = { [TYPEC_PORT_SRC] = "dual [source] sink", [TYPEC_PORT_SNK] = "dual source [sink]", @@ -1252,6 +1259,51 @@ void typec_set_pwr_opmode(struct typec_port *port, } EXPORT_SYMBOL_GPL(typec_set_pwr_opmode); +/** + * typec_find_power_type - Get the typec port power type + * @name: port type string + * + * This routine is used to find the typec_port_type by its string name. + * + * Returns typec_port_type if success, otherwise negative error code. + */ +int typec_find_power_type(const char *name) +{ + return match_string(typec_port_types, ARRAY_SIZE(typec_port_types), + name); +} +EXPORT_SYMBOL_GPL(typec_find_power_type); + +/** + * typec_find_preferred_role - Find the typec drp port preferred power role + * @name: power role string + * + * This routine is used to find the typec_role by its string name of + * preferred power role(Try.SRC or Try.SNK). + * + * Returns typec_role if success, otherwise negative error code. + */ +int typec_find_preferred_role(const char *name) +{ + return match_string(typec_roles, ARRAY_SIZE(typec_roles), name); +} +EXPORT_SYMBOL_GPL(typec_find_preferred_role); + +/** + * typec_find_data_type - Get the typec port data capability + * @name: data type string + * + * This routine is used to find the typec_port_data by its string name. + * + * Returns typec_port_data if success, otherwise negative error code. + */ +int typec_find_data_type(const char *name) +{ + return match_string(typec_data_types, ARRAY_SIZE(typec_data_types), + name); +} +EXPORT_SYMBOL_GPL(typec_find_data_type); + /* ------------------------------------------ */ /* API for Multiplexer/DeMultiplexer Switches */ diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h index 672b39b..00c93e7 100644 --- a/include/linux/usb/typec.h +++ b/include/linux/usb/typec.h @@ -267,4 +267,7 @@ int typec_set_orientation(struct typec_port *port, enum typec_orientation orientation); int typec_set_mode(struct typec_port *port, int mode); +int typec_find_power_type(const char *name); +int typec_find_preferred_role(const char *name); +int typec_find_data_type(const char *name); #endif /* __LINUX_USB_TYPEC_H */
This patch adds 3 APIs to get the typec port power and data type, and preferred power role by its name string. Signed-off-by: Li Jun <jun.li@nxp.com> --- drivers/usb/typec/class.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/usb/typec.h | 3 +++ 2 files changed, 55 insertions(+)