Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 31 May 2023 17:36:18 +0800
From:   ChiYuan Huang <cy_huang@richtek.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
CC:     <sre@kernel.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <chiaen_wu@richtek.com>, <linux-pm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] dt-bindings: power: supply: Fix rt9467 charger enable
 gpio active level
Message-ID: <20230531093618.GA17005@linuxcarl2.richtek.com>
References: <1685522813-14481-1-git-send-email-cy_huang@richtek.com>
 <1f966e93-50c3-142e-620c-8fbb16e9b1a7@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <1f966e93-50c3-142e-620c-8fbb16e9b1a7@linaro.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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