[PATCH] usb: ehci-mx6: replace regulator_set_enable with *_if_allowed

[Eugen Hristev]

On 5/4/23 00:52, Marek Vasut wrote:
> On 5/2/23 18:13, Tim Harvey wrote:
>> On Mon, May 1, 2023 at 11:51 PM Eugen Hristev
>> <eugen.hristev at collabora.com> wrote:
>>>
>>> regulator_set_enable_if_allowed already handles cases when the
>>> regulator is already enabled, or already disabled, and does not
>>> treat these as errors.
>>> With this change, the driver can work correctly even if the regulator
>>> is already taken or already disabled by another consumer.
>>>
>>> Signed-off-by: Eugen Hristev <eugen.hristev at collabora.com>
>>> ---
>>> Hi Tim,
>>>
>>> I have not tested this as I do not have a mx6 board. can you please
>>> try it ?
>>>
>>> Thanks,
>>> Eugen
>>
>> Eugen,
>>
>> This does resolve the issue that occurs after your refcount series [1].
>>
>> However after thinking about this more and seeing Marek's responses
>> this wasn't an issue of multiple consumers sharing the same regulator.
>> Instead this particular issue was that the vbus regulator is getting
>> enabled twice without being disabled.

This kind of issues can be uncovered when we add the refcounter :)
On one hand I am happy to uncover such bugs.
However, this leads me to wonder, even without a refcount, even without 
my series, enabling a regulator that is already enabled should return 
-EALREADY

On the other hand, it appears it will take longer to get the refcount 
applied though.

  Adding a print in
>> regulator_set_enable shows me:
>> --- a/drivers/power/regulator/regulator-uclass.c
>> +++ b/drivers/power/regulator/regulator-uclass.c
>> @@ -165,6 +165,7 @@ int regulator_set_enable(struct udevice *dev, bool 
>> enable)
>>          struct dm_regulator_uclass_plat *uc_pdata;
>>          int ret, old_enable = 0;
>>
>> +printf("%s %s %s\n", __func__, dev->name, enable ? "enable" : 
>> "disable");
>>          if (!ops || !ops->set_enable)
>>                  return -ENOSYS;
>>
>> u-boot=> usb start
>> starting USB...
>> Bus usb at 32e40000: regulator_set_enable regulator-usb-otg1 enable
>> ^^^ from ehci_usb_probe
>> Bus usb at 32e50000: regulator_set_enable regulator-usb-otg2 enable
>> ^^^ from ehci_usb_probe
>> regulator_set_enable regulator-usb-otg2 enable
>> ^^^ from mx6_init_after_reset - 2nd enable without a disable
>>
>> So while I think this patch does make sense to cover the case where a
>> regulator could be shared
> 
> Does such a case really still exist after the discovery you made above ?
> 
>> there probably is a follow-on patch needed
>> to balance the regulator calls (unrelated to your series).
>>
>> Marek,
>>
>> Should vbus regulator enable really be in init_after_reset call?
> 
> Yes, but I am not entirely convinced the VBUS should be enabled in 
> ehci_usb_probe(), because in ehci_usb_probe() resp. in ehci_register() 
> which is called at the end, we might detect that the port is configured 
> as PERIPHERAL and we don't want to enable VBUS in that case .
> 
> So I suspect regulator_set_enable() should rather be dropped from 
> ehci_usb_probe() . What do you think ?