[PATCH] dt-bindings: pinctrl: convert pinctrl-mcp23s08.txt to yaml format

[PATCH] dt-bindings: pinctrl: convert pinctrl-mcp23s08.txt to yaml format

[Himanshu Bhavani]

Convert the text bindings of pinctrl-mcp23s08 to YAML so it could be used to
validate the DTS.

Signed-off-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
---
 .../bindings/pinctrl/pinctrl-mcp23s08.txt     | 148 -----------------
 .../bindings/pinctrl/pinctrl-mcp23s08.yaml    | 153 ++++++++++++++++++
 2 files changed, 153 insertions(+), 148 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
 create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
deleted file mode 100644
index 2fa5edac7a35..000000000000
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
+++ /dev/null
@@ -1,148 +0,0 @@
-Microchip MCP2308/MCP23S08/MCP23017/MCP23S17 driver for
-8-/16-bit I/O expander with serial interface (I2C/SPI)
-
-Required properties:
-- compatible : Should be
-    - "mcp,mcp23s08" (DEPRECATED) for  8 GPIO SPI version
-    - "mcp,mcp23s17" (DEPRECATED) for 16 GPIO SPI version
-    - "mcp,mcp23008" (DEPRECATED) for  8 GPIO I2C version or
-    - "mcp,mcp23017" (DEPRECATED) for 16 GPIO I2C version of the chip
-
-    - "microchip,mcp23s08" for  8 GPIO SPI version
-    - "microchip,mcp23s17" for 16 GPIO SPI version
-    - "microchip,mcp23s18" for 16 GPIO SPI version
-    - "microchip,mcp23008" for  8 GPIO I2C version or
-    - "microchip,mcp23017" for 16 GPIO I2C version of the chip
-    - "microchip,mcp23018" for 16 GPIO I2C version
-    NOTE: Do not use the old mcp prefix any more. It is deprecated and will be
-    removed.
-- #gpio-cells : Should be two.
-  - first cell is the pin number
-  - second cell is used to specify flags as described in
-    'Documentation/devicetree/bindings/gpio/gpio.txt'. Allowed values defined by
-    'include/dt-bindings/gpio/gpio.h' (e.g. GPIO_ACTIVE_LOW).
-- gpio-controller : Marks the device node as a GPIO controller.
-- reg : For an address on its bus. I2C uses this a the I2C address of the chip.
-        SPI uses this to specify the chipselect line which the chip is
-        connected to. The driver and the SPI variant of the chip support
-        multiple chips on the same chipselect. Have a look at
-        microchip,spi-present-mask below.
-
-Required device specific properties (only for SPI chips):
-- mcp,spi-present-mask (DEPRECATED)
-- microchip,spi-present-mask : This is a present flag, that makes only sense for SPI
-        chips - as the name suggests. Multiple SPI chips can share the same
-        SPI chipselect. Set a bit in bit0-7 in this mask to 1 if there is a
-        chip connected with the corresponding spi address set. For example if
-        you have a chip with address 3 connected, you have to set bit3 to 1,
-        which is 0x08. mcp23s08 chip variant only supports bits 0-3. It is not
-        possible to mix mcp23s08 and mcp23s17 on the same chipselect. Set at
-        least one bit to 1 for SPI chips.
-    NOTE: Do not use the old mcp prefix any more. It is deprecated and will be
-    removed.
-- spi-max-frequency = The maximum frequency this chip is able to handle
-
-Optional properties:
-- #interrupt-cells : Should be two.
-  - first cell is the pin number
-  - second cell is used to specify flags.
-- interrupt-controller: Marks the device node as a interrupt controller.
-- drive-open-drain: Sets the ODR flag in the IOCON register. This configures
-        the IRQ output as open drain active low.
-- reset-gpios: Corresponds to the active-low RESET# pin for the chip
-
-Optional device specific properties:
-- microchip,irq-mirror: Sets the mirror flag in the IOCON register. Devices
-        with two interrupt outputs (these are the devices ending with 17 and
-        those that have 16 IOs) have two IO banks: IO 0-7 form bank 1 and
-        IO 8-15 are bank 2. These chips have two different interrupt outputs:
-        One for bank 1 and another for bank 2. If irq-mirror is set, both
-        interrupts are generated regardless of the bank that an input change
-        occurred on. If it is not set, the interrupt are only generated for the
-        bank they belong to.
-        On devices with only one interrupt output this property is useless.
-- microchip,irq-active-high: Sets the INTPOL flag in the IOCON register. This
-        configures the IRQ output polarity as active high.
-
-Example I2C (with interrupt):
-gpiom1: gpio@20 {
-        compatible = "microchip,mcp23017";
-        gpio-controller;
-        #gpio-cells = <2>;
-        reg = <0x20>;
-
-        interrupt-parent = <&gpio1>;
-        interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
-        interrupt-controller;
-        #interrupt-cells=<2>;
-        microchip,irq-mirror;
-};
-
-Example SPI:
-gpiom1: gpio@0 {
-        compatible = "microchip,mcp23s17";
-        gpio-controller;
-        #gpio-cells = <2>;
-        microchip,spi-present-mask = <0x01>;
-        reg = <0>;
-        spi-max-frequency = <1000000>;
-};
-
-Pull-up configuration
-=====================
-
-If pins are used as output, they can also be configured with pull-ups. This is
-done with pinctrl.
-
-Please refer file <devicetree/bindings/pinctrl/pinctrl-bindings.txt>
-for details of the common pinctrl bindings used by client devices,
-including the meaning of the phrase "pin configuration node".
-
-Optional Pinmux properties:
---------------------------
-Following properties are required if default setting of pins are required
-at boot.
-- pinctrl-names: A pinctrl state named per <pinctrl-bindings.txt>.
-- pinctrl[0...n]: Properties to contain the phandle for pinctrl states per
-		<pinctrl-bindings.txt>.
-
-The pin configurations are defined as child of the pinctrl states node. Each
-sub-node have following properties:
-
-Required properties:
-------------------
-- pins: List of pins. Valid values of pins properties are:
-		      gpio0 ... gpio7 for the devices with 8 GPIO pins and
-		      gpio0 ... gpio15 for the devices with 16 GPIO pins.
-
-Optional properties:
--------------------
-The following optional property is defined in the pinmux DT binding document
-<pinctrl-bindings.txt>. Absence of this property will leave the configuration
-in its default state.
-	bias-pull-up
-
-Example with pinctrl to pull-up output pins:
-gpio21: gpio@21 {
-	compatible = "microchip,mcp23017";
-	gpio-controller;
-	#gpio-cells = <0x2>;
-	reg = <0x21>;
-	interrupt-parent = <&socgpio>;
-	interrupts = <0x17 0x8>;
-	interrupt-names = "mcp23017@21 irq";
-	interrupt-controller;
-	#interrupt-cells = <0x2>;
-	microchip,irq-mirror;
-	pinctrl-names = "default";
-	pinctrl-0 = <&i2cgpio0irq>, <&gpio21pullups>;
-	reset-gpios = <&gpio6 15 GPIO_ACTIVE_LOW>;
-
-	gpio21pullups: pinmux {
-		pins =	"gpio0", "gpio1", "gpio2", "gpio3",
-			"gpio4", "gpio5", "gpio6", "gpio7",
-			"gpio8", "gpio9", "gpio10", "gpio11",
-			"gpio12", "gpio13", "gpio14", "gpio15";
-		bias-pull-up;
-	};
-};
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml
new file mode 100644
index 000000000000..3904b8adba44
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml
@@ -0,0 +1,153 @@
+# SPDX-License-Identifier: GPL-2.0-only
+# Copyright 2024 Silicon Signals Pvt. Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/pinctrl-mcp23s08.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip I/O expander with serial interface (I2C/SPI)
+
+maintainers:
+  - Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
+
+description: |
+  Microchip MCP23008, MCP23017, MCP23S08, MCP23S17, MCP23S18 GPIO expander
+  chips.These chips provide 8 or 16 GPIO pins with either I2C or SPI interface.
+
+properties:
+  compatible:
+    enum:
+      - microchip,mcp23s08
+      - microchip,mcp23s17
+      - microchip,mcp23s18
+      - microchip,mcp23008
+      - microchip,mcp23017
+      - microchip,mcp23018
+
+  reg:
+    maxItems: 1
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+  interrupt-controller: true
+
+  '#interrupt-cells':
+    const: 2
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    description: GPIO specifier for active-low reset pin.
+    maxItems: 1
+
+  spi-max-frequency:
+    description: Maximum frequency for SPI devices.
+
+  microchip,spi-present-mask:
+    description: |
+      SPI present mask. Specifies which chips are present on the shared SPI
+      chipselect. Each bit in the mask represents one SPI address.
+    $ref: /schemas/types.yaml#/definitions/uint8
+
+  microchip,irq-mirror:
+    type: boolean
+    description: |
+      Sets the mirror flag in the IOCON register. Devices with two interrupt
+      outputs (these are the devices ending with 17 and those that have 16 IOs)
+      have two IO banks IO 0-7 form bank 1 and IO 8-15 are bank 2. These chips
+      have two different interrupt outputs One for bank 1 and another for
+      bank 2. If irq-mirror is set, both interrupts are generated regardless of
+      the bank that an input change occurred on. If it is not set,the interrupt
+      are only generated for the bank they belong to.
+
+  microchip,irq-active-high:
+    type: boolean
+    description: |
+      Sets the INTPOL flag in the IOCON register.This configures the IRQ output
+      polarity as active high.
+
+  drive-open-drain:
+    type: boolean
+    description: |
+      Sets the ODR flag in the IOCON register. This configures the IRQ output as
+      open drain active low.
+
+  pinmux:
+    type: object
+    properties:
+      pins:  
+        description: |
+          The list of GPIO pins controlled by this node. Each pin name corresponds
+          to a physical pin on the GPIO expander.
+        items:
+          pattern: "^gpio([0-9]|[1][0-5])$"
+        maxItems: 16
+
+      bias-pull-up:
+        type: boolean
+        description: Configures pull-up resistors for the GPIO pins.
+
+    required:
+      - pins
+      - bias-pull-up
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - '#gpio-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        
+        mcp23017: gpio@20 {
+            compatible = "microchip,mcp23017";
+            reg = <0x20>;
+            gpio-controller;
+            #gpio-cells = <2>;
+
+            interrupt-parent = <&gpio1>;
+            interrupts = <17 IRQ_TYPE_LEVEL_LOW>; // Check this line
+            interrupt-controller;
+            #interrupt-cells = <2>;
+
+            microchip,irq-mirror;
+            pinctrl-names = "default";
+            pinctrl-0 = <&pinctrl_i2c_gpio0>, <&gpio21pullups>;
+            reset-gpios = <&gpio6 15 GPIO_ACTIVE_LOW>;
+            
+            gpio21pullups: pinmux {
+                pins = "gpio0", "gpio1", "gpio2", "gpio3",
+                       "gpio4", "gpio5", "gpio6", "gpio7",
+                       "gpio8", "gpio9", "gpio10", "gpio11",
+                       "gpio12", "gpio13", "gpio14", "gpio15";
+                bias-pull-up;
+            };
+        };
+    };
+
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        
+        mcp23s17: gpio@0 {
+            compatible = "microchip,mcp23s17";
+            reg = <0>;
+            gpio-controller;
+            #gpio-cells = <2>;
+            spi-max-frequency = <1000000>;
+            microchip,spi-present-mask = /bits/ 8 <0x01>;
+        };
+    };
-- 
2.43.0

