| Message ID | 20231023-pm8916-dtsi-bms-lbc-v2-0-343e3dbf423e@trvn.ru |
|---|---|
| Headers | show |
| Series | pm8916: Add BMS and charger | expand |
On Tue, 24 Oct 2023, Nikita Travkin wrote: > Rob Herring писал(а) 23.10.2023 22:40: > > On Mon, 23 Oct 2023 11:20:32 +0500, Nikita Travkin wrote: > >> PM8916 (and probably some other similar pmics) have hardware blocks for > >> battery monitoring and charging. Add patterns for respecive nodes so the > >> devicetree for those blocks can be validated properly. > >> > >> Signed-off-by: Nikita Travkin <nikita@trvn.ru> > >> --- > >> Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.yaml | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > > > yamllint warnings/errors: > > > > dtschema/dtc warnings/errors: > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.yaml: > > Error in referenced schema matching $id: http://devicetree.org/schemas/power/supply/qcom,pm8916-bms-vm.yaml > > > > doc reference errors (make refcheckdocs): > > > > See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231023-pm8916-dtsi-bms-lbc-v2-1-343e3dbf423e@trvn.ru > > > > The base for the series is generally the latest rc1. A different dependency > > should be noted in *this* patch. > > > > Somehow I missed the memo and thought it tracks -next... > > This patch depends on 7f590e3831 and 5cee843d56 in linux-next.git > They were applied in [1]. > > I'm wondering if the bot just bails out when the "depend" is present > or there is some more sophisticated logic to suggest the base to it? So is this good to go, or not?
Lee Jones писал(а) 25.10.2023 17:21: > On Tue, 24 Oct 2023, Nikita Travkin wrote: > >> Rob Herring писал(а) 23.10.2023 22:40: >> > On Mon, 23 Oct 2023 11:20:32 +0500, Nikita Travkin wrote: >> >> PM8916 (and probably some other similar pmics) have hardware blocks for >> >> battery monitoring and charging. Add patterns for respecive nodes so the >> >> devicetree for those blocks can be validated properly. >> >> >> >> Signed-off-by: Nikita Travkin <nikita@trvn.ru> >> >> --- >> >> Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.yaml | 6 ++++++ >> >> 1 file changed, 6 insertions(+) >> >> >> > >> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' >> > on your patch (DT_CHECKER_FLAGS is new in v5.13): >> > >> > yamllint warnings/errors: >> > >> > dtschema/dtc warnings/errors: >> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.yaml: >> > Error in referenced schema matching $id: http://devicetree.org/schemas/power/supply/qcom,pm8916-bms-vm.yaml >> > >> > doc reference errors (make refcheckdocs): >> > >> > See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231023-pm8916-dtsi-bms-lbc-v2-1-343e3dbf423e@trvn.ru >> > >> > The base for the series is generally the latest rc1. A different dependency >> > should be noted in *this* patch. >> > >> >> Somehow I missed the memo and thought it tracks -next... >> >> This patch depends on 7f590e3831 and 5cee843d56 in linux-next.git >> They were applied in [1]. >> >> I'm wondering if the bot just bails out when the "depend" is present >> or there is some more sophisticated logic to suggest the base to it? > > So is this good to go, or not? IMO this patch should be good, it passes the check on today's linux-next on my end. The only concern might be that if someone runs dt_binding_check on for-mfd-next, it would skip that file with an error since there is no dependency yet. If this is critical to you, I was going to respin this after the -rc1, but if you can pick the schema now, I can respin the remainder earlier. Nikita
On Mon, 23 Oct 2023 11:20:32 +0500, Nikita Travkin wrote: > PM8916 (and probably some other similar pmics) have hardware blocks for > battery monitoring and charging. Add patterns for respecive nodes so the > devicetree for those blocks can be validated properly. > > Applied, thanks! [1/3] dt-bindings: mfd: qcom,spmi-pmic: Add pm8916 vm-bms and lbc commit: e9aec86e211ee493081e8934b8c821d660b417ee -- Lee Jones [李琼斯]
On 10/24/23 11:29, Nikita Travkin wrote: > Konrad Dybcio писал(а) 24.10.2023 13:34: >> On 10/23/23 08:20, Nikita Travkin wrote: >>> pm8916 contains some hardware blocks for battery powered devices: >>> >>> - VM-BMS: Battery voltage monitoring block. >>> - LBC: Linear battery charger. >>> >>> Add them to the pmic dtsi so the devices that make use of those blocks >>> can enable them. >>> >>> Signed-off-by: Nikita Travkin <nikita@trvn.ru> >>> --- >>> arch/arm64/boot/dts/qcom/pm8916.dtsi | 48 ++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 48 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/pm8916.dtsi b/arch/arm64/boot/dts/qcom/pm8916.dtsi >>> index f4de86787743..4b2e8fb47d2d 100644 >>> --- a/arch/arm64/boot/dts/qcom/pm8916.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/pm8916.dtsi >>> @@ -41,6 +41,35 @@ watchdog { >>> }; >>> }; >>> + pm8916_charger: charger@1000 { >>> + compatible = "qcom,pm8916-lbc"; >>> + reg = <0x1000>, <0x1200>, <0x1300>, <0x1600>; >>> + reg-names = "chgr", "bat_if", "usb", "misc"; >>> + >>> + interrupts = <0x0 0x10 0 IRQ_TYPE_EDGE_BOTH>, >>> + <0x0 0x10 5 IRQ_TYPE_EDGE_BOTH>, >>> + <0x0 0x10 6 IRQ_TYPE_EDGE_BOTH>, >>> + <0x0 0x10 7 IRQ_TYPE_EDGE_BOTH>, >>> + <0x0 0x12 0 IRQ_TYPE_EDGE_BOTH>, >>> + <0x0 0x12 1 IRQ_TYPE_EDGE_BOTH>, >>> + <0x0 0x13 0 IRQ_TYPE_EDGE_BOTH>, >>> + <0x0 0x13 1 IRQ_TYPE_EDGE_BOTH>, >>> + <0x0 0x13 2 IRQ_TYPE_EDGE_BOTH>, >>> + <0x0 0x13 4 IRQ_TYPE_EDGE_BOTH>; >>> + interrupt-names = "vbat_det", >>> + "fast_chg", >>> + "chg_fail", >>> + "chg_done", >>> + "bat_pres", >>> + "temp_ok", >>> + "coarse_det", >>> + "usb_vbus", >> So, both the charger and the USBIN driver use the same irq? :/ >> > > AFAIU the usbin extcon driver pretty much just tracks the state > of the IRQ to report extcon. It happens to assume the same part > of the pmic though, yes, which also means there will be no user > that would enable both charger and vbus extcon, since charger > driver provides this functionality as well. So, should USBIN be removed from PM8916 dt since it's essentially a part of the charger block? Konrad
On Thu, Oct 26, 2023 at 08:54:00PM +0200, Konrad Dybcio wrote: > On 10/24/23 11:29, Nikita Travkin wrote: > > Konrad Dybcio писал(а) 24.10.2023 13:34: > > > On 10/23/23 08:20, Nikita Travkin wrote: > > > > pm8916 contains some hardware blocks for battery powered devices: > > > > > > > > - VM-BMS: Battery voltage monitoring block. > > > > - LBC: Linear battery charger. > > > > > > > > Add them to the pmic dtsi so the devices that make use of those blocks > > > > can enable them. > > > > > > > > Signed-off-by: Nikita Travkin <nikita@trvn.ru> > > > > --- > > > > arch/arm64/boot/dts/qcom/pm8916.dtsi | 48 ++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 48 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/pm8916.dtsi b/arch/arm64/boot/dts/qcom/pm8916.dtsi > > > > index f4de86787743..4b2e8fb47d2d 100644 > > > > --- a/arch/arm64/boot/dts/qcom/pm8916.dtsi > > > > +++ b/arch/arm64/boot/dts/qcom/pm8916.dtsi > > > > @@ -41,6 +41,35 @@ watchdog { > > > > }; > > > > }; > > > > + pm8916_charger: charger@1000 { > > > > + compatible = "qcom,pm8916-lbc"; > > > > + reg = <0x1000>, <0x1200>, <0x1300>, <0x1600>; > > > > + reg-names = "chgr", "bat_if", "usb", "misc"; > > > > + > > > > + interrupts = <0x0 0x10 0 IRQ_TYPE_EDGE_BOTH>, > > > > + <0x0 0x10 5 IRQ_TYPE_EDGE_BOTH>, > > > > + <0x0 0x10 6 IRQ_TYPE_EDGE_BOTH>, > > > > + <0x0 0x10 7 IRQ_TYPE_EDGE_BOTH>, > > > > + <0x0 0x12 0 IRQ_TYPE_EDGE_BOTH>, > > > > + <0x0 0x12 1 IRQ_TYPE_EDGE_BOTH>, > > > > + <0x0 0x13 0 IRQ_TYPE_EDGE_BOTH>, > > > > + <0x0 0x13 1 IRQ_TYPE_EDGE_BOTH>, > > > > + <0x0 0x13 2 IRQ_TYPE_EDGE_BOTH>, > > > > + <0x0 0x13 4 IRQ_TYPE_EDGE_BOTH>; > > > > + interrupt-names = "vbat_det", > > > > + "fast_chg", > > > > + "chg_fail", > > > > + "chg_done", > > > > + "bat_pres", > > > > + "temp_ok", > > > > + "coarse_det", > > > > + "usb_vbus", > > > So, both the charger and the USBIN driver use the same irq? :/ > > > > > > > AFAIU the usbin extcon driver pretty much just tracks the state > > of the IRQ to report extcon. It happens to assume the same part > > of the pmic though, yes, which also means there will be no user > > that would enable both charger and vbus extcon, since charger > > driver provides this functionality as well. > So, should USBIN be removed from PM8916 dt since it's essentially > a part of the charger block? > The "USB_IN" pad of the PM8916 seems to be connected on pretty much all devices, even if they are using external chargers and the charging functionality of PM8916 is completely disabled. For those devices, the &pm8916_usbin device provides a convenient way to detect the USB state, even without a working charger driver. While we could modify the PM8916 charger driver and DT node to have some special mode where charging and battery monitoring is completely disabled and only the USBIN extcon is provided, I'm not sure if that would provide a significant advantage compared to just keeping the simple &pm8916_usbin node with the existing driver. Thanks, Stephan
On 10/26/23 21:17, Stephan Gerhold wrote: > On Thu, Oct 26, 2023 at 08:54:00PM +0200, Konrad Dybcio wrote: >> On 10/24/23 11:29, Nikita Travkin wrote: >>> Konrad Dybcio писал(а) 24.10.2023 13:34: >>>> On 10/23/23 08:20, Nikita Travkin wrote: >>>>> pm8916 contains some hardware blocks for battery powered devices: >>>>> >>>>> - VM-BMS: Battery voltage monitoring block. >>>>> - LBC: Linear battery charger. >>>>> >>>>> Add them to the pmic dtsi so the devices that make use of those blocks >>>>> can enable them. >>>>> >>>>> Signed-off-by: Nikita Travkin <nikita@trvn.ru> >>>>> --- >>>>> arch/arm64/boot/dts/qcom/pm8916.dtsi | 48 ++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 48 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/qcom/pm8916.dtsi b/arch/arm64/boot/dts/qcom/pm8916.dtsi >>>>> index f4de86787743..4b2e8fb47d2d 100644 >>>>> --- a/arch/arm64/boot/dts/qcom/pm8916.dtsi >>>>> +++ b/arch/arm64/boot/dts/qcom/pm8916.dtsi >>>>> @@ -41,6 +41,35 @@ watchdog { >>>>> }; >>>>> }; >>>>> + pm8916_charger: charger@1000 { >>>>> + compatible = "qcom,pm8916-lbc"; >>>>> + reg = <0x1000>, <0x1200>, <0x1300>, <0x1600>; >>>>> + reg-names = "chgr", "bat_if", "usb", "misc"; >>>>> + >>>>> + interrupts = <0x0 0x10 0 IRQ_TYPE_EDGE_BOTH>, >>>>> + <0x0 0x10 5 IRQ_TYPE_EDGE_BOTH>, >>>>> + <0x0 0x10 6 IRQ_TYPE_EDGE_BOTH>, >>>>> + <0x0 0x10 7 IRQ_TYPE_EDGE_BOTH>, >>>>> + <0x0 0x12 0 IRQ_TYPE_EDGE_BOTH>, >>>>> + <0x0 0x12 1 IRQ_TYPE_EDGE_BOTH>, >>>>> + <0x0 0x13 0 IRQ_TYPE_EDGE_BOTH>, >>>>> + <0x0 0x13 1 IRQ_TYPE_EDGE_BOTH>, >>>>> + <0x0 0x13 2 IRQ_TYPE_EDGE_BOTH>, >>>>> + <0x0 0x13 4 IRQ_TYPE_EDGE_BOTH>; >>>>> + interrupt-names = "vbat_det", >>>>> + "fast_chg", >>>>> + "chg_fail", >>>>> + "chg_done", >>>>> + "bat_pres", >>>>> + "temp_ok", >>>>> + "coarse_det", >>>>> + "usb_vbus", >>>> So, both the charger and the USBIN driver use the same irq? :/ >>>> >>> >>> AFAIU the usbin extcon driver pretty much just tracks the state >>> of the IRQ to report extcon. It happens to assume the same part >>> of the pmic though, yes, which also means there will be no user >>> that would enable both charger and vbus extcon, since charger >>> driver provides this functionality as well. >> So, should USBIN be removed from PM8916 dt since it's essentially >> a part of the charger block? >> > > The "USB_IN" pad of the PM8916 seems to be connected on pretty much all > devices, even if they are using external chargers and the charging > functionality of PM8916 is completely disabled. For those devices, the > &pm8916_usbin device provides a convenient way to detect the USB state, > even without a working charger driver. > > While we could modify the PM8916 charger driver and DT node to have some > special mode where charging and battery monitoring is completely > disabled and only the USBIN extcon is provided, I'm not sure if that > would provide a significant advantage compared to just keeping the > simple &pm8916_usbin node with the existing driver. Hmm okay I see.. Generally it's rather "no bueno" to have two DT nodes consuming the same register space.. What happens when you enable BMS on a device with a non-PM8916 charger? Does it correctly recognize "no battery" etc.? Konrad
Konrad Dybcio писал(а) 27.10.2023 01:03: > On 10/26/23 21:17, Stephan Gerhold wrote: >> On Thu, Oct 26, 2023 at 08:54:00PM +0200, Konrad Dybcio wrote: >>> On 10/24/23 11:29, Nikita Travkin wrote: >>>> Konrad Dybcio писал(а) 24.10.2023 13:34: >>>>> On 10/23/23 08:20, Nikita Travkin wrote: >>>>>> pm8916 contains some hardware blocks for battery powered devices: >>>>>> >>>>>> - VM-BMS: Battery voltage monitoring block. >>>>>> - LBC: Linear battery charger. >>>>>> >>>>>> Add them to the pmic dtsi so the devices that make use of those blocks >>>>>> can enable them. >>>>>> >>>>>> Signed-off-by: Nikita Travkin <nikita@trvn.ru> >>>>>> --- >>>>>> arch/arm64/boot/dts/qcom/pm8916.dtsi | 48 ++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 48 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm64/boot/dts/qcom/pm8916.dtsi b/arch/arm64/boot/dts/qcom/pm8916.dtsi >>>>>> index f4de86787743..4b2e8fb47d2d 100644 >>>>>> --- a/arch/arm64/boot/dts/qcom/pm8916.dtsi >>>>>> +++ b/arch/arm64/boot/dts/qcom/pm8916.dtsi >>>>>> @@ -41,6 +41,35 @@ watchdog { >>>>>> }; >>>>>> }; >>>>>> + pm8916_charger: charger@1000 { >>>>>> + compatible = "qcom,pm8916-lbc"; >>>>>> + reg = <0x1000>, <0x1200>, <0x1300>, <0x1600>; >>>>>> + reg-names = "chgr", "bat_if", "usb", "misc"; >>>>>> + >>>>>> + interrupts = <0x0 0x10 0 IRQ_TYPE_EDGE_BOTH>, >>>>>> + <0x0 0x10 5 IRQ_TYPE_EDGE_BOTH>, >>>>>> + <0x0 0x10 6 IRQ_TYPE_EDGE_BOTH>, >>>>>> + <0x0 0x10 7 IRQ_TYPE_EDGE_BOTH>, >>>>>> + <0x0 0x12 0 IRQ_TYPE_EDGE_BOTH>, >>>>>> + <0x0 0x12 1 IRQ_TYPE_EDGE_BOTH>, >>>>>> + <0x0 0x13 0 IRQ_TYPE_EDGE_BOTH>, >>>>>> + <0x0 0x13 1 IRQ_TYPE_EDGE_BOTH>, >>>>>> + <0x0 0x13 2 IRQ_TYPE_EDGE_BOTH>, >>>>>> + <0x0 0x13 4 IRQ_TYPE_EDGE_BOTH>; >>>>>> + interrupt-names = "vbat_det", >>>>>> + "fast_chg", >>>>>> + "chg_fail", >>>>>> + "chg_done", >>>>>> + "bat_pres", >>>>>> + "temp_ok", >>>>>> + "coarse_det", >>>>>> + "usb_vbus", >>>>> So, both the charger and the USBIN driver use the same irq? :/ >>>>> >>>> >>>> AFAIU the usbin extcon driver pretty much just tracks the state >>>> of the IRQ to report extcon. It happens to assume the same part >>>> of the pmic though, yes, which also means there will be no user >>>> that would enable both charger and vbus extcon, since charger >>>> driver provides this functionality as well. >>> So, should USBIN be removed from PM8916 dt since it's essentially >>> a part of the charger block? >>> >> >> The "USB_IN" pad of the PM8916 seems to be connected on pretty much all >> devices, even if they are using external chargers and the charging >> functionality of PM8916 is completely disabled. For those devices, the >> &pm8916_usbin device provides a convenient way to detect the USB state, >> even without a working charger driver. >> >> While we could modify the PM8916 charger driver and DT node to have some >> special mode where charging and battery monitoring is completely >> disabled and only the USBIN extcon is provided, I'm not sure if that >> would provide a significant advantage compared to just keeping the >> simple &pm8916_usbin node with the existing driver. > Hmm okay I see.. > > Generally it's rather "no bueno" to have two DT nodes consuming the > same register space.. What happens when you enable BMS on a device > with a non-PM8916 charger? Does it correctly recognize "no battery" > etc.? > The _charger and _bms are separate and communicate in a generic manner via power-supplies and supply core (see 3/3) so giving a different charger to _bms can work. If an external charger is present in the device, qcom mandates "external charger" optional line of the pmic to be tied, and _charger is then disabled. The driver bails out in this case, but _usbin could still be used. > Konrad
On 25/10/2023 14:57, Nikita Travkin wrote: > Lee Jones писал(а) 25.10.2023 17:21: >> On Tue, 24 Oct 2023, Nikita Travkin wrote: >> >>> Rob Herring писал(а) 23.10.2023 22:40: >>>> On Mon, 23 Oct 2023 11:20:32 +0500, Nikita Travkin wrote: >>>>> PM8916 (and probably some other similar pmics) have hardware blocks for >>>>> battery monitoring and charging. Add patterns for respecive nodes so the >>>>> devicetree for those blocks can be validated properly. >>>>> >>>>> Signed-off-by: Nikita Travkin <nikita@trvn.ru> >>>>> --- >>>>> Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.yaml | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>> >>>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' >>>> on your patch (DT_CHECKER_FLAGS is new in v5.13): >>>> >>>> yamllint warnings/errors: >>>> >>>> dtschema/dtc warnings/errors: >>>> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.yaml: >>>> Error in referenced schema matching $id: http://devicetree.org/schemas/power/supply/qcom,pm8916-bms-vm.yaml >>>> >>>> doc reference errors (make refcheckdocs): >>>> >>>> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231023-pm8916-dtsi-bms-lbc-v2-1-343e3dbf423e@trvn.ru >>>> >>>> The base for the series is generally the latest rc1. A different dependency >>>> should be noted in *this* patch. >>>> >>> >>> Somehow I missed the memo and thought it tracks -next... >>> >>> This patch depends on 7f590e3831 and 5cee843d56 in linux-next.git >>> They were applied in [1]. >>> >>> I'm wondering if the bot just bails out when the "depend" is present >>> or there is some more sophisticated logic to suggest the base to it? >> >> So is this good to go, or not? > > IMO this patch should be good, it passes the check on today's linux-next > on my end. It's not the next which matters, but maintainers tree. > > The only concern might be that if someone runs dt_binding_check on > for-mfd-next, it would skip that file with an error since there is no > dependency yet. Eee, so this has dependency on some other tree? Then no, it is not good to go. Best regards, Krzysztof
On 25/10/2023 17:44, Lee Jones wrote: > On Mon, 23 Oct 2023 11:20:32 +0500, Nikita Travkin wrote: >> PM8916 (and probably some other similar pmics) have hardware blocks for >> battery monitoring and charging. Add patterns for respecive nodes so the >> devicetree for those blocks can be validated properly. >> >> > > Applied, thanks! > > [1/3] dt-bindings: mfd: qcom,spmi-pmic: Add pm8916 vm-bms and lbc > commit: e9aec86e211ee493081e8934b8c821d660b417ee Hi Lee, It seems this patch depends on something not in your tree. This should have been clearly explained in cover letter or this patch changelog, but wasn't. Please drop the patch. Best regards, Krzysztof
On Fri, 27 Oct 2023, Krzysztof Kozlowski wrote: > On 25/10/2023 17:44, Lee Jones wrote: > > On Mon, 23 Oct 2023 11:20:32 +0500, Nikita Travkin wrote: > >> PM8916 (and probably some other similar pmics) have hardware blocks for > >> battery monitoring and charging. Add patterns for respecive nodes so the > >> devicetree for those blocks can be validated properly. > >> > >> > > > > Applied, thanks! > > > > [1/3] dt-bindings: mfd: qcom,spmi-pmic: Add pm8916 vm-bms and lbc > > commit: e9aec86e211ee493081e8934b8c821d660b417ee > > Hi Lee, > > It seems this patch depends on something not in your tree. This should > have been clearly explained in cover letter or this patch changelog, but > wasn't. > > Please drop the patch. Done.
On 27.10.2023 07:44, Nikita Travkin wrote: > Konrad Dybcio писал(а) 27.10.2023 01:03: >> On 10/26/23 21:17, Stephan Gerhold wrote: >>> On Thu, Oct 26, 2023 at 08:54:00PM +0200, Konrad Dybcio wrote: >>>> On 10/24/23 11:29, Nikita Travkin wrote: >>>>> Konrad Dybcio писал(а) 24.10.2023 13:34: >>>>>> On 10/23/23 08:20, Nikita Travkin wrote: >>>>>>> pm8916 contains some hardware blocks for battery powered devices: >>>>>>> >>>>>>> - VM-BMS: Battery voltage monitoring block. >>>>>>> - LBC: Linear battery charger. >>>>>>> >>>>>>> Add them to the pmic dtsi so the devices that make use of those blocks >>>>>>> can enable them. >>>>>>> >>>>>>> Signed-off-by: Nikita Travkin <nikita@trvn.ru> >>>>>>> --- >>>>>>> arch/arm64/boot/dts/qcom/pm8916.dtsi | 48 ++++++++++++++++++++++++++++++++++++ >>>>>>> 1 file changed, 48 insertions(+) >>>>>>> >>>>>>> diff --git a/arch/arm64/boot/dts/qcom/pm8916.dtsi b/arch/arm64/boot/dts/qcom/pm8916.dtsi >>>>>>> index f4de86787743..4b2e8fb47d2d 100644 >>>>>>> --- a/arch/arm64/boot/dts/qcom/pm8916.dtsi >>>>>>> +++ b/arch/arm64/boot/dts/qcom/pm8916.dtsi >>>>>>> @@ -41,6 +41,35 @@ watchdog { >>>>>>> }; >>>>>>> }; >>>>>>> + pm8916_charger: charger@1000 { >>>>>>> + compatible = "qcom,pm8916-lbc"; >>>>>>> + reg = <0x1000>, <0x1200>, <0x1300>, <0x1600>; >>>>>>> + reg-names = "chgr", "bat_if", "usb", "misc"; >>>>>>> + >>>>>>> + interrupts = <0x0 0x10 0 IRQ_TYPE_EDGE_BOTH>, >>>>>>> + <0x0 0x10 5 IRQ_TYPE_EDGE_BOTH>, >>>>>>> + <0x0 0x10 6 IRQ_TYPE_EDGE_BOTH>, >>>>>>> + <0x0 0x10 7 IRQ_TYPE_EDGE_BOTH>, >>>>>>> + <0x0 0x12 0 IRQ_TYPE_EDGE_BOTH>, >>>>>>> + <0x0 0x12 1 IRQ_TYPE_EDGE_BOTH>, >>>>>>> + <0x0 0x13 0 IRQ_TYPE_EDGE_BOTH>, >>>>>>> + <0x0 0x13 1 IRQ_TYPE_EDGE_BOTH>, >>>>>>> + <0x0 0x13 2 IRQ_TYPE_EDGE_BOTH>, >>>>>>> + <0x0 0x13 4 IRQ_TYPE_EDGE_BOTH>; >>>>>>> + interrupt-names = "vbat_det", >>>>>>> + "fast_chg", >>>>>>> + "chg_fail", >>>>>>> + "chg_done", >>>>>>> + "bat_pres", >>>>>>> + "temp_ok", >>>>>>> + "coarse_det", >>>>>>> + "usb_vbus", >>>>>> So, both the charger and the USBIN driver use the same irq? :/ >>>>>> >>>>> >>>>> AFAIU the usbin extcon driver pretty much just tracks the state >>>>> of the IRQ to report extcon. It happens to assume the same part >>>>> of the pmic though, yes, which also means there will be no user >>>>> that would enable both charger and vbus extcon, since charger >>>>> driver provides this functionality as well. >>>> So, should USBIN be removed from PM8916 dt since it's essentially >>>> a part of the charger block? >>>> >>> >>> The "USB_IN" pad of the PM8916 seems to be connected on pretty much all >>> devices, even if they are using external chargers and the charging >>> functionality of PM8916 is completely disabled. For those devices, the >>> &pm8916_usbin device provides a convenient way to detect the USB state, >>> even without a working charger driver. >>> >>> While we could modify the PM8916 charger driver and DT node to have some >>> special mode where charging and battery monitoring is completely >>> disabled and only the USBIN extcon is provided, I'm not sure if that >>> would provide a significant advantage compared to just keeping the >>> simple &pm8916_usbin node with the existing driver. >> Hmm okay I see.. >> >> Generally it's rather "no bueno" to have two DT nodes consuming the >> same register space.. What happens when you enable BMS on a device >> with a non-PM8916 charger? Does it correctly recognize "no battery" >> etc.? >> > > The _charger and _bms are separate and communicate in a generic > manner via power-supplies and supply core (see 3/3) so giving > a different charger to _bms can work. > > If an external charger is present in the device, qcom mandates > "external charger" optional line of the pmic to be tied, and > _charger is then disabled. The driver bails out in this case, > but _usbin could still be used. Meh.. I guess I'll reluctantly let it slide, unless Bjorn has some objections Konrad
Lee Jones писал(а) 31.10.2023 12:54: > On Fri, 27 Oct 2023, Krzysztof Kozlowski wrote: > >> On 25/10/2023 17:44, Lee Jones wrote: >> > On Mon, 23 Oct 2023 11:20:32 +0500, Nikita Travkin wrote: >> >> PM8916 (and probably some other similar pmics) have hardware blocks for >> >> battery monitoring and charging. Add patterns for respecive nodes so the >> >> devicetree for those blocks can be validated properly. >> >> >> >> >> > >> > Applied, thanks! >> > >> > [1/3] dt-bindings: mfd: qcom,spmi-pmic: Add pm8916 vm-bms and lbc >> > commit: e9aec86e211ee493081e8934b8c821d660b417ee >> >> Hi Lee, >> >> It seems this patch depends on something not in your tree. This should >> have been clearly explained in cover letter or this patch changelog, but >> wasn't. >> >> Please drop the patch. > > Done. Hi, v6.7-rc1 now includes the dependencies for this bindings change, could you pick it up again? Or maybe I should respin the series with it included back? Sorry for making this inconvenient for you... Nikita
This series enables VM-BMS and LBC blocks in the pm8916 pmic. The VM-BMS is a simple voltage monitoring block that allows the software to estimate the battery capacity left. The LBC is a linear battery charger for lipo batteries. Add both devices to the pmic dtsi and enable them for Longcheer L8150 which makes use of them. Signed-off-by: Nikita Travkin <nikita@trvn.ru> --- Changes in v2: - No changes - resend with minor commit message edits. - Link to v1: https://lore.kernel.org/r/20230916-pm8916-dtsi-bms-lbc-v1-0-7db0b42f9fb1@trvn.ru --- Nikita Travkin (3): dt-bindings: mfd: qcom,spmi-pmic: Add pm8916 vm-bms and lbc arm64: dts: qcom: pm8916: Add BMS and charger arm64: dts: qcom: msm8916-longcheer-l8150: Add battery and charger .../devicetree/bindings/mfd/qcom,spmi-pmic.yaml | 6 +++ .../boot/dts/qcom/msm8916-longcheer-l8150.dts | 43 ++++++++++++++++--- arch/arm64/boot/dts/qcom/pm8916.dtsi | 48 ++++++++++++++++++++++ 3 files changed, 91 insertions(+), 6 deletions(-) --- base-commit: 2030579113a1b1b5bfd7ff24c0852847836d8fd1 change-id: 20230916-pm8916-dtsi-bms-lbc-2fb1b99d1eb2 Best regards,