Re: [PATCH v3 07/12] usb: common: Add function to get num_lanes and transfer rate

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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@synopsys.com>
> ---
> 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