Re: [PATCH v3] phy: qcom-mipi-csi2: Add support for single power-domain SoCs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] phy: qcom-mipi-csi2: missing -ENODEV on empty devm_pm_domain_attach_list()
--

commit 6f161d17e069ed4df2dd29378fafa1b47c7e3710
Author: Anusha Arun Nandi <anusha.nandi@oss.qualcomm.com>

phy: qcom-mipi-csi2: Add support for single power-domain SoCs

This commit adds support for SoCs with a single power-domain on the CSI2 PHY
node by calling dev_pm_domain_attach() instead of devm_pm_domain_attach_list()
when num_genpds is 1, and guarding csi2phy->pd_list walks against NULL.

> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> index c0a4fa20eaf0c..269232499fca8 100644
> --- a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c

[ ... ]

> @@ -231,7 +231,11 @@ static int phy_qcom_mipi_csi2_attach_pm_domains(struct mipi_csi2phy_device *csi2
>  {
>  	struct dev_pm_domain_attach_data pd_data = { 0 };
>  	const char **pd_names;
> -	int i;
> +	int i, ret;
> +
> +	/* Single power-domain devices are already attached by the bus core. */
> +	if (csi2phy->soc_cfg->num_genpds == 1)
> +		return dev_pm_domain_attach(csi2phy->dev, 0);
>  
>  	pd_names = devm_kzalloc(csi2phy->dev,
>  				sizeof(char *) * csi2phy->soc_cfg->num_genpds,
> @@ -245,8 +249,12 @@ static int phy_qcom_mipi_csi2_attach_pm_domains(struct mipi_csi2phy_device *csi2
>  	pd_data.pd_names = pd_names;
>  	pd_data.num_pd_names = csi2phy->soc_cfg->num_genpds;
>  
> -	return devm_pm_domain_attach_list(csi2phy->dev, &pd_data,
> -					  &csi2phy->pd_list);
> +	ret = devm_pm_domain_attach_list(csi2phy->dev, &pd_data,
> +					 &csi2phy->pd_list);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
>  }
>  
>  static int phy_qcom_mipi_csi2_parse_routing(struct mipi_csi2phy_device *csi2phy)
> @@ -369,11 +377,8 @@ static int phy_qcom_mipi_csi2_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	ret = phy_qcom_mipi_csi2_attach_pm_domains(csi2phy);
> -	if (ret < 0 || csi2phy->pd_list == NULL) {
> -		if (ret == 0)
> -			ret = -ENODEV;
> +	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to attach power-domain list\n");
> -	}

[Severity: Medium]
As reported by Bryan O'Donoghue on v2 [1], doesn't this drop the check for
when devm_pm_domain_attach_list() returns 0 without attaching any power
domains?

The commit message notes:

    devm_pm_domain_attach_list() returns the number of attached
    power-domains on success, so a bare "ret < 0" check in probe() is not
    enough to catch the case where it succeeds without actually attaching
    any domains. phy_qcom_mipi_csi2_attach_pm_domains() now also checks for
    that case directly and reports -ENODEV, keeping probe()'s own check
    unchanged.

However, phy_qcom_mipi_csi2_probe() removes the csi2phy->pd_list == NULL and
ret == 0 check while phy_qcom_mipi_csi2_attach_pm_domains() only checks
if (ret < 0) and then returns 0.

If devm_pm_domain_attach_list() returns 0 on a multi-power-domain SoC (for
instance when CONFIG_PM_GENERIC_DOMAINS is disabled), shouldn't
phy_qcom_mipi_csi2_attach_pm_domains() return -ENODEV instead of 0 so probe
does not succeed with an unattached power-domain list?

[1] https://lore.kernel.org/r/58599b61-a56b-4d6b-b09f-03d813a64c1c@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102213936.1251316-1-anusha.nandi@oss.qualcomm.com?part=1

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