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

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

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@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"?
> 
> 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@synopsys.com/T/#mac3a4d0b1c02866e3bdbd6809506fbbee8bf84c2
> 
> https://lore.kernel.org/linux-usb/0818e876-ea5b-9ebc-7427-8e26b627e6b4@synopsys.com/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@synopsys.com/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.

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 :)

thanks,

greg k-h