2022-08-11 13:55:09

by ChiYuan Huang

[permalink] [raw]
Subject: [PATCH 1/2] dt-bindings: power: supply: Add Richtek RT9471 battery charger

From: ChiYuan Huang <[email protected]>

Add bindings for the Richtek RT9471 I2C controlled battery charger.

Co-developed-by: Alina Yu <[email protected]>
Signed-off-by: Alina Yu <[email protected]>
Signed-off-by: ChiYuan Huang <[email protected]>
---
.../bindings/power/supply/richtek,rt9471.yaml | 78 ++++++++++++++++++++++
1 file changed, 78 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/richtek,rt9471.yaml

diff --git a/Documentation/devicetree/bindings/power/supply/richtek,rt9471.yaml b/Documentation/devicetree/bindings/power/supply/richtek,rt9471.yaml
new file mode 100644
index 00000000..6fef1bd
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/richtek,rt9471.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/richtek,rt9471.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RT9471 3A Single Cell Switching Battery charger
+
+maintainers:
+ - Alina Yu <[email protected]>
+ - ChiYuan Huang <[email protected]>
+
+description: |
+ RT9471 is a switch-mode single cell Li-Ion/Li-Polymer battery charger for
+ portable applications. It supports USB BC1.2 port detection, current and
+ voltage regulations in both charging and boost mode.
+
+ Datasheet is available at
+ https://www.richtek.com/assets/product_file/RT9471=RT9471D/DS9471D-02.pdf
+
+properties:
+ compatible:
+ const: richtek,rt9471
+
+ reg:
+ maxItems: 1
+
+ ceb-gpios:
+ maxItems: 1
+
+ wakeup-source: true
+
+ interrupts:
+ maxItems: 1
+
+ interrupt-controller: true
+
+ "#interrupt-cells":
+ const: 1
+
+ usb-otg-vbus-regulator:
+ type: object
+ unevaluatedProperties: false
+ $ref: /schemas/regulator/regulator.yaml#
+
+required:
+ - compatible
+ - reg
+ - wakeup-source
+ - interrupts
+ - interrupt-controller
+ - "#interrupt-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ charger@53 {
+ compatible = "richtek,rt9471";
+ reg = <0x53>;
+ ceb-gpios = <&gpio26 1 0>;
+ wakeup-source;
+ interrupts-extended = <&gpio1 32 IRQ_TYPE_EDGE_FALLING>;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ usb-otg-vbus-regulator {
+ regulator-compatible = "usb-otg-vbus";
+ regulator-min-microvolt = <4850000>;
+ regulator-max-microvolt = <5300000>;
+ };
+ };
+ };
--
2.7.4


2022-08-11 14:33:34

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: supply: Add Richtek RT9471 battery charger

On 11/08/2022 16:41, cy_huang wrote:
> From: ChiYuan Huang <[email protected]>
>
> Add bindings for the Richtek RT9471 I2C controlled battery charger.
>

Thank you for your patch. There is something to discuss/improve.

> +properties:
> + compatible:
> + const: richtek,rt9471
> +
> + reg:
> + maxItems: 1
> +
> + ceb-gpios:
> + maxItems: 1

This looks not standard, so please provide a description.

> +
> + wakeup-source: true
> +
> + interrupts:
> + maxItems: 1
> +
> + interrupt-controller: true
> +
> + "#interrupt-cells":
> + const: 1

Why a charger driver is a interrupt-controller?

