Re: [PATCH 7/8] mfd: omap-usb-host: Add pbias regulator support

[Thomas Richard <thomas.richard@bootlin.com>]

Hello Lee,

On 3/31/26 6:55 PM, Lee Jones wrote:
> On Mon, 23 Mar 2026, Thomas Richard wrote:
> 
>> Add pbias regulator support to enable SIM_VDDS supply and unlock USB I/O
>> cell. Previously, this was handled by the bootloader, now the kernel can
>> take responsibility for managing the PBIAS regulator, ensuring correct
>> operation regardless of the bootloader.
>>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>> ---
>>  drivers/mfd/omap-usb-host.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
>> index ac974285be341fa579ef198d1893b77af428b5f8..9e254e00183e940b775d5bde6e891f0d26af27b0 100644
>> --- a/drivers/mfd/omap-usb-host.c
>> +++ b/drivers/mfd/omap-usb-host.c
>> @@ -15,6 +15,9 @@
>>  #include <linux/pm_runtime.h>
>>  #include <linux/of.h>
>>  #include <linux/of_platform.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/string_choices.h>
>> +
> 
> ?
> 
>>  
>>  #include "omap-usb.h"
>>  
>> @@ -95,6 +98,8 @@ struct usbhs_hcd_omap {
>>  	struct usbhs_omap_platform_data	*pdata;
>>  
>>  	u32				usbhs_rev;
>> +
>> +	struct regulator		*pbias;
>>  };
>>  /*-------------------------------------------------------------------------*/
>>  
>> @@ -270,6 +275,25 @@ static bool is_ohci_port(enum usbhs_omap_port_mode pmode)
>>  	}
>>  }
>>  
>> +static int omap_usbhs_set_pbias(struct device *dev, bool power_on)
>> +{
>> +	struct usbhs_hcd_omap *omap = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	if (!omap->pbias)
>> +		return 0;
>> +
>> +	if (power_on)
>> +		ret = regulator_enable(omap->pbias);
>> +	else
>> +		ret = regulator_disable(omap->pbias);
>> +
>> +	if (ret)
>> +		dev_err(dev, "pbias reg %s failed\n", str_enable_disable(power_on));
>> +
>> +	return ret;
>> +}
>> +
>>  static int usbhs_runtime_resume(struct device *dev)
>>  {
>>  	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
>> @@ -278,6 +302,10 @@ static int usbhs_runtime_resume(struct device *dev)
>>  
>>  	dev_dbg(dev, "usbhs_runtime_resume\n");
>>  
>> +	r = omap_usbhs_set_pbias(dev, true);
>> +	if (r)
>> +		return r;
>> +
>>  	omap_tll_enable(pdata);
>>  
>>  	if (!IS_ERR(omap->ehci_logic_fck))
>> @@ -355,7 +383,7 @@ static int usbhs_runtime_suspend(struct device *dev)
>>  
>>  	omap_tll_disable(pdata);
>>  
>> -	return 0;
>> +	return omap_usbhs_set_pbias(dev, false);
>>  }
>>  
>>  static unsigned omap_usbhs_rev1_hostconfig(struct usbhs_hcd_omap *omap,
>> @@ -564,6 +592,11 @@ static int usbhs_omap_probe(struct platform_device *pdev)
>>  
>>  	omap->pdata = pdata;
>>  
>> +	omap->pbias = devm_regulator_get_optional(dev, "pbias");
>> +	if (IS_ERR(omap->pbias))
>> +		return dev_err_probe(dev, PTR_ERR(omap->pbias),
>> +				     "unable to get pbias regulator\n");
> 
> You need to check for '-ENODEV' here or you are ignoring the optional part.
> 
>> +
>>  	/* Initialize the TLL subsystem */
>>  	omap_tll_init(pdata);
>>  
>> @@ -759,6 +792,10 @@ static int usbhs_omap_probe(struct platform_device *pdev)
>>  	}
>>  
>>  initialize:
>> +	ret = omap_usbhs_set_pbias(dev, true);
>> +	if (ret)
>> +		goto err_mem;
> 
> Since this regulator is also managed by 'usbhs_runtime_resume' and
> 'usbhs_runtime_suspend', could manually enabling it here in probe and
> disabling it in remove interfere with the reference counting during runtime
> PM transitions? Should we consider relying entirely on runtime PM to manage
> its state?
> 

Thanks a lot for the review. I already sent a v2, do not spend your time
on this version. This comment is also valid for v2 (and the headers
one), but I fixed -ENODEV in v2.

Best Regards,
Thomas