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"?
> 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?
> {
> 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?
> + 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?
usb_get_maximum_speed_and_num_lanes_and_generation() shows that this is
not a good thing, right?
Again, 3 different functions please.
> + 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".
thanks,
greg k-h
