| Message ID | 20250530-qcom_battmgr_update-v2-6-9e377193a656@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | power: supply: Add several features support in qcom-battmgr driver | expand |
On Fri, May 30, 2025 at 03:35:11PM +0800, 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? > > Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com> > --- > Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml > index 4c9e78f29523e3d77aacb4299f64ab96f9b1a831..972bec151118f2e20e1f3b4e0c0a8fbbbea7ab90 100644 > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml > @@ -39,9 +39,11 @@ properties: > - enum: > - qcom,sm8650-pmic-glink > - qcom,sm8750-pmic-glink > - - qcom,x1e80100-pmic-glink > - const: qcom,sm8550-pmic-glink > - const: qcom,pmic-glink > + - items: > + - const: qcom,x1e80100-pmic-glink > + - const: qcom,pmic-glink > > '#address-cells': > const: 1 > > -- > 2.34.1 > >
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
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. I explained this a little bit in the commit text of patch [7/8] in this series, I can make similar description in patch [6/8].
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. 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. > > Best regards, > Krzysztof
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" 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
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
diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml index 4c9e78f29523e3d77aacb4299f64ab96f9b1a831..972bec151118f2e20e1f3b4e0c0a8fbbbea7ab90 100644 --- a/Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml @@ -39,9 +39,11 @@ properties: - enum: - qcom,sm8650-pmic-glink - qcom,sm8750-pmic-glink - - qcom,x1e80100-pmic-glink - const: qcom,sm8550-pmic-glink - const: qcom,pmic-glink + - items: + - const: qcom,x1e80100-pmic-glink + - const: qcom,pmic-glink '#address-cells': const: 1