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

[Krzysztof Kozlowski <krzk@kernel.org>]

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