From: ChiYuan Huang <[email protected]>
The RT9467 charger enable pin is an external signal that used to enable
battery charging. From the datasheet, the active level is low. Although
it's already configured to logic low at driver probe function, but the
current binding example declared it as 'GPIO_ACTIVE_LOW', this causes
this pin be output high and disable battery charging.
Fixes: e1b4620fb503 ("dt-bindings: power: supply: Add Richtek RT9467 battery charger")
Signed-off-by: ChiYuan Huang <[email protected]>
---
Hi,
This patch is to fix the active level for charger enable gpio polarity.
Currently, the wrong active level makes the user confused and
unexpectedly disable battery charging by default.
---
Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml b/Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml
index 3723717..cdc7678 100644
--- a/Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml
+++ b/Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml
@@ -69,7 +69,7 @@ examples:
reg = <0x5b>;
wakeup-source;
interrupts-extended = <&gpio_intc 32 IRQ_TYPE_LEVEL_LOW>;
- charge-enable-gpios = <&gpio26 1 GPIO_ACTIVE_LOW>;
+ charge-enable-gpios = <&gpio26 1 GPIO_ACTIVE_HIGH>;
rt9467_otg_vbus: usb-otg-vbus-regulator {
regulator-name = "rt9467-usb-otg-vbus";
--
2.7.4
On 31/05/2023 10:46, [email protected] wrote:
> From: ChiYuan Huang <[email protected]>
>
> The RT9467 charger enable pin is an external signal that used to enable
> battery charging. From the datasheet, the active level is low. Although
> it's already configured to logic low at driver probe function, but the
NAK.
You mix two different things. Driver behavior and DTS. Driver can
operate either on real level - matching hardware - or on logical level
(high as enable, low as disable). First choice is usually wrong, because
it does not allow inverted signals.
'Correcting' bindings to wrong approach is wrong. If the signal is
active low, then the flag is active low. Simple as that.
> current binding example declared it as 'GPIO_ACTIVE_LOW', this causes
> this pin be output high and disable battery charging.
>
> Fixes: e1b4620fb503 ("dt-bindings: power: supply: Add Richtek RT9467 battery charger")
> Signed-off-by: ChiYuan Huang <[email protected]>
> ---
> Hi,
>
> This patch is to fix the active level for charger enable gpio polarity.
This is just example - it does not fix anything...
> Currently, the wrong active level makes the user confused and
> unexpectedly disable battery charging by default.
> ---
> Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml b/Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml
> index 3723717..cdc7678 100644
> --- a/Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml
> +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml
> @@ -69,7 +69,7 @@ examples:
> reg = <0x5b>;
> wakeup-source;
> interrupts-extended = <&gpio_intc 32 IRQ_TYPE_LEVEL_LOW>;
> - charge-enable-gpios = <&gpio26 1 GPIO_ACTIVE_LOW>;
> + charge-enable-gpios = <&gpio26 1 GPIO_ACTIVE_HIGH>;
>
> rt9467_otg_vbus: usb-otg-vbus-regulator {
> regulator-name = "rt9467-usb-otg-vbus";
Best regards,
Krzysztof
On Wed, May 31, 2023 at 11:17:37AM +0200, Krzysztof Kozlowski wrote:
> On 31/05/2023 10:46, [email protected] wrote:
> > From: ChiYuan Huang <[email protected]>
> >
> > The RT9467 charger enable pin is an external signal that used to enable
> > battery charging. From the datasheet, the active level is low. Although
> > it's already configured to logic low at driver probe function, but the
>
> NAK.
>
> You mix two different things. Driver behavior and DTS. Driver can
> operate either on real level - matching hardware - or on logical level
> (high as enable, low as disable). First choice is usually wrong, because
> it does not allow inverted signals.
>
> 'Correcting' bindings to wrong approach is wrong. If the signal is
> active low, then the flag is active low. Simple as that.
>
If my understanding is right, so the correct way is to fix the driver code,
not binding exmaple.
> > current binding example declared it as 'GPIO_ACTIVE_LOW', this causes
> > this pin be output high and disable battery charging.
> >
> > Fixes: e1b4620fb503 ("dt-bindings: power: supply: Add Richtek RT9467 battery charger")
> > Signed-off-by: ChiYuan Huang <[email protected]>
> > ---
> > Hi,
> >
> > This patch is to fix the active level for charger enable gpio polarity.
>
> This is just example - it does not fix anything...
>
Sorry, this issue comes from the customer. They directly copy the example into their
platform dts. That's why originally I think it may be a fix.
Anyway, you're right. To maintain the maximum flexibility, the choice shouldn't be to fix
the example. It has to correct the driver code for this pin behavior.
Thanks.
> > Currently, the wrong active level makes the user confused and
> > unexpectedly disable battery charging by default.
> > ---
> > Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml b/Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml
> > index 3723717..cdc7678 100644
> > --- a/Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml
> > +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt9467.yaml
> > @@ -69,7 +69,7 @@ examples:
> > reg = <0x5b>;
> > wakeup-source;
> > interrupts-extended = <&gpio_intc 32 IRQ_TYPE_LEVEL_LOW>;
> > - charge-enable-gpios = <&gpio26 1 GPIO_ACTIVE_LOW>;
> > + charge-enable-gpios = <&gpio26 1 GPIO_ACTIVE_HIGH>;
> >
> > rt9467_otg_vbus: usb-otg-vbus-regulator {
> > regulator-name = "rt9467-usb-otg-vbus";
>
> Best regards,
> Krzysztof
>
On 31/05/2023 11:36, ChiYuan Huang wrote:
> On Wed, May 31, 2023 at 11:17:37AM +0200, Krzysztof Kozlowski wrote:
>> On 31/05/2023 10:46, [email protected] wrote:
>>> From: ChiYuan Huang <[email protected]>
>>>
>>> The RT9467 charger enable pin is an external signal that used to enable
>>> battery charging. From the datasheet, the active level is low. Although
>>> it's already configured to logic low at driver probe function, but the
>>
>> NAK.
>>
>> You mix two different things. Driver behavior and DTS. Driver can
>> operate either on real level - matching hardware - or on logical level
>> (high as enable, low as disable). First choice is usually wrong, because
>> it does not allow inverted signals.
>>
>> 'Correcting' bindings to wrong approach is wrong. If the signal is
>> active low, then the flag is active low. Simple as that.
>>
> If my understanding is right, so the correct way is to fix the driver code,
> not binding exmaple.
Yes, that's my opinion. The DTS should describe the hardware and in the
hardware the pin is active low.
Now the driver should operate on logical state, so it want to enable
(value of 1) or disable something (value of 0).
Best regards,
Krzysztof