Re: [PATCH] dt-bindings: power: supply: Fix rt9467 charger enable gpio active level

[ChiYuan Huang]

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
>