[PATCH] usb: gadget: dwc2_udc_otg: implement pullup()

[Marek Vasut]

On 1/10/23 14:30, Harald Seiler wrote:
> Hi,
> 
> On Tue, 2023-01-10 at 10:11 +0100, Mattijs Korpershoek wrote:
>> Pullup is used by the usb framework in order to do software-controlled
>> usb_gadget_connect() and usb_gadget_disconnect().
>>
>> Implement pullup() for dwc2 using the SOFT_DISCONNECT bit in the dctl
>> register.
>>
>> This is especially useful when a gadget disconnection is initiated but
>> no board_usb_cleanup() is called.
>>
>> Signed-off-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>
>> ---
>> On some boards using the dwc2 controller, like the Khadas VIM3L, whenever
>> usb_gadget_release() is called, the D+ and D- lines are in an unknown state.
>>
>> Because of that, the host can't detect usb disconnection.
>>
>> It was attempted to be be fixed with [1] but ended up doing the gadget disconnection
>> too early, creating issues on NXP-based boards which use uuu [2].
>>
>> By implementing pullup() in the controller driver, we ensure that the disconnection will
>> only be done when the framework calls usb_gadget_disconnect().
>>
>> [1] https://lore.kernel.org/all/20220728-reset-usb-controller-v2-1-ef7657ce78b1@baylibre.com/
>> [2] https://lore.kernel.org/all/20230107164807.3597020-1-dario.binacchi@amarulasolutions.com/
>> ---
>>   drivers/usb/gadget/dwc2_udc_otg.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/dwc2_udc_otg.c b/drivers/usb/gadget/dwc2_udc_otg.c
>> index 77988f78ab30..93ed9712d18a 100644
>> --- a/drivers/usb/gadget/dwc2_udc_otg.c
>> +++ b/drivers/usb/gadget/dwc2_udc_otg.c
>> @@ -236,6 +236,21 @@ static int udc_enable(struct dwc2_udc *dev)
>>   	return 0;
>>   }
>>   
>> +static int dwc2_gadget_pullup(struct usb_gadget *g, int is_on)
>> +{
>> +	unsigned int uTemp;
> 
> Just some minor points about style...
> 
> I think the `u32` type should be used for 32-bit registers instead.
> More explicit and no room for accidentally choosing the wrong size.
> 
> Also, U-Boot and Linux don't use hungarian notation for variable names,
> just call it `tmp` or even better `dctl` to be explicit.
> 
>> +
>> +	is_on = !!is_on;
> 
> This is superfluous, the if condition works either way.

clrsetbits_le32() would turn the above into a oneliner.