Re: [RFC PATCH 02/14] usb: gadget: Add callback to set lane and transfer rate

From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To: Alan Stern <stern@rowland.harvard.edu>, Felipe Balbi <balbi@kernel.org>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	Roger Quadros <rogerq@ti.com>, zhengbin <zhengbin13@huawei.com>,
	John Youn <John.Youn@synopsys.com>
Subject: Re: [RFC PATCH 02/14] usb: gadget: Add callback to set lane and transfer rate
Date: Thu, 12 Dec 2019 22:33:14 +0000	[thread overview]
Message-ID: <f315858e-4a47-aeaa-1976-577ff781bf0e@synopsys.com> (raw)
In-Reply-To: <Pine.LNX.4.44L0.1912121047120.14053-100000@netrider.rowland.org>

Hi,

Alan Stern wrote:
> On Thu, 12 Dec 2019, Felipe Balbi wrote:
>
>> Hi,
>>
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> Introduce gadget opts udc_set_sublink_speed callback to set the lane
>>> count and transfer rate (in lane speed mantissa of Gbps) for SuperSpeed
>>> Plus capable gadgets. In the same way udc_set_speed, this function can
>>> control the gadget's sublink attributes for SuperSpeed Plus.
>>>
>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>> ---
>>>   drivers/usb/gadget/composite.c           |  2 ++
>>>   drivers/usb/gadget/legacy/mass_storage.c |  2 ++
>> I would rather not add new features to the legacy gadgets and focus on
>> our configfs interface for anything new. Moreover, using the feature
>> you introduced could, arguably, be done as a separate patch.
>>
>>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>>> index 3b4f67000315..a4de5a8c0f19 100644
>>> --- a/drivers/usb/gadget/composite.c
>>> +++ b/drivers/usb/gadget/composite.c
>>> @@ -2353,6 +2353,8 @@ int usb_composite_probe(struct usb_composite_driver *driver)
>>>   	gadget_driver->function =  (char *) driver->name;
>>>   	gadget_driver->driver.name = driver->name;
>>>   	gadget_driver->max_speed = driver->max_speed;
>>> +	gadget_driver->max_lane_count = driver->max_lane_count;
>>> +	gadget_driver->max_lsm = driver->max_lsm;
>>>   
>>>   	return usb_gadget_probe_driver(gadget_driver);
>>>   }
>>> diff --git a/drivers/usb/gadget/legacy/mass_storage.c b/drivers/usb/gadget/legacy/mass_storage.c
>>> index f18f77584fc2..a0912c5afffc 100644
>>> --- a/drivers/usb/gadget/legacy/mass_storage.c
>>> +++ b/drivers/usb/gadget/legacy/mass_storage.c
>>> @@ -223,6 +223,8 @@ static struct usb_composite_driver msg_driver = {
>>>   	.name		= "g_mass_storage",
>>>   	.dev		= &msg_device_desc,
>>>   	.max_speed	= USB_SPEED_SUPER_PLUS,
>>> +	.max_lane_count	= 2,
>>> +	.max_lsm	= 10,
>> Right, as mentioned, I'd prefer not touch the legacy gadgets. But in any
>> case, why is it so that the gadget is telling you about max lane count
>> and lsm? That should be abstracted away from the gadget driver. Gadget
>> driver shouldn't have knowledge of number of lanes because, at the end
>> of the day, that doesn't really change anything in practice. Unlike HS
>> vs SS which changes a bunch of things.
> I agree completely.  Furthermore, it isn't at all clear where those two
> numbers came from.  Why would g-mass-storage care that lane_count <= 2
> and lsm <= 10?
>
> Alan Stern
>

Right, I'll remove this and update according to Felipe's suggestions.

Thanks,
Thinh