Greg Kroah-Hartman wrote: > On Sat, Jul 25, 2020 at 10:51:07AM +0000, Thinh Nguyen wrote: >> 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://urldefense.com/v3/__https://lore.kernel.org/linux-usb/f68f395c-0821-9e34-f6cf-75fa60c9a35b@xxxxxxxxxxxx/T/*mac3a4d0b1c02866e3bdbd6809506fbbee8bf84c2__;Iw!!A4F2R9G_pg!JvGMuHeacSdTRkN-1SCUsebqfPskVCdtFj6xUkMuvtkwBcGK5N4v5nukhHZKI-lKdFEE$ >> >> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/0818e876-ea5b-9ebc-7427-8e26b627e6b4@xxxxxxxxxxxx/T/*m87191333cb10a7f0caaf533bfa198724d33c2519__;Iw!!A4F2R9G_pg!JvGMuHeacSdTRkN-1SCUsebqfPskVCdtFj6xUkMuvtkwBcGK5N4v5nukhHZKI0SFG1Fk$ >> >> >>>> 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://urldefense.com/v3/__https://lore.kernel.org/linux-usb/3bc4fb6a0d7c1a9ea4d5d9384ade26c9066c87d0.1595631457.git.thinhn@xxxxxxxxxxxx/T/*u__;Iw!!A4F2R9G_pg!JvGMuHeacSdTRkN-1SCUsebqfPskVCdtFj6xUkMuvtkwBcGK5N4v5nukhHZKI6d-EKTE$ >> >>> >>> >>>> + 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. > I don't care about the properties, that should be independant of the > function call made to determine this information. Don't let the data > formats force you to write horrible code :) Ok. Will revise. Thanks, Thinh
