Greg Kroah-Hartman wrote: > On Fri, Jul 24, 2020 at 04:39:11PM -0700, Thinh Nguyen wrote: >> Add a new common function to parse maximum_speed property string for >> the specified number of lanes and transfer rate. >> >> Signed-off-by: Thinh Nguyen <thinhn@xxxxxxxxxxxx> >> --- >> Changes in v3: >> - Add new function to parse "maximum-speed" for lanes and transfer rate >> - Remove separate functions getting num_lanes and transfer rate properties > No, why have you split this into a single function that for some reason > "can not fail"? This patch was updated to read from a single property "maximum-speed" to get the device speed, gen, and number of lanes. So we use a single function to parse this property. This came up from on Rob's comments: https://lore.kernel.org/linux-usb/f68f395c-0821-9e34-f6cf-75fa60c9a35b@xxxxxxxxxxxx/T/#mac3a4d0b1c02866e3bdbd6809506fbbee8bf84c2 https://lore.kernel.org/linux-usb/0818e876-ea5b-9ebc-7427-8e26b627e6b4@xxxxxxxxxxxx/T/#m87191333cb10a7f0caaf533bfa198724d33c2519 > >> Changes in v2: >> - New commit >> >> drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++--- >> include/linux/usb/ch9.h | 25 ++++++++++++++++++++++++ >> 2 files changed, 69 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c >> index 1433260d99b4..53b4950c49e4 100644 >> --- a/drivers/usb/common/common.c >> +++ b/drivers/usb/common/common.c >> @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed) >> } >> EXPORT_SYMBOL_GPL(usb_speed_string); >> >> -enum usb_device_speed usb_get_maximum_speed(struct device *dev) >> +void usb_get_maximum_speed_and_num_lanes(struct device *dev, >> + enum usb_device_speed *speed, >> + enum usb_phy_gen *gen, >> + u8 *num_lanes) > What is wrong with just having multiple different functions instead? See my comment above. > >> { >> const char *maximum_speed; >> + enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN; >> + enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN; >> + u8 matched_num_lanes = 0; >> int ret; >> >> ret = device_property_read_string(dev, "maximum-speed", &maximum_speed); >> if (ret < 0) >> - return USB_SPEED_UNKNOWN; >> + goto done; >> >> ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed); >> + if (ret >= 0) { >> + matched_speed = ret; >> + goto done; >> + } >> + >> + if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) { >> + matched_speed = USB_SPEED_SUPER_PLUS; >> + matched_gen = USB_PHY_GEN_2; >> + matched_num_lanes = 2; >> + } else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) { >> + matched_speed = USB_SPEED_SUPER_PLUS; >> + matched_gen = USB_PHY_GEN_2; >> + matched_num_lanes = 1; >> + } else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) { > Where are all of these device properties documented? It's coming from a single property "maximum-speed", documented in patch 6/12 "usb: devicetree: Include USB SSP Gen X x Y" https://lore.kernel.org/linux-usb/3bc4fb6a0d7c1a9ea4d5d9384ade26c9066c87d0.1595631457.git.thinhn@xxxxxxxxxxxx/T/#u > > > >> + matched_speed = USB_SPEED_SUPER_PLUS; >> + matched_gen = USB_PHY_GEN_1; >> + matched_num_lanes = 2; >> + } >> + >> +done: >> + if (speed) >> + *speed = matched_speed; >> + >> + if (num_lanes) >> + *num_lanes = matched_num_lanes; >> + >> + if (gen) >> + *gen = matched_gen; > > >> +} >> +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes); >> + >> +enum usb_device_speed usb_get_maximum_speed(struct device *dev) >> +{ >> + enum usb_device_speed speed; >> >> - return (ret < 0) ? USB_SPEED_UNKNOWN : ret; >> + usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL); > Here's an example of why this isn't a good function. > > It isn't self-describing. Why do you pass in 3 pointers yet the name > only contains 2 things? Right... I can revise. > > usb_get_maximum_speed_and_num_lanes_and_generation() shows that this is > not a good thing, right? > > Again, 3 different functions please. Should we have 3 separate properties to describe the device? If so, this was done in v2 of this series, but Rob has different ideas. Please advise. > >> + return speed; >> } >> EXPORT_SYMBOL_GPL(usb_get_maximum_speed); >> >> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h >> index 01191649a0ad..46cfd72e7082 100644 >> --- a/include/linux/usb/ch9.h >> +++ b/include/linux/usb/ch9.h >> @@ -57,6 +57,13 @@ enum usb_link_protocol { >> USB_LP_SSP = 1, >> }; >> >> +/* USB phy signaling rate gen */ >> +enum usb_phy_gen { >> + USB_PHY_GEN_UNKNOWN, >> + USB_PHY_GEN_1, >> + USB_PHY_GEN_2, >> +}; >> + >> /** >> * struct usb_sublink_speed - sublink speed attribute >> * @id: sublink speed attribute ID (SSID) >> @@ -95,6 +102,24 @@ extern const char *usb_ep_type_string(int ep_type); >> */ >> extern const char *usb_speed_string(enum usb_device_speed speed); >> >> +/** >> + * usb_get_maximum_speed_and_num_lanes - Get maximum requested speed and number >> + * of lanes for a given USB controller. > You forgot that it also determines the "gen". Ok. Will fix. Thanks! Thinh
