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

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

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