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

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

[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 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.

Reviewed-by: Jigarkumar Zala <jigarkumar.zala@oss.qualcomm.com>
Signed-off-by: Anusha Arun Nandi <anusha.nandi@oss.qualcomm.com>
---
 .../phy/qualcomm/phy-qcom-mipi-csi2-core.c    | 25 +++++++++++--------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
index c0a4fa20eaf0..269232499fca 100644
--- a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
+++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
@@ -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)
-- 
2.34.1

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

[bod]

On 2026-11-02 13:39 -0800, Anusha Arun Nandi wrote:
> 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.
> 
> Reviewed-by: Jigarkumar Zala <jigarkumar.zala@oss.qualcomm.com>
> Signed-off-by: Anusha Arun Nandi <anusha.nandi@oss.qualcomm.com>
> ---
>  .../phy/qualcomm/phy-qcom-mipi-csi2-core.c    | 25 +++++++++++--------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> index c0a4fa20eaf0..269232499fca 100644
> --- a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> @@ -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)

I thought you said you were going to fix this ?

Once done you may add


Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

But not until ;)

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

[Anusha Arun Nandi]

On 11/7/2026 6:01 AM, bod@kernel.org wrote:
> On 2026-11-02 13:39 -0800, Anusha Arun Nandi wrote:
>> 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.
>>
>> Reviewed-by: Jigarkumar Zala <jigarkumar.zala@oss.qualcomm.com>
>> Signed-off-by: Anusha Arun Nandi <anusha.nandi@oss.qualcomm.com>
>> ---
>>  .../phy/qualcomm/phy-qcom-mipi-csi2-core.c    | 25 +++++++++++--------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
>> index c0a4fa20eaf0..269232499fca 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
>> @@ -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)
> 
> I thought you said you were going to fix this ?
> 
> Once done you may add
> 
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> 
> 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