diff mbox series

[v2,2/3] arm64: dts: qcom: pm8916: Add BMS and charger

Message ID	20231023-pm8916-dtsi-bms-lbc-v2-2-343e3dbf423e@trvn.ru
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5; From: Nikita Travkin <nikita@trvn.ru> Date: Mon, 23 Oct 2023 11:20:33 +0500 Subject: [PATCH v2 2/3] arm64: dts: qcom: pm8916: Add BMS and charger MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20231023-pm8916-dtsi-bms-lbc-v2-2-343e3dbf423e@trvn.ru> References: <20231023-pm8916-dtsi-bms-lbc-v2-0-343e3dbf423e@trvn.ru> In-Reply-To: <20231023-pm8916-dtsi-bms-lbc-v2-0-343e3dbf423e@trvn.ru> To: Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Lee Jones <lee@kernel.org>, Stephen Boyd <sboyd@kernel.org> Cc: linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, ~postmarketos/upstreaming@lists.sr.ht, Nikita Travkin <nikita@trvn.ru> Precedence: bulk
Series	pm8916: Add BMS and charger \| [v2,0/3] pm8916: Add BMS and charger [v2,1/3] dt-bindings: mfd: qcom,spmi-pmic: Add pm8916 vm-bms and lbc [v2,2/3] arm64: dts: qcom: pm8916: Add BMS and charger [v2,3/3] arm64: dts: qcom: msm8916-longcheer-l8150: Add battery and charger

Commit Message

Nikita Travkin Oct. 23, 2023, 6:20 a.m. UTC

  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(+)

Comments

Konrad Dybcio Oct. 24, 2023, 8:34 a.m. UTC | #1

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? :/

Konrad

Nikita Travkin Oct. 24, 2023, 9:29 a.m. UTC | #2

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.

Nikita

> Konrad

Konrad Dybcio Oct. 26, 2023, 6:54 p.m. UTC | #3

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

Stephan Gerhold Oct. 26, 2023, 7:17 p.m. UTC | #4

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

Konrad Dybcio Oct. 26, 2023, 8:03 p.m. UTC | #5

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

Nikita Travkin Oct. 27, 2023, 5:44 a.m. UTC | #6

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

Konrad Dybcio Oct. 31, 2023, 11:20 a.m. UTC | #7

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

diff mbox series

Patch

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",
+					  "chg_gone",
+					  "overtemp";
+
+			status = "disabled";
+		};
+
 		pm8916_usbin: usb-detect@1300 {
 			compatible = "qcom,pm8941-misc";
 			reg = <0x1300>;
@@ -91,6 +120,25 @@  channel@f {
 			};
 		};
 
+		pm8916_bms: battery@4000 {
+			compatible = "qcom,pm8916-bms-vm";
+			reg = <0x4000>;
+			interrupts = <0x0 0x40 0 IRQ_TYPE_EDGE_RISING>,
+				     <0x0 0x40 1 IRQ_TYPE_EDGE_RISING>,
+				     <0x0 0x40 2 IRQ_TYPE_EDGE_RISING>,
+				     <0x0 0x40 3 IRQ_TYPE_EDGE_RISING>,
+				     <0x0 0x40 4 IRQ_TYPE_EDGE_RISING>,
+				     <0x0 0x40 5 IRQ_TYPE_EDGE_RISING>;
+			interrupt-names = "cv_leave",
+					  "cv_enter",
+					  "ocv_good",
+					  "ocv_thr",
+					  "fifo",
+					  "state_chg";
+
+			status = "disabled";
+		};
+
 		rtc@6000 {
 			compatible = "qcom,pm8941-rtc";
 			reg = <0x6000>, <0x6100>;