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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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@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://urldefense.com/v3/__https://lore.kernel.org/linux-usb/f68f395c-0821-9e34-f6cf-75fa60c9a35b@synopsys.com/T/*mac3a4d0b1c02866e3bdbd6809506fbbee8bf84c2__;Iw!!A4F2R9G_pg!JvGMuHeacSdTRkN-1SCUsebqfPskVCdtFj6xUkMuvtkwBcGK5N4v5nukhHZKI-lKdFEE$
>>
>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/0818e876-ea5b-9ebc-7427-8e26b627e6b4@synopsys.com/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@synopsys.com/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