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@xxxxxxxxx] >>>> Sent: 2018???5???12??? 3:56 >>>> To: Jun Li <jun.li@xxxxxxx>; robh+dt@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; >>>> heikki.krogerus@xxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx >>>> Cc: a.hajda@xxxxxxxxxxx; cw00.choi@xxxxxxxxxxx; >>>> shufan_lee@xxxxxxxxxxx; Peter Chen <peter.chen@xxxxxxx>; >>>> gsomlo@xxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; >>>> dl-linux-imx <linux-imx@xxxxxxx> >>>> 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@xxxxxxx> >>>>> --- >>>>> 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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
