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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