Re: [PATCH 1/7] phy: qcom-mipi-csi2: fix -EEXIST probe failure on single power-domain SoCs

[Anusha Arun Nandi <anusha.nandi@oss.qualcomm.com>]

Hi Bryan,

Thank you for the feedback! Will drop this patch as a fix
with the other comments addressed.

Regards,
Anusha

On 10/19/2026 2:55 AM, Bryan O'Donoghue wrote:
> On 17/10/2026 00:54, 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:
>>
>>    qcom-mipi-csi2-phy ace6000.phy: error -EEXIST: Failed to attach
>>    power-domain list
>>    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.
> 
> Fix implies bug but, this patch really adds support for one power-domain
> cases.
> 
> I think the patch title should be something like "Add support for single
> power-domain case"
> 
> or if you believe it is a bugfix level change add
> 
> Fixes:
> Cc: stable@vger.kernel.org
> 
> My own preference is "Add support" since the driver hasn't been designed
> to support single PD cases thus far but, OTOH you could make an argument
> that that is in fact a design bug.
> 
>> Signed-off-by: Anusha Arun Nandi <anusha.nandi@oss.qualcomm.com>
>> ---
>>   .../phy/qualcomm/phy-qcom-mipi-csi2-core.c    | 24 +++++++++++++------
>>   1 file changed, 17 insertions(+), 7 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..00285be9dd64 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;
>>
>> @@ -233,6 +233,19 @@ static int
>> phy_qcom_mipi_csi2_attach_pm_domains(struct mipi_csi2phy_device *csi2
>>       const char **pd_names;
>>       int i;
>>
>> +    /*
>> +     * A device with a single power-domain entry in the devicetree is
>> +     * already attached by the generic platform-bus code before probe()
>> +     * runs (genpd_dev_pm_attach()), so calling
>> devm_pm_domain_attach_list()
>> +     * here would fail with -EEXIST. dev_pm_domain_attach() returns
>> early
>> +     * in that case (dev->pm_domain already set) instead of leaving
>> +     * csi2phy->pd_list unset; this is fine since pd_list is only
>> used for
>> +     * performance-state scaling, which is unused ("scaled = false") on
>> +     * every SoC that has a single "top" power-domain.
>> +     */
> 
> This is alot of comment - looks like LLM babble to me. I think it can be
> dropped the code is pretty obvious, if num_genpds == 1 do this other
> thing here.
> 
>> +    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,
>>                   GFP_KERNEL);
>> @@ -369,11 +382,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
>