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

[Marek Vasut]

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. 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 ?