Date:   Mon, 27 Feb 2023 13:51:39 -0600
From:   Rob Herring <robh@kernel.org>
To:     Sasha Finkelstein <fnkl.kernel@gmail.com>
Cc:     Hector Martin <marcan@marcan.st>, Sven Peter <sven@svenpeter.dev>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        - <asahi@lists.linux.dev>, Henrik Rydberg <rydberg@bitmath.org>,
        linux-arm-kernel@lists.infradead.org, linux-input@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2
 controller bindings.
Message-ID: <20230227195139.GA677578-robh@kernel.org>
References: <20230223-z2-for-ml-v1-0-028f2b85dc15@gmail.com>
 <20230223-z2-for-ml-v1-1-028f2b85dc15@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230223-z2-for-ml-v1-1-028f2b85dc15@gmail.com>
Precedence: bulk

On Fri, Feb 24, 2023 at 11:20:06AM +0100, Sasha Finkelstein wrote:
> Add bindings for touchscreen controllers attached using the Z2 protocol.
> Those are present in most Apple devices.
> 
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
> ---
>  .../input/touchscreen/apple,z2-touchscreen.yaml    | 81 ++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> new file mode 100644
> index 000000000000..695594494b1e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/apple,z2-touchscreen.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple touchscreens attached using the Z2 protocol.
> +
> +maintainers:
> +  - asahi@lists.linux.dev
> +  - Sasha Finkelstein <fnkl.kernel@gmail.com>
> +
> +description: A series of touschscreen controllers used in Apple products.
> +
> +allOf:
> +  - $ref: touchscreen.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: apple,z2-touchscreen

Is 'z2' anything other than a touchscreen? If not, '-touchscreen' is 
redundant. If so, then what else is there? You should be describing 
physical devices, not just a protocol for touchscreen.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts-extended:

Just 'interrupts' here. 'interrupts-extended' is implicitly supported.

> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  cs-gpios:

There is a standard way to do GPIO based chip-selects. It happens to be 
'cs-gpios', but this is in the wrong place. It goes in the SPI 
controller node.

> +    maxItems: 1
> +
> +  firmware-name:
> +    maxItems: 1
> +
> +  apple,z2-device-name:
> +    description: The name to be used for the input device
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  touchscreen-size-x: true
> +  touchscreen-size-y: true
> +  spi-max-frequency: true
> +
> +required:
> +  - compatible
> +  - interrupts-extended
> +  - reset-gpios
> +  - cs-gpios
> +  - firmware-name
> +  - apple,z2-device-name
> +  - touchscreen-size-x
> +  - touchscreen-size-y
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    spi {
> +            #address-cells = <1>;
> +            #size-cells = <0>;

4 space indentation is preferred here.

> +
> +            touchscreen@0 {
> +                    compatible = "apple,z2-touchscreen";
> +                    reg = <0>;
> +                    spi-max-frequency = <11500000>;
> +                    reset-gpios = <&pinctrl_ap 139 0>;
> +                    cs-gpios = <&pinctrl_ap 109 0>;
> +                    interrupts-extended = <&pinctrl_ap 194 IRQ_TYPE_EDGE_FALLING>;
> +                    firmware-name = "apple/dfrmtfw-j293.bin";
> +                    touchscreen-size-x = <23045>;
> +                    touchscreen-size-y = <640>;
> +                    apple,z2-device-name = "MacBookPro17,1 Touch Bar";

Why do we need this string? If you want a human consumed label for 
some identification, we have a property for that purpose. It's called 
'label'. But when there is only 1 instance, I don't really see the 
point.

Rob