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;
Message-ID: <1f966e93-50c3-142e-620c-8fbb16e9b1a7@linaro.org>
Date:   Wed, 31 May 2023 11:17:37 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] dt-bindings: power: supply: Fix rt9467 charger enable
 gpio active level
Content-Language: en-US
To:     cy_huang@richtek.com, sre@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org
Cc:     chiaen_wu@richtek.com, linux-pm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <1685522813-14481-1-git-send-email-cy_huang@richtek.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <1685522813-14481-1-git-send-email-cy_huang@richtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

> 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...

> 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