phy: qcom-mipi-csi2: Add support for single power-domain SoCs
Subsystems (3)
PATCH v3 1/1 phy: qcom-mipi-csi2: Add support for single power-domain SoCs
sa8775p, qcs8300 and sc7280 each list exactly one power-domain ("top") on the CSI2 PHY node. For a device with a single power-domain phandle, the generic platform-bus code (platform_probe() -> dev_pm_domain_attach()) auto-attaches it via genpd_dev_pm_attach() before the driver's own probe() runs. phy_qcom_mipi_csi2_probe() then unconditionally calls devm_pm_domain_attach_list(), which fails with -EEXIST because dev->pm_domain is already set, and the driver treats that as fatal:
1. qcom-mipi-csi2-phy ace6000.phy: error -EEXIST: Failed to attach power-domain list
2. qcom-mipi-csi2-phy ace6000.phy: probe with driver qcom-mipi-csi2-phy failed with error -17
x1e80100 is unaffected: it lists three power-domains, so the generic code does not auto-attach any of them (auto-attach only applies to single-power-domain devices); the driver's explicit attach is the only one that runs.
For the single power-domain case, call dev_pm_domain_attach() instead of devm_pm_domain_attach_list(): it returns early when dev->pm_domain is already set, so csi2phy->pd_list is left NULL rather than built up by hand. The three call sites that walk pd_list for genpd performance-state scaling now guard against a NULL pd_list, which is harmless since scaling is unused (scaled = false) on every SoC that has a single "top" domain.
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.
--- .../phy/qualcomm/phy-qcom-mipi-csi2-core.c | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-)
drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c +15 -10
@@ -43,7 +43,7 @@ phy_qcom_mipi_csi2_set_clock_rates(struct mipi_csi2phy_device *csi2phy, } pstate =0 ; -for (i =0 ; i < csi2phy->pd_list->num_pds; i++) { +for (i =0 ; csi2phy->pd_list && i < csi2phy->pd_list->num_pds; i++) {unsigned int perf;if (!csi2phy->soc_cfg->genpds[i].scaled)
@@ -174,7 +174,7 @@ static int phy_qcom_mipi_csi2_power_on(struct phy *phy) unset_rate:dev_pm_opp_set_rate (csi2phy->dev,0 ); -for (i =0 ; i < csi2phy->pd_list->num_pds; i++) { +for (i =0 ; csi2phy->pd_list && i < csi2phy->pd_list->num_pds; i++) {if (!csi2phy->soc_cfg->genpds[i].scaled)continue ;
@@ -204,7 +204,7 @@ static int phy_qcom_mipi_csi2_power_off(struct phy *phy)dev_pm_opp_set_rate (csi2phy->dev,0 ); -for (i =0 ; i < csi2phy->pd_list->num_pds; i++) { +for (i =0 ; csi2phy->pd_list && i < csi2phy->pd_list->num_pds; i++) {if (!csi2phy->soc_cfg->genpds[i].scaled)continue ;
@@ -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" ); - } ret =devm_pm_runtime_enable (dev);if (ret <0 )
Signature · 1 line
2.34.1
[email protected]
AI reviewThank 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() --
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 <[email protected]>
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.
Quoted 4 lines
drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c +0
-0
[ ... ]
Quoted 29 lines
drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c +11
-3@@ -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)
drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c static int phy_qcom_mipi_csi2_probe(struct platform_device *pdev) · line 376
Quoted 9 lines
drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c +1
-4@@ -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/[email protected]
Signature · 6 lines
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1 -- linux-phy mailing list [email protected] https://lists.infradead.org/mailman/listinfo/linux-phy
Quoted 100 lines · Anusha Arun Nandi
sa8775p, qcs8300 and sc7280 each list exactly one power-domain ("top") on the CSI2 PHY node. For a device with a single power-domain phandle, the generic platform-bus code (
platform_probe()->dev_pm_domain_attach()) auto-attaches it viagenpd_dev_pm_attach()before the driver's ownprobe()runs.phy_qcom_mipi_csi2_probe()then unconditionally callsdevm_pm_domain_attach_list(), which fails with-EEXISTbecausedev->pm_domainis already set, and the driver treats that as fatal:1. qcom-mipi-csi2-phy ace6000.phy: error
-EEXIST: Failed to attach power-domain list
2. qcom-mipi-csi2-phy ace6000.phy: probe with driver qcom-mipi-csi2-phy failed with error -17x1e80100 is unaffected: it lists three power-domains, so the generic code does not auto-attach any of them (auto-attach only applies to single-power-domain devices); the driver's explicit attach is the only one that runs.
For the single power-domain case, call
dev_pm_domain_attach()instead ofdevm_pm_domain_attach_list(): it returns early whendev->pm_domainis already set, socsi2phy->pd_listis left NULL rather than built up by hand. The three call sites that walkpd_listfor genpd performance-state scaling now guard against a NULLpd_list, which is harmless since scaling is unused (scaled = false) on every SoC that has a single "top" domain.
devm_pm_domain_attach_list()returns the number of attached power-domains on success, so a bare "ret < 0" check inprobe()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.Reviewed-by Jigarkumar Zala <[email protected]>Signed-off-by Anusha Arun Nandi <[email protected]>--- .../phy/qualcomm/phy-qcom-mipi-csi2-core.c | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-)drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c +14
-6@@ -43,7 +43,7 @@ phy_qcom_mipi_csi2_set_clock_rates(struct mipi_csi2phy_device *csi2phy, } pstate =0 ; -for (i =0 ; i < csi2phy->pd_list->num_pds; i++) { +for (i =0 ; csi2phy->pd_list && i < csi2phy->pd_list->num_pds; i++) {unsigned int perf;if (!csi2phy->soc_cfg->genpds[i].scaled)@@ -174,7 +174,7 @@ static int phy_qcom_mipi_csi2_power_on(struct phy *phy) unset_rate:dev_pm_opp_set_rate (csi2phy->dev,0 ); -for (i =0 ; i < csi2phy->pd_list->num_pds; i++) { +for (i =0 ; csi2phy->pd_list && i < csi2phy->pd_list->num_pds; i++) {if (!csi2phy->soc_cfg->genpds[i].scaled)continue ;@@ -204,7 +204,7 @@ static int phy_qcom_mipi_csi2_power_off(struct phy *phy)dev_pm_opp_set_rate (csi2phy->dev,0 ); -for (i =0 ; i < csi2phy->pd_list->num_pds; i++) { +for (i =0 ; csi2phy->pd_list && i < csi2phy->pd_list->num_pds; i++) {if (!csi2phy->soc_cfg->genpds[i].scaled)continue ;@@ -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)
drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c static int phy_qcom_mipi_csi2_probe(struct platform_device *pdev) · line 380
Quoted 7 lines
drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c +1
-3@@ -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 )
I thought you said you were going to fix this ?
Once done you may add
But not until ;)
Quoted 111 lines · On 11/7/2026 6:01 AM, [email protected] wrote:
Quoted 100 lines · Anusha Arun Nandi
sa8775p, qcs8300 and sc7280 each list exactly one power-domain ("top") on the CSI2 PHY node. For a device with a single power-domain phandle, the generic platform-bus code (
platform_probe()->dev_pm_domain_attach()) auto-attaches it viagenpd_dev_pm_attach()before the driver's ownprobe()runs.phy_qcom_mipi_csi2_probe()then unconditionally callsdevm_pm_domain_attach_list(), which fails with-EEXISTbecausedev->pm_domainis already set, and the driver treats that as fatal:1. qcom-mipi-csi2-phy ace6000.phy: error
-EEXIST: Failed to attach power-domain list
2. qcom-mipi-csi2-phy ace6000.phy: probe with driver qcom-mipi-csi2-phy failed with error -17x1e80100 is unaffected: it lists three power-domains, so the generic code does not auto-attach any of them (auto-attach only applies to single-power-domain devices); the driver's explicit attach is the only one that runs.
For the single power-domain case, call
dev_pm_domain_attach()instead ofdevm_pm_domain_attach_list(): it returns early whendev->pm_domainis already set, socsi2phy->pd_listis left NULL rather than built up by hand. The three call sites that walkpd_listfor genpd performance-state scaling now guard against a NULLpd_list, which is harmless since scaling is unused (scaled = false) on every SoC that has a single "top" domain.
devm_pm_domain_attach_list()returns the number of attached power-domains on success, so a bare "ret < 0" check inprobe()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.Reviewed-by Jigarkumar Zala <[email protected]>Signed-off-by Anusha Arun Nandi <[email protected]>--- .../phy/qualcomm/phy-qcom-mipi-csi2-core.c | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-)drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c +14
-6@@ -43,7 +43,7 @@ phy_qcom_mipi_csi2_set_clock_rates(struct mipi_csi2phy_device *csi2phy, } pstate =0 ; -for (i =0 ; i < csi2phy->pd_list->num_pds; i++) { +for (i =0 ; csi2phy->pd_list && i < csi2phy->pd_list->num_pds; i++) {unsigned int perf;if (!csi2phy->soc_cfg->genpds[i].scaled)@@ -174,7 +174,7 @@ static int phy_qcom_mipi_csi2_power_on(struct phy *phy) unset_rate:dev_pm_opp_set_rate (csi2phy->dev,0 ); -for (i =0 ; i < csi2phy->pd_list->num_pds; i++) { +for (i =0 ; csi2phy->pd_list && i < csi2phy->pd_list->num_pds; i++) {if (!csi2phy->soc_cfg->genpds[i].scaled)continue ;@@ -204,7 +204,7 @@ static int phy_qcom_mipi_csi2_power_off(struct phy *phy)dev_pm_opp_set_rate (csi2phy->dev,0 ); -for (i =0 ; i < csi2phy->pd_list->num_pds; i++) { +for (i =0 ; csi2phy->pd_list && i < csi2phy->pd_list->num_pds; i++) {if (!csi2phy->soc_cfg->genpds[i].scaled)continue ;@@ -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)drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c static int phy_qcom_mipi_csi2_probe(struct platform_device *pdev) · line 380
Quoted 7 lines
drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c +1
-3@@ -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 )I thought you said you were going to fix this ?
Quoted 6 lines
Once done you may add
Reviewed-by Bryan O'Donoghue <[email protected]>But not until ;)
I went back and took a closer look at this than I did when I replied with "Applied" earlier, and I now think I may have agreed a bit too quickly.
Looking at the actual implementation of dev_pm_domain_attach_list(), it appears to have only three possible outcomes:
1. A negative errno on any failure path (-ENOMEM, -EEXIST, or a failure from dev_pm_domain_attach_by_name() propagated via err_attach/err_link).
2. A positive return value equal to num_pds on success. In that case *list = pds is assigned in the same success path that returns num_pds, so a non-NULL pd_list and a positive return always occur together.
3. 0, but only from two early-return cases before *list is ever touched:
a. if (!np) return 0;
b. if (num_pds <= 0) return 0;
As far as I can tell, neither of those 0 return paths is reachable here.
The !np case doesn't seem applicable because this driver only supports DT systems through its of_match_table; there is no ACPI path, so dev->of_node should be present when probe() runs.
Likewise, num_pds <= 0 doesn't appear possible. We only enter this branch when num_genpds != 1, and num_genpds comes from the SoC's static soc_cfg data (for example, x1e80100 uses 3). It isn't runtime-controlled and isn't expected to be zero by construction.
Given those constraints, ret < 0 and pd_list == NULL seem to collapse to the same condition at this call site. That's what led me to revisit my earlier agreement: I no longer see a path where the NULL check provides additional protection beyond the return-value check. To me, this looks more like dead code than a safety net, although I'm happy to hear if there's a scenario I've overlooked.
Best regards,
Anusha