[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b69251f7-7827-4f0e-b4fe-184a5cb54ee7@kernel.org>
Date: Tue, 22 Oct 2024 08:33:28 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Himanshu Bhavani <himanshu.bhavani@...iconsignals.io>,
linus.walleij@...aro.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org
Cc: linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: pinctrl: convert pinctrl-mcp23s08.txt to
yaml format
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@...iconsignals.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@...iconsignals.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
Powered by blists - more mailing lists