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

[Fenglin Wu]

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@xxxxxxxxxxxxxxxx>
> > > > > >
> > > > > > 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" 
when registering the power supply device.

2. sm8550 was a fallback of sm8350, and they all used 
"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.

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),
                                             "failed to register 
battery power supply\n");
@@ -1394,7 +1628,12 @@ static int qcom_battmgr_probe(struct 
auxiliary_device *adev,
                        return dev_err_probe(dev, 
PTR_ERR(battmgr->wls_psy),
                                             "failed to register 
wireless charing power supply\n");
        } else {
-               battmgr->bat_psy = devm_power_supply_register(dev, 
&sm8350_bat_psy_desc, &psy_cfg);
+               if (battmgr->variant == QCOM_BATTMGR_SM8550)
+                       psy_desc = &sm8550_bat_psy_desc;
+               else
+                       psy_desc = &sm8350_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),
                                             "failed to register 
battery power supply\n");

> Best regards,
> Krzysztof