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;
MIME-Version: 1.0
References: <1660225318-4063-1-git-send-email-u0084500@gmail.com>
 <1660225318-4063-2-git-send-email-u0084500@gmail.com> <3cae9d60-4012-1dfd-abd9-4d0b9379e6bb@linaro.org>
In-Reply-To: <3cae9d60-4012-1dfd-abd9-4d0b9379e6bb@linaro.org>
From:   ChiYuan Huang <u0084500@gmail.com>
Date:   Fri, 12 Aug 2022 09:32:31 +0800
Message-ID: <CADiBU3_depGDZtiyizU3MB939A3oH1uTWzTMyruUy0z=u6BZkQ@mail.gmail.com>
Subject: Re: [PATCH 1/2] dt-bindings: power: supply: Add Richtek RT9471
 battery charger
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Sebastian Reichel <sre@kernel.org>,
        =?UTF-8?B?5ri45a2Q6aao?= <alina_yu@richtek.com>,
        cy_huang <cy_huang@richtek.com>, alinayu829@gmail.com,
        Linux PM <linux-pm@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, lkml <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> =E6=96=BC 2022=E5=B9=
=B48=E6=9C=8811=E6=97=A5 =E9=80=B1=E5=9B=9B =E6=99=9A=E4=B8=8A10:12=E5=AF=
=AB=E9=81=93=EF=BC=9A
>
> On 11/08/2022 16:41, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Add bindings for the Richtek RT9471 I2C controlled battery charger.
> >
>
> Thank you for your patch. There is something to discuss/improve.
>
> > +properties:
> > +  compatible:
> > +    const: richtek,rt9471
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  ceb-gpios:
> > +    maxItems: 1
>
> This looks not standard, so please provide a description.
It's the external 'charge enable' pin that's used to control battery chargi=
ng.
The priority is higher than the register 'CHG_EN' control.
In the word, 'b' means it's reverse logic, low to allow charging, high
to force disable charging.

description:
  External charge enable pin that can force control not to charge the batte=
ry.
  Low to allow charging, high to disable charging.

>
> > +
> > +  wakeup-source: true
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-controller: true
> > +
> > +  "#interrupt-cells":
> > +    const: 1
>
> Why a charger driver is a interrupt-controller?
There're 32 nested IRQs from RT9471.
The original thought is to make the user easy to bind the interrupt
into their driver.

For charger driver, does it mean legacy IRQ handler is more preferred?
>
> > +
> > +  usb-otg-vbus-regulator:
> > +    type: object
> > +    unevaluatedProperties: false
> > +    $ref: /schemas/regulator/regulator.yaml#
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - wakeup-source
> > +  - interrupts
> > +  - interrupt-controller
> > +  - "#interrupt-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c {
> > +      #address-cells =3D <1>;
> > +      #size-cells =3D <0>;
> > +
> > +      charger@53 {
> > +        compatible =3D "richtek,rt9471";
> > +        reg =3D <0x53>;
> > +        ceb-gpios =3D <&gpio26 1 0>;
>
> Isn't the last value a GPIO flag? If yes, use appropriate define.
I already specify GPIOD_OUT_LOW in the gpiod_request flag.
Do I need to convert the gpio request code to GPIOD_OUT_HIGH,
and specify here as GPIO_ACTIVE_LOW?
>
>
>
> Best regards,
> Krzysztof