linux-kernel - Re: [PATCH v3 1/8] dt-bindings: misc: Add mikrobus-connector

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <314a88e0-19cd-4b95-9cf3-aef1c7579eec@linaro.org>
Date: Fri, 15 Mar 2024 21:09:11 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Ayush Singh <ayushdevel1325@...il.com>, linux-kernel@...r.kernel.org
Cc: jkridner@...gleboard.org, robertcnelson@...gleboard.org,
 Vaishnav M A <vaishnav@...gleboard.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Nishanth Menon <nm@...com>,
 Vignesh Raghavendra <vigneshr@...com>, Tero Kristo <kristo@...nel.org>,
 Derek Kiernan <derek.kiernan@....com>, Dragan Cvetic
 <dragan.cvetic@....com>, Arnd Bergmann <arnd@...db.de>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Jiri Slaby <jirislaby@...nel.org>, Johan Hovold <johan@...nel.org>,
 Alex Elder <elder@...nel.org>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-spi@...r.kernel.org,
 linux-serial@...r.kernel.org, greybus-dev@...ts.linaro.org
Subject: Re: [PATCH v3 1/8] dt-bindings: misc: Add mikrobus-connector

On 15/03/2024 19:48, Ayush Singh wrote:
> Add DT bindings for mikroBUS interface. MikroBUS is an open standard
> developed by MikroElektronika for connecting add-on boards to
> microcontrollers or microprocessors.
> 
> Signed-off-by: Ayush Singh <ayushdevel1325@...il.com>
> ---
>  .../bindings/misc/mikrobus-connector.yaml     | 110 ++++++++++++++++++
>  MAINTAINERS                                   |   6 +
>  2 files changed, 116 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/mikrobus-connector.yaml
> 
> diff --git a/Documentation/devicetree/bindings/misc/mikrobus-connector.yaml b/Documentation/devicetree/bindings/misc/mikrobus-connector.yaml
> new file mode 100644
> index 000000000000..6eace2c0dddc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/mikrobus-connector.yaml

Please put it in connector directory.

> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/mikrobus-connector.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: mikroBUS add-on board socket
> +
> +maintainers:
> +  - Ayush Singh <ayushdevel1325@...il.com>
> +
> +properties:
> +  compatible:
> +    const: mikrobus-connector

Hm, why do you create binding for the connector, not for some sort of
controller? Please provide some rationale for this in commit msg.

> +
> +  pinctrl-0: true
> +  pinctrl-1: true
> +  pinctrl-2: true
> +  pinctrl-3: true
> +  pinctrl-4: true
> +  pinctrl-5: true
> +  pinctrl-6: true
> +  pinctrl-7: true
> +  pinctrl-8: true
> +
> +  pinctrl-names:
> +    items:
> +      - const: default
> +      - const: pwm_default
> +      - const: pwm_gpio
> +      - const: uart_default
> +      - const: uart_gpio
> +      - const: i2c_default
> +      - const: i2c_gpio
> +      - const: spi_default
> +      - const: spi_gpio

I fail to see why such choice is related to the connector itself.
Connector could have just SPI attached, so why all other entries needs
to be provided? Or is it fully plugable? But then really please explain
the hardware in the binding description.

> +
> +  mikrobus-gpios:
> +    minItems: 11
> +    maxItems: 12
> +
> +  i2c-adapter:
> +    description: i2c adapter attached to the mikrobus socket.
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +
> +  spi-controller:
> +    description: spi bus number of the spi-master attached to the mikrobus socket.
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +
> +  uart:
> +    description: uart port attached to the mikrobus socket
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +
> +  pwms:
> +    description: the pwm-controller corresponding to the mikroBUS PWM pin.
> +    maxItems: 1
> +
> +  spi-cs:
> +    description: spi chip-select numbers corresponding to the chip-selects on the mikrobus socket.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    items:
> +      - description: chip select corresponding to CS pin
> +      - description: chip select corresponding to RST pin

I don't understand why do you need all these properties. First, if this
is connector then I would rather see some sort of graph, not phandles.
Why would connector need to do anything with SPI controller?

All this looks like made for software. For the driver.

> +
> +required:
> +  - compatible
> +  - pinctrl-0
> +  - pinctrl-1
> +  - pinctrl-2
> +  - pinctrl-3
> +  - pinctrl-4
> +  - pinctrl-5
> +  - pinctrl-6
> +  - pinctrl-7
> +  - pinctrl-8
> +  - i2c-adapter
> +  - spi-controller
> +  - spi-cs
> +  - uart
> +  - pwms
> +  - mikrobus-gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +      mikrobus-0 {

mikrobus {

and fix the indentation. Use 4 spaces for example indentation.

> +        compatible = "mikrobus-connector";
> +        status = "okay";

Drop.

> +        pinctrl-names = "default", "pwm_default", "pwm_gpio","uart_default", "uart_gpio", "i2c_default",
> +                        "i2c_gpio", "spi_default", "spi_gpio";
> +        pinctrl-0 = <&P2_03_gpio_input_pin &P1_04_gpio_pin &P1_02_gpio_pin>;
> +        pinctrl-1 = <&P2_01_pwm_pin>;
> +        pinctrl-2 = <&P2_01_gpio_pin>;
> +        pinctrl-3 = <&P2_05_uart_pin &P2_07_uart_pin>;
> +        pinctrl-4 = <&P2_05_gpio_pin &P2_07_gpio_pin>;
> +        pinctrl-5 = <&P2_09_i2c_pin &P2_11_i2c_pin>;
> +        pinctrl-6 = <&P2_09_gpio_pin &P2_11_gpio_pin>;
> +        pinctrl-7 = <&P1_12_spi_pin &P1_10_spi_pin &P1_08_spi_sclk_pin &P1_06_spi_cs_pin>;
> +        pinctrl-8 = <&P1_12_gpio_pin &P1_10_gpio_pin &P1_08_gpio_pin &P1_06_gpio_pin>;
> +        i2c-adapter = <&i2c1>;
> +        spi-controller = <&spi1>;
> +        spi-cs = <0 1>;
> +        uart = <&uart1>;
> +        pwms = <&ehrpwm1 0 500000 0>;
> +        mikrobus-gpios = <&gpio1 18 0> , <&gpio0 23 0>, <&gpio0 30 0> , <&gpio0 31 0>, <&gpio0 15 0>,
> +                         <&gpio0 14 0>, <&gpio0 4 0> , <&gpio0 3 0>, <&gpio0 2 0>, <&gpio0 5 0>,
> +                         <&gpio2 25 0>, <&gpio2 3 0>;

Use proper defines for GPIO flags.



Best regards,
Krzysztof