Re: [PATCH 1/2] usb: bcma: control VBUS via a regulator instead of a GPIO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] usb: bcma: ABI breakage by removing vcc-gpio DT backward compatibility
- [High] usb: bcma: fatal probe failure on missing optional regulator
- [High] usb: bcma: regulator enable count leak on error and remove
- [Low] build: syntax error due to missing closing brace
--

commit 2da0c63e158c66b33d97e363d8f72cf580a7db29
Author: Rosen Penev <rosenp@gmail.com>

usb: bcma: control VBUS via a regulator instead of a GPIO

This commit replaces the custom vcc-gpio GPIO driving in bcma-hcd with the
standard regulator framework. It uses devm_regulator_get_optional so shared
regulators are properly reference-counted, avoiding conflicts on boards where
multiple controllers share the same GPIO.

> diff --git a/drivers/usb/host/bcma-hcd.c b/drivers/usb/host/bcma-hcd.c
> index 519386255886c..394ba9e5a7f77 100644
> --- a/drivers/usb/host/bcma-hcd.c
> +++ b/drivers/usb/host/bcma-hcd.c

[ ... ]

> @@ -405,11 +411,14 @@ static int bcma_hcd_probe(struct bcma_device *core)
>  		return -ENOMEM;
>  	usb_dev->core = core;
>  
> -	usb_dev->gpio_desc = devm_gpiod_get_optional(&core->dev, "vcc",
> -						     GPIOD_OUT_HIGH);
> -	if (IS_ERR(usb_dev->gpio_desc))
> -		return dev_err_probe(&core->dev, PTR_ERR(usb_dev->gpio_desc),
> -				     "error obtaining VCC GPIO");
> +	usb_dev->regulator = devm_regulator_get_optional(&core->dev, "vbus");

[Severity: Critical]
Does replacing the vcc property with vbus drop support for existing devices
using older Device Trees? It seems like this might break backward
compatibility for any board still relying on the legacy vcc-gpio binding.

> +	if (IS_ERR(usb_dev->regulator)) {
> +		return PTR_ERR(usb_dev->regulator);

[Severity: High]
Does this unconditionally fail the probe when an optional regulator is not
present? devm_regulator_get_optional() returns -ENODEV when no supply is
defined in the Device Tree, which seems like it would cause this to return
early instead of treating the regulator as optional.

If -ENODEV is handled to allow the probe to continue, does
usb_dev->regulator need to be set to NULL so an error pointer is not passed
to regulator_enable() below?

[Severity: Low]
Is there a missing closing brace here? It looks like this will cause a
compilation error.

> +
> +	err = regulator_enable(usb_dev->regulator);
> +	if (err)
> +		return dev_err_probe(&core->dev, err,
> +				     "error enabling VCC regulator");
>  
>  	switch (core->id.id) {
>  	case BCMA_CORE_USB20_HOST:

[ ... ]

> @@ -452,7 +461,7 @@ static void bcma_hcd_remove(struct bcma_device *dev)

[Severity: High]
If the probe fails later, or when the device is unbound via bcma_hcd_remove(),
does this leak the regulator enable count? It appears regulator_disable() is
not called in the probe error paths or during removal.

>  
>  static void bcma_hcd_shutdown(struct bcma_device *dev)
>  {
> -	bcma_hci_platform_power_gpio(dev, false);
> +	bcma_hci_platform_power(dev, false);
>  	bcma_core_disable(dev, 0);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920211356.186967-1-rosenp@gmail.com?part=1