Re: [PATCH v2 6/8] dt-bindings: soc: qcom: pmic-glink: Move X1E80100 out of fallbacks

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 6/4/25 11:40 AM, Fenglin Wu wrote:
> 
> On 6/3/2025 5:34 PM, Krzysztof Kozlowski wrote:
>> On 03/06/2025 09:41, Fenglin Wu wrote:
>>> On 6/3/2025 3:06 PM, Krzysztof Kozlowski wrote:
>>>> On 03/06/2025 08:59, Fenglin Wu wrote:
>>>>> On 6/3/2025 2:47 PM, Krzysztof Kozlowski wrote:
>>>>>> On 03/06/2025 08:42, Fenglin Wu wrote:
>>>>>>> On 6/2/2025 3:40 PM, Krzysztof Kozlowski wrote:
>>>>>>>> On 30/05/2025 09:35, Fenglin Wu via B4 Relay wrote:
>>>>>>>>> From: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
>>>>>>>>>
>>>>>>>>> Move X1E80100 out of the fallbacks of SM8550 in pmic-glink support.
>>>>>>>> Why?
>>>>>>>>
>>>>>>>> Do not describe what you do here, it's obvious. We see it from the diff.
>>>>>>>>
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Krzysztof
>>>>>>> Previously, in qcom_battmgr driver, x1e80100 was specified with a match
>>>>>>> data the same as sc8280xp, also sm8550 was treated a fallback of sm8350
>>>>>>> without the need of a match data.
>>>>>>>
>>>>>>> In ucsi_glink driver, sm8550 had a match data and x1e80100 was treated
>>>>>>> as a fallback of sm8550. There was no issues to make x1e80100 as a
>>>>>>> fallback of sm8550 from both qcom_battmgr and ucsi_glink driver perspective.
>>>>>>>
>>>>>>> In patch [5/8] in this series, in qcom_battmgr driver, it added charge
>>>>>>> control functionality for sm8550 and x1e80100 differently hence
>>>>>>> different match data was specified for them, and it makes x1e80100 ad
>>>>>>> sm8550 incompatible and they need to be treated differently.
>>>>>> So you break ABI and that's your problem to fix. You cannot make devices
>>>>>> incompatible without good justification.
>>>>> I would say x1e80100 and sm8550 are different and incompatible from a
>>>>> battery management firmware support perspective. The x1e80100 follows
>>>>> the sc8280xp as a compute platform, whereas the sm8550 follows the
>>>>> sm8350 as a mobile platform.
>>>> Not correct arguments for compatibility.
>>>>
>>>>> The difference between them was initially ignored because the sm8550
>>>>> could use everything that the sm8350 has, and no match data needed to be
>>>>> specified for it. However, now the sm8550 has new features that the
>>>>> sm8350 doesn't have, requiring us to treat it differently, thus the
>>>>> incompatibility was acknowledged.
>>>> So they are perfectly compatible.
>>>>
>>>> I really do not understand what we are discussing here. Explain in
>>>> simple terms of DT spec: what is incompatible that SW cannot use one
>>>> interface to handle the other?
>>> 1. x1e80100 was a fallback of sc8280xp, it used "sc8280xp_bat_psy_desc"
>>
>> No, that's not true. Read the binding again:
>>
>>                - qcom,x1e80100-pmic-glink
>>             - const: qcom,sm8550-pmic-glink
>>
>> No fallback to sc8280xp.
>>
>>
>>> when registering the power supply device.
>>>
>>> 2. sm8550 was a fallback of sm8350, and they all used
>>
>> Also not true. The remaining fallback is not sm8350.
>>
>>
>>> "sm8350_bat_psy_desc" when registering the power supply device.
>>>
>>> 3. x1e80100 and sm8550 they are incompatible as they are using different
>>> data structure of "xxx_bat_psy_desc"  and other “psy_desc" too, such as,
>>> ac/usb/wls.
>> Look at the driver and bindings now - they are compatible. It looks like
>> you made it incompatible and now you claim the "they are incompatible".
>> No, you did it. Look at the driver.
>>
>>
>>
>>> 4. For charge control functionality, it's only supported in the battery
>>> management firmware in x1e80100 and sm8550 platforms. And the change in
>>> battmgr driver (patch [5/8]) adds the support by using 2 additional
>>> power supply properties, which eventually need to be added in the
>>> "properties" data member of "xxx_bat_psy_desc" when registering power
>>> supply devices. Hence, "x1e80100_bat_psy_desc" and "sm8550_bat_psy_desc"
>>> are created and used separately when registering power supply device
>>> according to the "variant" value defined in the match data.
>>>
>>> The main code change is in [5/8], I am pasting a snippet which might
>>> help to explain this a little bit:
>>>
>>> -       if (battmgr->variant == QCOM_BATTMGR_SC8280XP) {
>>> -               battmgr->bat_psy = devm_power_supply_register(dev,
>>> &sc8280xp_bat_psy_desc, &psy_cfg);
>>> +       if (battmgr->variant == QCOM_BATTMGR_SC8280XP ||
>>> battmgr->variant == QCOM_BATTMGR_X1E80100) {
>>> +               if (battmgr->variant == QCOM_BATTMGR_X1E80100)
>>> +                       psy_desc = &x1e80100_bat_psy_desc;
>>> +               else
>>> +                       psy_desc = &sc8280xp_bat_psy_desc;
>>> +
>>> +               battmgr->bat_psy = devm_power_supply_register(dev,
>>> psy_desc, &psy_cfg);
>>>                   if (IS_ERR(battmgr->bat_psy))
>>>                           return dev_err_probe(dev,
>>> PTR_ERR(battmgr->bat_psy),
>>
>> This explains nothing to me. I think you did not get my questions at all
>> and just want to push whatever you have in drivers.
>>
>> Such ping pongs are just tiring, so go back to my previous email, read
>> it carefully and try harder to understand what compatibility means.
>>
>>
>> NAK, you are affecting the users and ABI with justification "I make it
>> now incompatible, so it is incompatible".
>>
>> Best regards,
>> Krzysztof
> 
> Thanks for the explanation with patience. I misunderstood the fallback behavior.
> 
> I was worried about if the compatible string matching would work correctly if both the device node and the driver declared multiple identical compatible strings.
> 
> I understand now and even if the device node and the driver have defined multiple identical compatible strings, the best match which is the most specific compatible string will be found.
> 
> So in the example below, for X1E80100-CRD, the battmgr driver will always match to "qcom,x1e80100-pmic-glink" which is the most specific compatible string defined at the beginning of the device node compatible string, and the compatibility has not been broken.
> 
> In qcom_battmgr driver:
> 
> static const struct of_device_id qcom_battmgr_of_variants[] = {
>         ...
>         { .compatible = "qcom,x1e80100-pmic-glink", .data = (void *)QCOM_BATTMGR_X1E80100 },
>         { .compatible = "qcom,sm8550-pmic-glink", .data = (void *)QCOM_BATTMGR_SM8550 },
>         ...
> };
> 
> In x1-crd.dtsi:
> 
> pmic-glink {
>           compatible = "qcom,x1e80100-pmic-glink",
>                      "qcom,sm8550-pmic-glink",
>                      "qcom,pmic-glink";
>         ...
> 
> }
> 
> Let me know if my understanding is correct. I will drop patch [6/8],[7/8],[8/8] in next version.

Unless we have some mobile-firmware-specific calls/behaviors that apply to
sm8550, but not to x1e80100 (which I don't believe we do), I think this is
fair

Konrad