linux-kernel - Re: [PATCH v1 1/2] dt-bindings: i2c: add binding for i2c-hotplug-gpio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f26edf98-617f-cb37-d34d-497fce5e0867@linaro.org>
Date:   Mon, 19 Jun 2023 16:45:53 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Svyatoslav Ryhel <clamor95@...il.com>,
        Andi Shyti <andi.shyti@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Wolfram Sang <wsa@...nel.org>,
        Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc:     linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] dt-bindings: i2c: add binding for i2c-hotplug-gpio

On 19/06/2023 16:36, Svyatoslav Ryhel wrote:
> Document device tree schema which describes hot-pluggable via GPIO
> i2c bus.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@...il.com>
> ---
>  .../bindings/i2c/i2c-hotplug-gpio.yaml        | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml b/Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml
> new file mode 100644
> index 000000000000..74544687a2b8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml
> @@ -0,0 +1,68 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/i2c-hotplug-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GPIO detected hot-plugged I2C bus
> +
> +maintainers:
> +  - Michał Mirosław <mirq-linux@...e.qmqm.pl>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  Driver for hot-plugged I2C busses, where some devices on a bus
> +  are hot-pluggable and their presence is indicated by GPIO line.
> +
> +properties:
> +  $nodename:
> +    pattern: "^i2c-(.*)?"

Drop

> +
> +  compatible:
> +    items:
> +      - const: i2c-hotplug-gpio
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  interrupts-extended:
> +    minItems: 1
> +
> +  detect-gpios:
> +    maxItems: 1
> +
> +  i2c-parent:
> +    maxItems: 1

I don't understand this part. You built it as a complimentary device to
the I2C controller, but there is no such device as "hotplug I2C", right?
The GPIO is part of the controller and this is imaginary (virtual) device?

Otherwise, where does the "detect-gpios" go? To the SoC? Then it is not
a real device...

> +
> +required:
> +  - compatible
> +  - "#address-cells"
> +  - "#size-cells"

Use consistent quotes (' or ").

> +  - interrupts-extended
> +  - detect-gpios
> +  - i2c-parent
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    /*
> +     * Asus Transformers use I2C hotplug for attachable dock keyboard
> +     */
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c-dock {
> +        compatible = "i2c-hotplug-gpio";
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        interrupts-extended = <&gpio 164 IRQ_TYPE_EDGE_BOTH>;
> +        detect-gpios = <&gpio 164 1>;

You forgot define.

> +
> +        i2c-parent = <&{/i2c@...0c400}>;

Use normal phandles/labels like entire DTS, not full paths or node names.

> +    };
> +...

Best regards,
Krzysztof