Re: [PATCH] dt-bindings: pinctrl: convert pinctrl-mcp23s08.txt to yaml format

[Krzysztof Kozlowski]

On 22/10/2024 08:01, Himanshu Bhavani wrote:
> Convert the text bindings of pinctrl-mcp23s08 to YAML so it could be used to
> validate the DTS.
> 

You silently dropped several compatibles. Document clearly what and why
you changed from original binding during conversion.

> Signed-off-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> ---
>  .../bindings/pinctrl/pinctrl-mcp23s08.txt     | 148 -----------------
>  .../bindings/pinctrl/pinctrl-mcp23s08.yaml    | 153 ++++++++++++++++++

Filename based on compatible, so microchip,mcp23s08.yaml.


>  2 files changed, 153 insertions(+), 148 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml

...

> -};
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml
> new file mode 100644
> index 000000000000..3904b8adba44
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml
> @@ -0,0 +1,153 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +# Copyright 2024 Silicon Signals Pvt. Ltd.

I don't see how Silicon signals contributed to original binding in
a157789b78f4e95f5d66f8b564356e396716f67e and I feel above suggests it is
a new work, not derivative. And if you claim this is not derivative
work, then why not licensed as checkpatch asks? IOW, I suggest dropping
copyright statement.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/pinctrl-mcp23s08.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip I/O expander with serial interface (I2C/SPI)
> +
> +maintainers:
> +  - Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> +
> +description: |

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

> +  Microchip MCP23008, MCP23017, MCP23S08, MCP23S17, MCP23S18 GPIO expander
> +  chips.These chips provide 8 or 16 GPIO pins with either I2C or SPI interface.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,mcp23s08
> +      - microchip,mcp23s17
> +      - microchip,mcp23s18
> +      - microchip,mcp23008
> +      - microchip,mcp23017
> +      - microchip,mcp23018
> +
> +  reg:
> +    maxItems: 1
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description: GPIO specifier for active-low reset pin.
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    description: Maximum frequency for SPI devices.

Drop, not needed. Is this a device on SPI bus? Then you miss ref to
spi-peripheral-props.


> +
> +  microchip,spi-present-mask:
> +    description: |

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

> +      SPI present mask. Specifies which chips are present on the shared SPI
> +      chipselect. Each bit in the mask represents one SPI address.
> +    $ref: /schemas/types.yaml#/definitions/uint8

Where is the entire description from old binding?

> +
> +  microchip,irq-mirror:
> +    type: boolean
> +    description: |
> +      Sets the mirror flag in the IOCON register. Devices with two interrupt
> +      outputs (these are the devices ending with 17 and those that have 16 IOs)
> +      have two IO banks IO 0-7 form bank 1 and IO 8-15 are bank 2. These chips
> +      have two different interrupt outputs One for bank 1 and another for
> +      bank 2. If irq-mirror is set, both interrupts are generated regardless of
> +      the bank that an input change occurred on. If it is not set,the interrupt
> +      are only generated for the bank they belong to.
> +
> +  microchip,irq-active-high:
> +    type: boolean
> +    description: |
> +      Sets the INTPOL flag in the IOCON register.This configures the IRQ output
> +      polarity as active high.
> +
> +  drive-open-drain:
> +    type: boolean
> +    description: |
> +      Sets the ODR flag in the IOCON register. This configures the IRQ output as
> +      open drain active low.
> +
> +  pinmux:
> +    type: object
> +    properties:
> +      pins:  
> +        description: |
> +          The list of GPIO pins controlled by this node. Each pin name corresponds
> +          to a physical pin on the GPIO expander.
> +        items:
> +          pattern: "^gpio([0-9]|[1][0-5])$"
> +        maxItems: 16
> +
> +      bias-pull-up:
> +        type: boolean
> +        description: Configures pull-up resistors for the GPIO pins.
> +
> +    required:
> +      - pins
> +      - bias-pull-up

This does not make much sense, why pull up is always required?

> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - '#gpio-cells'
> +
> +additionalProperties: false



Best regards,
Krzysztof

Re: [PATCH] dt-bindings: pinctrl: convert pinctrl-mcp23s08.txt to yaml format

[Rob Herring (Arm)]

On Tue, 22 Oct 2024 11:31:27 +0530, Himanshu Bhavani wrote:
> Convert the text bindings of pinctrl-mcp23s08 to YAML so it could be used to
> validate the DTS.
> 
> Signed-off-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> ---
>  .../bindings/pinctrl/pinctrl-mcp23s08.txt     | 148 -----------------
>  .../bindings/pinctrl/pinctrl-mcp23s08.yaml    | 153 ++++++++++++++++++
>  2 files changed, 153 insertions(+), 148 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.example.dts:85.3-86.1 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:129: Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1442: dt_binding_check] Error 2
make: *** [Makefile:224: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241022060157.36372-1-himanshu.bhavani@siliconsignals.io

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH] dt-bindings: pinctrl: convert pinctrl-mcp23s08.txt to yaml format

[Krzysztof Kozlowski]

On 22/10/2024 08:01, Himanshu Bhavani wrote:
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        
> +        mcp23017: gpio@20 {
> +            compatible = "microchip,mcp23017";
> +            reg = <0x20>;
> +            gpio-controller;
> +            #gpio-cells = <2>;
> +
> +            interrupt-parent = <&gpio1>;
> +            interrupts = <17 IRQ_TYPE_LEVEL_LOW>; // Check this line

Hm? Left-over?

BTW, did you test this before sending?

Best regards,
Krzysztof

Re: [PATCH] dt-bindings: pinctrl: convert pinctrl-mcp23s08.txt to yaml format

[Himanshu Bhavani]

Hi Krzysztof,

>Hm? Left-over?

>BTW, did you test this before sending?

Yes, I have tested. Below are the logs for the reference.
But I guess I have to upgrade dtschema

SCHEMA  Documentation/devicetree/bindings/processed-schema.json
/data/opensourcenxp/tarang/mainline_kernel/Documentation/devicetree/bindings/net/snps,dwmac.yaml: mac-mode: missing type definition
  CHKDT   ../Documentation/devicetree/bindings
  LINT    ../Documentation/devicetree/bindings
  DTEX    Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.example.dts
  DTC [C] Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.example.dtb

Best Regards,
Himanshu
________________________________________
From: Krzysztof Kozlowski <krzk@kernel.org>
Sent: 22 October 2024 12:58
To: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>; linus.walleij@linaro.org <linus.walleij@linaro.org>; robh@kernel.org <robh@kernel.org>; krzk+dt@kernel.org <krzk+dt@kernel.org>; conor+dt@kernel.org <conor+dt@kernel.org>
Cc: linux-gpio@vger.kernel.org <linux-gpio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] dt-bindings: pinctrl: convert pinctrl-mcp23s08.txt to yaml format
 
CAUTION: This email originated from outside the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

On 22/10/2024 08:01, Himanshu Bhavani wrote:
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        mcp23017: gpio@20 {
> +            compatible = "microchip,mcp23017";
> +            reg = <0x20>;
> +            gpio-controller;
> +            #gpio-cells = <2>;
> +
> +            interrupt-parent = <&gpio1>;
> +            interrupts = <17 IRQ_TYPE_LEVEL_LOW>; // Check this line

Hm? Left-over?

BTW, did you test this before sending?

Best regards,
Krzysztof

Re: [PATCH] dt-bindings: pinctrl: convert pinctrl-mcp23s08.txt to yaml format

[Himanshu Bhavani]

Hi Krzysztof,

>> Convert the text bindings of pinctrl-mcp23s08 to YAML so it could be used to
>> validate the DTS.
>>
>
>You silently dropped several compatibles. Document clearly what and why
>you changed from original binding during conversion.

Sure, will do it 

>> Signed-off-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
>> ---
>>  .../bindings/pinctrl/pinctrl-mcp23s08.txt     | 148 -----------------
>>  .../bindings/pinctrl/pinctrl-mcp23s08.yaml    | 153 ++++++++++++++++++
>
>Filename based on compatible, so microchip,mcp23s08.yaml.

ok I will change.

>>  2 files changed, 153 insertions(+), 148 deletions(-)
>>  delete mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
>>  create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml

...

>> -};
>> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml
>> new file mode 100644
>> index 000000000000..3904b8adba44
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml
>> @@ -0,0 +1,153 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +# Copyright 2024 Silicon Signals Pvt. Ltd.
>
>I don't see how Silicon signals contributed to original binding in
>a157789b78f4e95f5d66f8b564356e396716f67e and I feel above suggests it is
>a new work, not derivative. And if you claim this is not derivative
>work, then why not licensed as checkpatch asks? IOW, I suggest dropping
>copyright statement.

sure, I will drop it.

>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pinctrl/pinctrl-mcp23s08.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Microchip I/O expander with serial interface (I2C/SPI)
>> +
>> +maintainers:
>> +  - Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
>> +
>> +description: |
>
>Do not need '|' unless you need to preserve formatting.

okay.

>> +  Microchip MCP23008, MCP23017, MCP23S08, MCP23S17, MCP23S18 GPIO expander
>> +  chips.These chips provide 8 or 16 GPIO pins with either I2C or SPI interface.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - microchip,mcp23s08
>> +      - microchip,mcp23s17
>> +      - microchip,mcp23s18
>> +      - microchip,mcp23008
>> +      - microchip,mcp23017
>> +      - microchip,mcp23018
>> +
>> +  reg:
>> +    maxItems: 1
> >+
> >+  gpio-controller: true
> >+
> >+  '#gpio-cells':
> >+    const: 2
> >+
> >+  interrupt-controller: true
> >+
> >+  '#interrupt-cells':
> >+    const: 2
> >+
> >+  interrupts:
> >+    maxItems: 1
> >+
> >+  reset-gpios:
> >+    description: GPIO specifier for active-low reset pin.
> >+    maxItems: 1
> >+
> >+  spi-max-frequency:
> >+    description: Maximum frequency for SPI devices.
>
>Drop, not needed. Is this a device on SPI bus? Then you miss ref to
>spi-peripheral-props.

yes device is on SPI bus. I will add reference

>> +
>> +  microchip,spi-present-mask:
>> +    description: |
>
>Do not need '|' unless you need to preserve formatting.

sure,

>> +      SPI present mask. Specifies which chips are present on the shared SPI
>> +      chipselect. Each bit in the mask represents one SPI address.
>> +    $ref: /schemas/types.yaml#/definitions/uint8
>
>Where is the entire description from old binding?

Okay I will add whole description

>> +
>> +  microchip,irq-mirror:
>> +    type: boolean
>> +    description: |
> >+      Sets the mirror flag in the IOCON register. Devices with two interrupt
> >+      outputs (these are the devices ending with 17 and those that have 16 IOs)
> >+      have two IO banks IO 0-7 form bank 1 and IO 8-15 are bank 2. These chips
> >+      have two different interrupt outputs One for bank 1 and another for
>> +      bank 2. If irq-mirror is set, both interrupts are generated regardless of
>> +      the bank that an input change occurred on. If it is not set,the interrupt
>> +      are only generated for the bank they belong to.
> >+
> >+  microchip,irq-active-high:
> >+    type: boolean
> >+    description: |
> >+      Sets the INTPOL flag in the IOCON register.This configures the IRQ output
> >+      polarity as active high.
> >+
> >+  drive-open-drain:
> >+    type: boolean
> >+    description: |
> >+      Sets the ODR flag in the IOCON register. This configures the IRQ output as
> >+      open drain active low.
> >+
> >+  pinmux:
> >+    type: object
> >+    properties:
> >+      pins:
> >+        description: |
> >+          The list of GPIO pins controlled by this node. Each pin name corresponds
> >+          to a physical pin on the GPIO expander.
> >+        items:
> >+          pattern: "^gpio([0-9]|[1][0-5])$"
> >+        maxItems: 16
> >+
> >+      bias-pull-up:
> >+        type: boolean
> >+        description: Configures pull-up resistors for the GPIO pins.
> >+
> >+    required:
> >+      - pins
> >+      - bias-pull-up
>
>This does not make much sense, why pull up is always required?

Not always but you can configure as pull-up.

If you suggest then I will give two different example in i2c, with or without pull-up as old bindings had.

and in pinmux I will add description for pull-up .


Best regards,
Himanshu

> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - '#gpio-cells'
> +
> +additionalProperties: false

________________________________________
From: Krzysztof Kozlowski <krzk@kernel.org>
Sent: 22 October 2024 12:03
To: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>; linus.walleij@linaro.org <linus.walleij@linaro.org>; robh@kernel.org <robh@kernel.org>; krzk+dt@kernel.org <krzk+dt@kernel.org>; conor+dt@kernel.org <conor+dt@kernel.org>
Cc: linux-gpio@vger.kernel.org <linux-gpio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] dt-bindings: pinctrl: convert pinctrl-mcp23s08.txt to yaml format
 
CAUTION: This email originated from outside the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

On 22/10/2024 08:01, Himanshu Bhavani wrote:
> Convert the text bindings of pinctrl-mcp23s08 to YAML so it could be used to
> validate the DTS.
>

You silently dropped several compatibles. Document clearly what and why
you changed from original binding during conversion.

> Signed-off-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> ---
>  .../bindings/pinctrl/pinctrl-mcp23s08.txt     | 148 -----------------
>  .../bindings/pinctrl/pinctrl-mcp23s08.yaml    | 153 ++++++++++++++++++

Filename based on compatible, so microchip,mcp23s08.yaml.


>  2 files changed, 153 insertions(+), 148 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml

...

> -};
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml
> new file mode 100644
> index 000000000000..3904b8adba44
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml
> @@ -0,0 +1,153 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +# Copyright 2024 Silicon Signals Pvt. Ltd.

I don't see how Silicon signals contributed to original binding in
a157789b78f4e95f5d66f8b564356e396716f67e and I feel above suggests it is
a new work, not derivative. And if you claim this is not derivative
work, then why not licensed as checkpatch asks? IOW, I suggest dropping
copyright statement.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/pinctrl-mcp23s08.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip I/O expander with serial interface (I2C/SPI)
> +
> +maintainers:
> +  - Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> +
> +description: |

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

> +  Microchip MCP23008, MCP23017, MCP23S08, MCP23S17, MCP23S18 GPIO expander
> +  chips.These chips provide 8 or 16 GPIO pins with either I2C or SPI interface.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,mcp23s08
> +      - microchip,mcp23s17
> +      - microchip,mcp23s18
> +      - microchip,mcp23008
> +      - microchip,mcp23017
> +      - microchip,mcp23018
> +
> +  reg:
> +    maxItems: 1
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description: GPIO specifier for active-low reset pin.
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    description: Maximum frequency for SPI devices.

Drop, not needed. Is this a device on SPI bus? Then you miss ref to
spi-peripheral-props.


> +
> +  microchip,spi-present-mask:
> +    description: |

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

> +      SPI present mask. Specifies which chips are present on the shared SPI
> +      chipselect. Each bit in the mask represents one SPI address.
> +    $ref: /schemas/types.yaml#/definitions/uint8

Where is the entire description from old binding?

> +
> +  microchip,irq-mirror:
> +    type: boolean
> +    description: |
> +      Sets the mirror flag in the IOCON register. Devices with two interrupt
> +      outputs (these are the devices ending with 17 and those that have 16 IOs)
> +      have two IO banks IO 0-7 form bank 1 and IO 8-15 are bank 2. These chips
> +      have two different interrupt outputs One for bank 1 and another for
> +      bank 2. If irq-mirror is set, both interrupts are generated regardless of
> +      the bank that an input change occurred on. If it is not set,the interrupt
> +      are only generated for the bank they belong to.
> +
> +  microchip,irq-active-high:
> +    type: boolean
> +    description: |
> +      Sets the INTPOL flag in the IOCON register.This configures the IRQ output
> +      polarity as active high.
> +
> +  drive-open-drain:
> +    type: boolean
> +    description: |
> +      Sets the ODR flag in the IOCON register. This configures the IRQ output as
> +      open drain active low.
> +
> +  pinmux:
> +    type: object
> +    properties:
> +      pins:
> +        description: |
> +          The list of GPIO pins controlled by this node. Each pin name corresponds
> +          to a physical pin on the GPIO expander.
> +        items:
> +          pattern: "^gpio([0-9]|[1][0-5])$"
> +        maxItems: 16
> +
> +      bias-pull-up:
> +        type: boolean
> +        description: Configures pull-up resistors for the GPIO pins.
> +
> +    required:
> +      - pins
> +      - bias-pull-up

This does not make much sense, why pull up is always required?

> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - '#gpio-cells'
> +
> +additionalProperties: false



Best regards,
Krzysztof

Re: [PATCH] dt-bindings: pinctrl: convert pinctrl-mcp23s08.txt to yaml format

[Himanshu Bhavani]

Hi Rob,

Command  :  
                 make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml
 
Output : 
                 DTEX    Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.example.dts
                 DTC [C] Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.example.dtb
 
I am not getting any error or warnings 
 
I checked dtschema is up to date 
dtschema Version: 2024.5
yamllint 1.33.0

Regards,
Himanshu
________________________________________
From: Rob Herring (Arm) <robh@kernel.org>
Sent: 22 October 2024 12:55
To: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
Cc: devicetree@vger.kernel.org <devicetree@vger.kernel.org>; krzk+dt@kernel.org <krzk+dt@kernel.org>; linux-gpio@vger.kernel.org <linux-gpio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linus.walleij@linaro.org <linus.walleij@linaro.org>; conor+dt@kernel.org <conor+dt@kernel.org>
Subject: Re: [PATCH] dt-bindings: pinctrl: convert pinctrl-mcp23s08.txt to yaml format
 
CAUTION: This email originated from outside the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

On Tue, 22 Oct 2024 11:31:27 +0530, Himanshu Bhavani wrote:
> Convert the text bindings of pinctrl-mcp23s08 to YAML so it could be used to
> validate the DTS.
>
> Signed-off-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> ---
>  .../bindings/pinctrl/pinctrl-mcp23s08.txt     | 148 -----------------
>  .../bindings/pinctrl/pinctrl-mcp23s08.yaml    | 153 ++++++++++++++++++
>  2 files changed, 153 insertions(+), 148 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml
>

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.example.dts:85.3-86.1 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:129: Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1442: dt_binding_check] Error 2
make: *** [Makefile:224: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241022060157.36372-1-himanshu.bhavani@siliconsignals.io

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH] dt-bindings: pinctrl: convert pinctrl-mcp23s08.txt to yaml format

[Rob Herring]

On Tue, Oct 22, 2024 at 09:22:52AM +0000, Himanshu Bhavani wrote:
> Hi Rob,
> 
> Command  :  
>                  make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.yaml
>  
> Output : 
>                  DTEX    Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.example.dts
>                  DTC [C] Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.example.dtb
>  
> I am not getting any error or warnings 

Try applying the patch you sent. The applied patch truncates the file. 
It looks to me like the line count in the diff chunk is wrong. It says 
153 which is the length of the truncated file.

>  
> I checked dtschema is up to date 
> dtschema Version: 2024.5

Not the latest version, but that doesn't matter here.

> yamllint 1.33.0
> 
> Regards,
> Himanshu