On Wed, May 31, 2023 at 11:17:37AM +0200, Krzysztof Kozlowski wrote:
> On 31/05/2023 10:46, cy_huang@xxxxxxxxxxx wrote:
> > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> >
> > 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 <cy_huang@xxxxxxxxxxx>
> > ---
> > 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
>
