Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

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,

-- 
heikki