Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

On 02/22/2018 07:27 PM, Alan Stern wrote:
> On Thu, 22 Feb 2018, Amelie DELAUNAY wrote:
> 
>>>>>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>>>> @@ -19,6 +19,7 @@ Optional properties:
>>>>>>>      - phys : phandle + phy specifier pair
>>>>>>>      - phy-names : "usb"
>>>>>>>      - resets : phandle + reset specifier pair
>>>>>>> + - vbus-supply : phandle of regulator supplying vbus
>>>>>>>     
>>>>>>
>>>>>> Can platforms have more than one regulator e.g. one regulator per port?
>>>>>>
>>>>>
>>>>> I imagine that yes, platforms could have one regulator per port.
>>>>> Regulator consumers bindings impose a <name>-supply property per
>>>>> regulator, so, what do you think about :
>>>>> vbus0-supply for port#0
>>>>> vbus1-supply for port#1
>>>>> ...
>>>>> vbusN-supply for port#N
>>>>>
>>>>> And then in probe, allocate 'struct regulator *vbus_supplies' with a
>>>>> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
>>>>> regulator *)'.
>>>>> And loop to get optional regulator vbus0, vbus1,..., vbusN.
>>>>> And then enable/disable the corresponding regulator in
>>>>> ehci_platform_port_power thanks to portnum.
>>>>
>>>> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
>>>> If there isn't a single platform needing it we could probably do without it
>>>> but the DT binding must be scalable to add this feature in the future.
>>>
>>> I agree that for now there don't seem to be any platforms requiring
>>> more than one regulator, but this should be implemented in a way that
>>> could be expanded if necessary.
>>>
>>> Anyway, the basic idea is reasonable.  I don't know to what extent
>>> people want to power-off their EHCI ports, but if they do then we ought
>>> to turn off external regulators at the same time.
>>>
>>> Is there a real-life use case for this?
>>>
>>> Alan Stern
>>>
>>
>> On my setup I have the following:
>>
>>                    regulator_____vbus
>>    _________________             \
>> | EHCI controller |-port0-----[USB connector]
>> |_________________|-port1-----X
>>
>> So, I have one regulator only for port0. But I could I have one more if
>> port1 was connected. My current regulator could also supplies port1.
>>
>> To allocate a vbus_supplies array depending on N_PORTS, I have to move
>> this initialization from probe to ehci_platform_reset, after ehci_setup
>> is done.
>> Then, I have to define each regulator id depending on the port number.
>> This imposes a binding like
>> - portN_vbus-supply : phandle of regulator supplying vbus for port N
>> But I don't know if we can describe it like this in dt-bindings ?
>>
>> &ehci {
>> 	...
>> 	port0_vbus-supply = <&vbus_reg>;
>> 	port1_vbus-supply = <&vbus_reg>; //Could be another regulator, or not
>> specified if port is not connected.
>> 	...
>> };
>>
>> Is it ok to move vbus_supplies initialization in ehci_platform_reset ?
> 
> Yes, it's okay to move the code if you need to.
> 
> However, I can not speak on the DT implications.  Someone who knows
> more about it should chime in.
> 
> Alan Stern
> 

Thanks Alan,

I'm going to send a v3 including all these changes to ease review.

Regards,
Amelie