> +
> + usb-otg-vbus-regulator:
> + type: object
> + unevaluatedProperties: false
> + $ref: /schemas/regulator/regulator.yaml#
> +
> +required:
> + - compatible
> + - reg
> + - wakeup-source
> + - interrupts
> + - interrupt-controller
> + - "#interrupt-cells"
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + charger@53 {
> + compatible = "richtek,rt9471";
> + reg = <0x53>;
> + ceb-gpios = <&gpio26 1 0>;

Isn't the last value a GPIO flag? If yes, use appropriate define.



Best regards,
Krzysztof

2022-08-12 01:37:32

by ChiYuan Huang

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: supply: Add Richtek RT9471 battery charger

Krzysztof Kozlowski <[email protected]> 於 2022年8月11日 週四 晚上10:12寫道:
>
> On 11/08/2022 16:41, cy_huang wrote:
> > From: ChiYuan Huang <[email protected]>
> >
> > Add bindings for the Richtek RT9471 I2C controlled battery charger.
> >
>
> Thank you for your patch. There is something to discuss/improve.
>
> > +properties:
> > + compatible:
> > + const: richtek,rt9471
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + ceb-gpios:
> > + maxItems: 1
>
> This looks not standard, so please provide a description.
It's the external 'charge enable' pin that's used to control battery charging.
The priority is higher than the register 'CHG_EN' control.
In the word, 'b' means it's reverse logic, low to allow charging, high
to force disable charging.

description:
External charge enable pin that can force control not to charge the battery.
Low to allow charging, high to disable charging.

>
> > +
> > + wakeup-source: true
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + interrupt-controller: true
> > +
> > + "#interrupt-cells":
> > + const: 1
>
> Why a charger driver is a interrupt-controller?
There're 32 nested IRQs from RT9471.
The original thought is to make the user easy to bind the interrupt
into their driver.

For charger driver, does it mean legacy IRQ handler is more preferred?
>
> > +
> > + usb-otg-vbus-regulator:
> > + type: object
> > + unevaluatedProperties: false
> > + $ref: /schemas/regulator/regulator.yaml#
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - wakeup-source
> > + - interrupts
> > + - interrupt-controller
> > + - "#interrupt-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + charger@53 {
> > + compatible = "richtek,rt9471";
> > + reg = <0x53>;
> > + ceb-gpios = <&gpio26 1 0>;
>
> Isn't the last value a GPIO flag? If yes, use appropriate define.
I already specify GPIOD_OUT_LOW in the gpiod_request flag.
Do I need to convert the gpio request code to GPIOD_OUT_HIGH,
and specify here as GPIO_ACTIVE_LOW?
>
>
>
> Best regards,
> Krzysztof

2022-08-12 07:09:07

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: supply: Add Richtek RT9471 battery charger

On 12/08/2022 04:32, ChiYuan Huang wrote:
> Krzysztof Kozlowski <[email protected]> 於 2022年8月11日 週四 晚上10:12寫道:
>>
>> On 11/08/2022 16:41, cy_huang wrote:
>>> From: ChiYuan Huang <[email protected]>
>>>
>>> Add bindings for the Richtek RT9471 I2C controlled battery charger.
>>>
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> +properties:
>>> + compatible:
>>> + const: richtek,rt9471
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + ceb-gpios:
>>> + maxItems: 1
>>
>> This looks not standard, so please provide a description.
> It's the external 'charge enable' pin that's used to control battery charging.
> The priority is higher than the register 'CHG_EN' control.
> In the word, 'b' means it's reverse logic, low to allow charging, high
> to force disable charging.

Isn't this standard enable-gpios property?

>
> description:
> External charge enable pin that can force control not to charge the battery.
> Low to allow charging, high to disable charging.
>
>>
>>> +
>>> + wakeup-source: true
>>> +
>>> + interrupts:
>>> + maxItems: 1
>>> +
>>> + interrupt-controller: true
>>> +
>>> + "#interrupt-cells":
>>> + const: 1
>>
>> Why a charger driver is a interrupt-controller?
> There're 32 nested IRQs from RT9471.
> The original thought is to make the user easy to bind the interrupt
> into their driver.

Bindings are not related to the driver but to hardware...

>
> For charger driver, does it mean legacy IRQ handler is more preferred?

Who is the consumer of these interrupts? Can you show the DTS with the
interrupt consumer?

>>
>>> +
>>> + usb-otg-vbus-regulator:
>>> + type: object
>>> + unevaluatedProperties: false
>>> + $ref: /schemas/regulator/regulator.yaml#
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> + - wakeup-source
>>> + - interrupts
>>> + - interrupt-controller
>>> + - "#interrupt-cells"
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>> + i2c {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> +
>>> + charger@53 {
>>> + compatible = "richtek,rt9471";
>>> + reg = <0x53>;
>>> + ceb-gpios = <&gpio26 1 0>;
>>
>> Isn't the last value a GPIO flag? If yes, use appropriate define.
> I already specify GPIOD_OUT_LOW in the gpiod_request flag.

It is not related to the DTS. Anyway writing "low" for a meaning of high
is not correct usually...

> Do I need to convert the gpio request code to GPIOD_OUT_HIGH,
> and specify here as GPIO_ACTIVE_LOW?

You need to properly describe the hardware. The polarity of logical
signal is defined by DTS, not by driver. It does not make sense to do it
in driver. What if on some board the signal is inverted?

Best regards,
Krzysztof

2022-08-12 15:17:36

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: supply: Add Richtek RT9471 battery charger

On Thu, 11 Aug 2022 21:41:57 +0800, cy_huang wrote:
> From: ChiYuan Huang <[email protected]>
>
> Add bindings for the Richtek RT9471 I2C controlled battery charger.
>
> Co-developed-by: Alina Yu <[email protected]>
> Signed-off-by: Alina Yu <[email protected]>
> Signed-off-by: ChiYuan Huang <[email protected]>
> ---
> .../bindings/power/supply/richtek,rt9471.yaml | 78 ++++++++++++++++++++++
> 1 file changed, 78 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/richtek,rt9471.yaml
>

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/linux-dt-review/Documentation/devicetree/bindings/power/supply/richtek,rt9471.example.dtb: charger@53: usb-otg-vbus-regulator: Unevaluated properties are not allowed ('regulator-compatible' was unexpected)
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/supply/richtek,rt9471.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

2022-08-12 16:07:10

by ChiYuan Huang

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: supply: Add Richtek RT9471 battery charger

Krzysztof Kozlowski <[email protected]> 於 2022年8月12日 週五 下午2:54寫道:
>
> On 12/08/2022 04:32, ChiYuan Huang wrote:
> > Krzysztof Kozlowski <[email protected]> 於 2022年8月11日 週四 晚上10:12寫道:
> >>
> >> On 11/08/2022 16:41, cy_huang wrote:
> >>> From: ChiYuan Huang <[email protected]>
> >>>
> >>> Add bindings for the Richtek RT9471 I2C controlled battery charger.
> >>>
> >>
> >> Thank you for your patch. There is something to discuss/improve.
> >>
> >>> +properties:
> >>> + compatible:
> >>> + const: richtek,rt9471
> >>> +
> >>> + reg:
> >>> + maxItems: 1
> >>> +
> >>> + ceb-gpios:
> >>> + maxItems: 1
> >>
> >> This looks not standard, so please provide a description.
> > It's the external 'charge enable' pin that's used to control battery charging.
> > The priority is higher than the register 'CHG_EN' control.
> > In the word, 'b' means it's reverse logic, low to allow charging, high
> > to force disable charging.
>
> Isn't this standard enable-gpios property?
Not the same thing, this charger includes power patch control.
This gpio is used to 'force disable' charge the battery.
>
> >
> > description:
> > External charge enable pin that can force control not to charge the battery.
> > Low to allow charging, high to disable charging.
> >
> >>
> >>> +
> >>> + wakeup-source: true
> >>> +
> >>> + interrupts:
> >>> + maxItems: 1
> >>> +
> >>> + interrupt-controller: true
> >>> +
> >>> + "#interrupt-cells":
> >>> + const: 1
> >>
> >> Why a charger driver is a interrupt-controller?
> > There're 32 nested IRQs from RT9471.
> > The original thought is to make the user easy to bind the interrupt
> > into their driver.
>
> Bindings are not related to the driver but to hardware...
>
Sorry, I mislead your comment.
Refer to bq2515x.yaml, I think it's better to change this property to
'charge-enable-gpios'.
It's the same usage like as TI charger.
> >
> > For charger driver, does it mean legacy IRQ handler is more preferred?
>
> Who is the consumer of these interrupts? Can you show the DTS with the
> interrupt consumer?
>
> >>
> >>> +
> >>> + usb-otg-vbus-regulator:
> >>> + type: object
> >>> + unevaluatedProperties: false
> >>> + $ref: /schemas/regulator/regulator.yaml#
> >>> +
> >>> +required:
> >>> + - compatible
> >>> + - reg
> >>> + - wakeup-source
> >>> + - interrupts
> >>> + - interrupt-controller
> >>> + - "#interrupt-cells"
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> + - |
> >>> + #include <dt-bindings/interrupt-controller/irq.h>
> >>> + i2c {
> >>> + #address-cells = <1>;
> >>> + #size-cells = <0>;
> >>> +
> >>> + charger@53 {
> >>> + compatible = "richtek,rt9471";
> >>> + reg = <0x53>;
> >>> + ceb-gpios = <&gpio26 1 0>;
> >>
> >> Isn't the last value a GPIO flag? If yes, use appropriate define.
> > I already specify GPIOD_OUT_LOW in the gpiod_request flag.
>
> It is not related to the DTS. Anyway writing "low" for a meaning of high
> is not correct usually...
>
> > Do I need to convert the gpio request code to GPIOD_OUT_HIGH,
> > and specify here as GPIO_ACTIVE_LOW?
>
> You need to properly describe the hardware. The polarity of logical
> signal is defined by DTS, not by driver. It does not make sense to do it
> in driver. What if on some board the signal is inverted?
>
From our discussion, binding example just keep the active level that the pin is.
So 'GPIO_ACTIVE_LOW', thanks.

All of the above will be fixed in the next revision.

> Best regards,
> Krzysztof

2022-08-12 16:11:08

by ChiYuan Huang

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: supply: Add Richtek RT9471 battery charger

Rob Herring <[email protected]> 於 2022年8月12日 週五 晚上11:13寫道:
>
> On Thu, 11 Aug 2022 21:41:57 +0800, cy_huang wrote:
> > From: ChiYuan Huang <[email protected]>
> >
> > Add bindings for the Richtek RT9471 I2C controlled battery charger.
> >
> > Co-developed-by: Alina Yu <[email protected]>
> > Signed-off-by: Alina Yu <[email protected]>
> > Signed-off-by: ChiYuan Huang <[email protected]>
> > ---
> > .../bindings/power/supply/richtek,rt9471.yaml | 78 ++++++++++++++++++++++
> > 1 file changed, 78 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/power/supply/richtek,rt9471.yaml
> >
>
> 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/linux-dt-review/Documentation/devicetree/bindings/power/supply/richtek,rt9471.example.dtb: charger@53: usb-otg-vbus-regulator: Unevaluated properties are not allowed ('regulator-compatible' was unexpected)
> From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/supply/richtek,rt9471.yaml
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/patch/
>
Thanks, after I add 'DT_CHECKER_FLAGS=-m', it also can be found for this error.

This is typo, not 'regulator-compatible', it's 'regulator-name'.

Will be fixed in next revision.
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit.
>

2022-08-12 16:29:33

by ChiYuan Huang

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: supply: Add Richtek RT9471 battery charger

ChiYuan Huang <[email protected]> 於 2022年8月12日 週五 晚上11:57寫道:
>
> Krzysztof Kozlowski <[email protected]> 於 2022年8月12日 週五 下午2:54寫道:
> >
> > On 12/08/2022 04:32, ChiYuan Huang wrote:
> > > Krzysztof Kozlowski <[email protected]> 於 2022年8月11日 週四 晚上10:12寫道:
> > >>
> > >> On 11/08/2022 16:41, cy_huang wrote:
> > >>> From: ChiYuan Huang <[email protected]>
> > >>>
> > >>> Add bindings for the Richtek RT9471 I2C controlled battery charger.
> > >>>
> > >>
> > >> Thank you for your patch. There is something to discuss/improve.
> > >>
> > >>> +properties:
> > >>> + compatible:
> > >>> + const: richtek,rt9471
> > >>> +
> > >>> + reg:
> > >>> + maxItems: 1
> > >>> +
> > >>> + ceb-gpios:
> > >>> + maxItems: 1
> > >>
> > >> This looks not standard, so please provide a description.
> > > It's the external 'charge enable' pin that's used to control battery charging.
> > > The priority is higher than the register 'CHG_EN' control.
> > > In the word, 'b' means it's reverse logic, low to allow charging, high
> > > to force disable charging.
> >
> > Isn't this standard enable-gpios property?
> Not the same thing, this charger includes power patch control.
> This gpio is used to 'force disable' charge the battery.
> >
> > >
> > > description:
> > > External charge enable pin that can force control not to charge the battery.
> > > Low to allow charging, high to disable charging.
> > >
> > >>
> > >>> +
> > >>> + wakeup-source: true
> > >>> +
> > >>> + interrupts:
> > >>> + maxItems: 1
> > >>> +
> > >>> + interrupt-controller: true
> > >>> +
> > >>> + "#interrupt-cells":
> > >>> + const: 1
> > >>
> > >> Why a charger driver is a interrupt-controller?
> > > There're 32 nested IRQs from RT9471.
> > > The original thought is to make the user easy to bind the interrupt
> > > into their driver.
> >
> > Bindings are not related to the driver but to hardware...
> >
> Sorry, I mislead your comment.
> Refer to bq2515x.yaml, I think it's better to change this property to
> 'charge-enable-gpios'.
> It's the same usage like as TI charger.
> > >
> > > For charger driver, does it mean legacy IRQ handler is more preferred?
> >
> > Who is the consumer of these interrupts? Can you show the DTS with the
> > interrupt consumer?
> >
Sorry, I forget to reply this question.
Some battery driver may need to know the 'full', 'recharge' , 'ieoc' status.
The usage will be like as below

battery {
interrupts-extended = <&rt9471_chg 2 0>, <&rt9471_chg 3 0>, &(rt9471_chg 5 0>;
interrupt-names = "chg-done", "chg-recharge", "chg-ieoc";
};

Some gauge HW needs this information to enhance the battery capacity accuracy.

> > >>
> > >>> +
> > >>> + usb-otg-vbus-regulator:
> > >>> + type: object
> > >>> + unevaluatedProperties: false
> > >>> + $ref: /schemas/regulator/regulator.yaml#
> > >>> +
> > >>> +required:
> > >>> + - compatible
> > >>> + - reg
> > >>> + - wakeup-source
> > >>> + - interrupts
> > >>> + - interrupt-controller
> > >>> + - "#interrupt-cells"
> > >>> +
> > >>> +additionalProperties: false
> > >>> +
> > >>> +examples:
> > >>> + - |
> > >>> + #include <dt-bindings/interrupt-controller/irq.h>
> > >>> + i2c {
> > >>> + #address-cells = <1>;
> > >>> + #size-cells = <0>;
> > >>> +
> > >>> + charger@53 {
> > >>> + compatible = "richtek,rt9471";
> > >>> + reg = <0x53>;
> > >>> + ceb-gpios = <&gpio26 1 0>;
> > >>
> > >> Isn't the last value a GPIO flag? If yes, use appropriate define.
> > > I already specify GPIOD_OUT_LOW in the gpiod_request flag.
> >
> > It is not related to the DTS. Anyway writing "low" for a meaning of high
> > is not correct usually...
> >
> > > Do I need to convert the gpio request code to GPIOD_OUT_HIGH,
> > > and specify here as GPIO_ACTIVE_LOW?
> >
> > You need to properly describe the hardware. The polarity of logical
> > signal is defined by DTS, not by driver. It does not make sense to do it
> > in driver. What if on some board the signal is inverted?
> >
> From our discussion, binding example just keep the active level that the pin is.
> So 'GPIO_ACTIVE_LOW', thanks.
>
> All of the above will be fixed in the next revision.
>
> > Best regards,
> > Krzysztof

2022-08-12 19:08:45

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: supply: Add Richtek RT9471 battery charger

On 12/08/2022 19:05, ChiYuan Huang wrote:
>> It's the same usage like as TI charger.
>>>>
>>>> For charger driver, does it mean legacy IRQ handler is more preferred?
>>>
>>> Who is the consumer of these interrupts? Can you show the DTS with the
>>> interrupt consumer?
>>>
> Sorry, I forget to reply this question.
> Some battery driver may need to know the 'full', 'recharge' , 'ieoc' status.
> The usage will be like as below
>
> battery {
> interrupts-extended = <&rt9471_chg 2 0>, <&rt9471_chg 3 0>, &(rt9471_chg 5 0>;
> interrupt-names = "chg-done", "chg-recharge", "chg-ieoc";
> };
>
> Some gauge HW needs this information to enhance the battery capacity accuracy.

Other supply stack pieces do it via supplies (supplied to/from in
include/linux/power_supply.h) and reporting power_supply_changed().

With such explanation, your device is an interrupt source, but it is not
an interrupt controller. If your device is interrupt controller, it
means someone routes the interrupt line to your device. Physical line.

Best regards,
Krzysztof

2022-08-13 15:12:59

by ChiYuan Huang

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: supply: Add Richtek RT9471 battery charger

Krzysztof Kozlowski <[email protected]> 於 2022年8月13日 週六 凌晨2:53寫道:
>
> On 12/08/2022 19:05, ChiYuan Huang wrote:
> >> It's the same usage like as TI charger.
> >>>>
> >>>> For charger driver, does it mean legacy IRQ handler is more preferred?
> >>>
> >>> Who is the consumer of these interrupts? Can you show the DTS with the
> >>> interrupt consumer?
> >>>
> > Sorry, I forget to reply this question.
> > Some battery driver may need to know the 'full', 'recharge' , 'ieoc' status.
> > The usage will be like as below
> >
> > battery {
> > interrupts-extended = <&rt9471_chg 2 0>, <&rt9471_chg 3 0>, &(rt9471_chg 5 0>;
> > interrupt-names = "chg-done", "chg-recharge", "chg-ieoc";
> > };
> >
> > Some gauge HW needs this information to enhance the battery capacity accuracy.
>
> Other supply stack pieces do it via supplies (supplied to/from in
> include/linux/power_supply.h) and reporting power_supply_changed().
>
> With such explanation, your device is an interrupt source, but it is not
> an interrupt controller. If your device is interrupt controller, it
> means someone routes the interrupt line to your device. Physical line.
>
Yap, sure. And so on, just use the SW power supply chain to do this
kind of event notification.
To remove it, it doesn't affect the internal interrupt request inside
the driver.
Just cannot be used for the outer driver to request the events directly.

If so, I think 'interrupt-controller' and even '#interrupt-cells' need
to be removed.
OK?
> Best regards,
> Krzysztof

2022-08-16 09:50:23

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: supply: Add Richtek RT9471 battery charger

On 13/08/2022 17:52, ChiYuan Huang wrote:
>>> Some gauge HW needs this information to enhance the battery capacity accuracy.
>>
>> Other supply stack pieces do it via supplies (supplied to/from in
>> include/linux/power_supply.h) and reporting power_supply_changed().
>>
>> With such explanation, your device is an interrupt source, but it is not
>> an interrupt controller. If your device is interrupt controller, it
>> means someone routes the interrupt line to your device. Physical line.
>>
> Yap, sure. And so on, just use the SW power supply chain to do this
> kind of event notification.
> To remove it, it doesn't affect the internal interrupt request inside
> the driver.
> Just cannot be used for the outer driver to request the events directly.
>
> If so, I think 'interrupt-controller' and even '#interrupt-cells' need
> to be removed.
> OK?

Yes, both should be removed. Your device is not an interrupt controller...

Best regards,
Krzysztof