Re: [PATCH RESEND v8 07/10] phy: qualcomm: eusb2-repeater: Migrate to devm_spmi_subdevice_alloc_and_add()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Hardcoded -ENODEV error return masks devm_regmap_init_spmi_ext() failure reason.
- [Medium] Redundant NULL check for `sparent` and lack of parent bus type validation.
--

commit 2df6d70edb9e23df116d298eaf4222879e5acb2e
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: qualcomm: eusb2-repeater: Migrate to devm_spmi_subdevice_alloc_and_add()

This commit registers a new SPMI sub-device for the EUSB2 repeater and
initializes its own regmap. This avoids manually adding the register base
address to every read/write call by leveraging the regmap API instead.

> diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> --- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> +++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c

[ ... ]

> @@ -242,13 +240,23 @@ static const struct phy_ops eusb2_repeater_ops = {
>  
>  static int eusb2_repeater_probe(struct platform_device *pdev)
>  {

[ ... ]

> +	if (!dev->parent)
> +		return -ENODEV;
> +

[ ... ]

> -	rptr->regmap = dev_get_regmap(dev->parent, NULL);
> -	if (!rptr->regmap)
> +	sparent = to_spmi_device(dev->parent);
> +	if (!sparent)
>  		return -ENODEV;

Since to_spmi_device() is a container_of() wrapper and dev->parent was just
checked for NULL, won't sparent always be non-NULL here?

Also, does this safely verify the parent is actually an SPMI device? If the
driver is instantiated under a non-SPMI parent via an incorrect Device Tree,
could this lead to type confusion and pass an invalid pointer to
devm_spmi_subdevice_alloc_and_add()?

> +
> +	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
> +	if (IS_ERR(sub_sdev))
> +		return PTR_ERR(sub_sdev);
> +
> +	ret = device_property_read_u32(dev, "reg", &eusb2_regmap_config.reg_base);
>  	if (ret < 0)
>  		return ret;
>  
> -	rptr->base = res;
> +	rptr->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &eusb2_regmap_config);
> +	if (IS_ERR(rptr->regmap))
> +		return -ENODEV;

Is it intentional to return a hardcoded -ENODEV here instead of propagating
the actual error code from devm_regmap_init_spmi_ext() using
PTR_ERR(rptr->regmap)? Masking the original error might make debugging
initialization failures more difficult.

>  
>  	ret = eusb2_repeater_init_vregs(rptr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511100805.121432-1-angelogioacchino.delregno@collabora.com?part=7

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy