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

[Himanshu Bhavani <himanshu.bhavani@xxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxx>
>> ---
>>  .../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@xxxxxxxxxxxxxxxxx>
>> +
>> +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@xxxxxxxxxx>
Sent: 22 October 2024 12:03
To: Himanshu Bhavani <himanshu.bhavani@xxxxxxxxxxxxxxxxx>; linus.walleij@xxxxxxxxxx <linus.walleij@xxxxxxxxxx>; robh@xxxxxxxxxx <robh@xxxxxxxxxx>; krzk+dt@xxxxxxxxxx <krzk+dt@xxxxxxxxxx>; conor+dt@xxxxxxxxxx <conor+dt@xxxxxxxxxx>
Cc: linux-gpio@xxxxxxxxxxxxxxx <linux-gpio@xxxxxxxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx <devicetree@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxx>
> ---
>  .../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@xxxxxxxxxxxxxxxxx>
> +
> +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