Re: [PATCHv3 1/2] usb: bcma: add regulator support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] usb: bcma: redundant NULL check for regulator pointer
--

commit 3c80ddac8dba5090761a99f4b7fdd330945bd836
Author: Rosen Penev <rosenp@gmail.com>

usb: bcma: add regulator support

This commit adds standard regulator interface support to the bcma-hcd
driver, replacing the non-standard exclusive GPIO manipulation for VBUS
power. It allows the regulator core to reference-count enable and disable
requests so that shared GPIOs do not cause conflicts between USB2 and
USB3 controllers.

> diff --git a/drivers/usb/host/bcma-hcd.c b/drivers/usb/host/bcma-hcd.c
> index 519386255886c..cb253cae29aef 100644
> --- a/drivers/usb/host/bcma-hcd.c
> +++ b/drivers/usb/host/bcma-hcd.c
[ ... ]
> @@ -280,14 +282,23 @@ static int bcma_hcd_usb20_ns_init(struct bcma_hcd_device *bcma_hcd)
>  	return 0;
>  }
>  
> -static void bcma_hci_platform_power_gpio(struct bcma_device *dev, bool val)
> +static int bcma_hci_platform_power(struct bcma_device *dev, bool on)
>  {
>  	struct bcma_hcd_device *usb_dev = bcma_get_drvdata(dev);
> +	int err;
> +
> +	if (usb_dev->gpio_desc)
> +		return gpiod_set_value(usb_dev->gpio_desc, on);
>  
> -	if (!usb_dev->gpio_desc)
> -		return;
> +	if (!usb_dev->regulator)
> +		return 0;

[Severity: Low]
Since devm_regulator_get() returns either an ERR_PTR or a valid pointer,
and returns NULL only if CONFIG_REGULATOR is disabled, is this NULL check
necessary? The regulator stubs safely accept NULL pointers and return 0,
and the probe function already aborts if IS_ERR() is true.

> +
> +	if (on)
> +		err = regulator_enable(usb_dev->regulator);
> +	else
> +		err = regulator_disable(usb_dev->regulator);
> +
> +	return err;
>  }
[ ... ]
> @@ -395,12 +406,12 @@ static int bcma_hcd_usb30_init(struct bcma_hcd_device *bcma_hcd)
>  static int bcma_hcd_probe(struct bcma_device *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");
> +				     "error obtaining VBUS GPIO");
> +
> +	usb_dev->regulator = devm_regulator_get(dev, "vbus");
> +	if (IS_ERR(usb_dev->regulator))
> +		return dev_err_probe(dev, PTR_ERR(usb_dev->regulator), "error obtaining VBUS regulator");

[Severity: Low]
This is a pre-existing issue, but if probe fails here and returns directly,
will the physical GPIO pin remain driven high indefinitely? The earlier call
to devm_gpiod_get_optional() uses GPIOD_OUT_HIGH, and while devres will free
the software descriptor, gpiod_put() intentionally does not revert the
physical pin state.

> +
> +	err = regulator_enable(usb_dev->regulator);
> +	if (err)
> +		return dev_err_probe(dev, err, "error enabling VBUS regulator");

[Severity: Low]
This is a pre-existing issue, but similar to the above, does this direct
return skip reverting the GPIO state?

>  
>  	switch (core->id.id) {
>  	case BCMA_CORE_USB20_HOST:
[ ... ]
> @@ -427,13 +446,16 @@ static int bcma_hcd_probe(struct bcma_device *core)
>  	if (err)
> -		return err;
> +		goto error;
>  
>  	bcma_set_drvdata(core, usb_dev);
>  	return 0;
> +error:
> +	regulator_disable(usb_dev->regulator);
> +	return err;

[Severity: Low]
This is a pre-existing issue, but while this common error path correctly
handles disabling the newly added regulator, does it also need to drive
the pre-existing GPIO back low so the peripheral isn't left powered
indefinitely on failure?

>  }

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