Re: [PATCH v2 10/11] mfd: omap-usb-host: Add pbias regulator support

[Lee Jones]

On Mon, 30 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@xxxxxxxxxxx>
> ---
>  drivers/mfd/omap-usb-host.c | 55 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
> index 17a54f0087c3..907fb614d464 100644
> --- a/drivers/mfd/omap-usb-host.c
> +++ b/drivers/mfd/omap-usb-host.c
> @@ -15,6 +15,8 @@
>  #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 +97,8 @@ struct usbhs_hcd_omap {
>  	struct usbhs_omap_platform_data	*pdata;
>  
>  	u32				usbhs_rev;
> +
> +	struct regulator		*pbias;
>  };
>  /*-------------------------------------------------------------------------*/
>  
> @@ -334,26 +338,60 @@ static int usbhs_clocks_enable(struct device *dev, bool enable)
>  	return r;
>  }
>  
> +static int omap_usbhs_set_pbias(struct device *dev, bool power_on)

I think this is a little over-engineered as well.

I'm willing to bet that it saves ~0 LoC.

> +{
> +	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);
>  	struct usbhs_omap_platform_data	*pdata = omap->pdata;
> +	int ret;
>  
>  	omap_tll_enable(pdata);
>  
> -	return usbhs_clocks_enable(dev, true);
> +	ret = usbhs_clocks_enable(dev, true);
> +	if (ret)
> +		return ret;
> +
> +	return omap_usbhs_set_pbias(dev, true);

... but if you _insist_ on doing it this way, at least define the true
to something meaningful.

>  }
>  
>  static int usbhs_runtime_suspend(struct device *dev)
>  {
>  	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
>  	struct usbhs_omap_platform_data	*pdata = omap->pdata;
> +	int ret;
>  
>  	usbhs_clocks_enable(dev, false);
>  
>  	omap_tll_disable(pdata);
>  
> +	ret = omap_usbhs_set_pbias(dev, false);
> +	if (ret)
> +		goto err;
> +
>  	return 0;
> +
> +err:
> +	omap_tll_enable(pdata);
> +	usbhs_clocks_enable(dev, true);
> +	return ret;
>  }
>  
>  static unsigned omap_usbhs_rev1_hostconfig(struct usbhs_hcd_omap *omap,
> @@ -562,6 +600,15 @@ 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)) {
> +		if (PTR_ERR(omap->pbias) == -ENODEV)
> +			omap->pbias = NULL;
> +		else
> +			return dev_err_probe(dev, PTR_ERR(omap->pbias),
> +					     "unable to get pbias regulator\n");
> +	}
> +
>  	/* Initialize the TLL subsystem */
>  	omap_tll_init(pdata);
>  
> @@ -757,6 +804,10 @@ static int usbhs_omap_probe(struct platform_device *pdev)
>  	}
>  
>  initialize:
> +	ret = omap_usbhs_set_pbias(dev, true);
> +	if (ret)
> +		goto err_mem;
> +
>  	omap_usbhs_init(dev);
>  
>  	if (dev->of_node) {
> @@ -804,6 +855,8 @@ static void usbhs_omap_remove(struct platform_device *pdev)
>  		of_platform_depopulate(&pdev->dev);
>  	else
>  		device_for_each_child(&pdev->dev, NULL, usbhs_omap_remove_child);
> +
> +	omap_usbhs_set_pbias(&pdev->dev, false);
>  }
>  
>  static const struct dev_pm_ops usbhsomap_dev_pm_ops = {
> 
> -- 
> 2.53.0
> 

-- 
Lee Jones