Krzysztof Kozlowski [off-list ref] 於 2022年8月12日 週五 下午2:54寫道：

On 12/08/2022 04:32, ChiYuan Huang wrote:

quoted

Krzysztof Kozlowski [off-list ref] 於 2022年8月11日 週四 晚上10:12寫道：

quoted

On 11/08/2022 16:41, cy_huang wrote:

quoted

From: ChiYuan Huang <redacted>

Add bindings for the Richtek RT9471 I2C controlled battery charger.

Thank you for your patch. There is something to discuss/improve.

quoted

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

Isn't this standard enable-gpios property?

Not the same thing, this charger includes power patch control.
This gpio is used to 'force disable' charge the battery.

quoted

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

quoted

quoted

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

Bindings are not related to the driver but to hardware...

Sorry, I mislead  your comment.
Refer to bq2515x.yaml, I think it's better to change this property to
'charge-enable-gpios'.
It's the same usage like as TI charger.

quoted

For charger driver, does it mean legacy IRQ handler is more preferred?

Who is the consumer of these interrupts? Can you show the DTS with the
interrupt consumer?

quoted

quoted

quoted

+
+  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 = <1>;
+      #size-cells = <0>;
+
+      charger@53 {
+        compatible = "richtek,rt9471";
+        reg = <0x53>;
+        ceb-gpios = <&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.

It is not related to the DTS. Anyway writing "low" for a meaning of high
is not correct usually...

quoted

Do I need to convert the gpio request code to GPIOD_OUT_HIGH,
and specify here as GPIO_ACTIVE_LOW?

You need to properly describe the hardware. The polarity of logical
signal is defined by DTS, not by driver. It does not make sense to do it
in driver. What if on some board the signal is inverted?

From our discussion, binding example just keep the active level that the pin is.
So 'GPIO_ACTIVE_LOW', thanks.

All of the above will be fixed in the next revision.

Best regards,
Krzysztof