| Message ID | 20241126-z2-v1-1-c43c4cc6200d@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Driver for Apple Z2 touchscreens. | expand |
On Tue, Nov 26, 2024 at 09:47:59PM +0100, Sasha Finkelstein wrote: > +properties: > + compatible: > + items: > + - enum: > + - apple,j293-touchbar > + - apple,j493-touchbar > + - const: apple,z2-touchbar > + - const: apple,z2-multitouch What is the meaning of these two last compatibles in the list? What are these devices? > + > + interrupts: > + maxItems: 1 > + > + reset-gpios: > + maxItems: 1 > + > + cs-gpios: > + maxItems: 1 > + description: | Do not need '|' unless you need to preserve formatting. > + J293 has a hardware quirk where the CS line is unusable and has > + to the be driven by a GPIO pin instead > + > + firmware-name: > + maxItems: 1 > + > + label: > + maxItems: 1 Why is this needed? I think it is not part of common touchscreen schema. Drop, devices do not need labels - node name and unit address identify it. If this is needed for something else, then come with generic property matching all touchscreens. > + > + touchscreen-size-x: true > + touchscreen-size-y: true Drop these two > + > +required: > + - compatible > + - interrupts > + - reset-gpios > + - firmware-name > + - label > + - touchscreen-size-x > + - touchscreen-size-y > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + touchscreen@0 { > + compatible = "apple,j293-touchbar", "apple,z2-touchbar", > + "apple,z2-multitouch"; > + reg = <0>; > + spi-max-frequency = <11500000>; > + reset-gpios = <&pinctrl_ap 139 0>; > + cs-gpios = <&pinctrl_ap 109 0>; Use proper GPIO bindings constants. You included header for this, I guess. > + interrupts-extended = <&pinctrl_ap 194 IRQ_TYPE_EDGE_FALLING>; > + firmware-name = "apple/dfrmtfw-j293.bin"; > + touchscreen-size-x = <23045>; > + touchscreen-size-y = <640>; > + label = "MacBookPro17,1 Touch Bar"; > + }; > + }; > + > +... > > -- > 2.47.1 >
On 27/11/2024 09:46, Krzysztof Kozlowski wrote: > On Tue, Nov 26, 2024 at 09:47:59PM +0100, Sasha Finkelstein wrote: >> +properties: >> + compatible: >> + items: >> + - enum: >> + - apple,j293-touchbar >> + - apple,j493-touchbar >> + - const: apple,z2-touchbar >> + - const: apple,z2-multitouch > > What is the meaning of these two last compatibles in the list? What are > these devices? Previous Rob's comment apply here as well. If z2 is protocol, then multitouch and touchbar do not feel appropriate, unless these are some subsets of the protocol. But as in other cases no one knows here what's there inside, so avoid making generic compatibles. Just apple,j293-touchbar and 493+293. That's the recommendation we keep insisting on almost always. As Rob explained: protocol does not matter in terms of compatible. We do not have devices like "analog,j293-spi" (and there is clear NAK when people post them, with one or two exceptions). > >> + >> + interrupts: >> + maxItems: 1 >> + >> + reset-gpios: >> + maxItems: 1 >> + >> + cs-gpios: >> + maxItems: 1 >> + description: | > > Do not need '|' unless you need to preserve formatting. > >> + J293 has a hardware quirk where the CS line is unusable and has >> + to the be driven by a GPIO pin instead >> + >> + firmware-name: >> + maxItems: 1 >> + >> + label: >> + maxItems: 1 > > Why is this needed? I think it is not part of common touchscreen schema. > Drop, devices do not need labels - node name and unit address identify > it. If this is needed for something else, then come with generic > property matching all touchscreens. This is v1 so I did not expect previous talks, but now I dig them out and there was conclusion: your compatible defines label. You do not have two of same devices in the DTS to justify it. Drop the property. Best regards, Krzysztof
On Wed, 27 Nov 2024 at 09:47, Krzysztof Kozlowski <krzk@kernel.org> wrote: > What is the meaning of these two last compatibles in the list? What are > these devices? Those are generic compatibles for everything that speaks the Z2 protocol multitouch is used for everything, as this is currently enough for the driver, while touchbar is for userspace, as touchscreens and touchbars need different handling. This specific schema was suggested on the previous version. > > + label: > > + maxItems: 1 > > Why is this needed? I think it is not part of common touchscreen schema. > Drop, devices do not need labels - node name and unit address identify > it. If this is needed for something else, then come with generic > property matching all touchscreens. I want some sort of a property to contain a human readable (ish) name of this device.
On Wed, 27 Nov 2024 at 11:49, Sasha Finkelstein <fnkl.kernel@gmail.com> wrote: > > Why is this needed? I think it is not part of common touchscreen schema. > > Drop, devices do not need labels - node name and unit address identify > > it. If this is needed for something else, then come with generic > > property matching all touchscreens. > > I want some sort of a property to contain a human readable (ish) > name of this device. Actually, nvm, i think i understand it now, you want the labels stored in the driver and set based on device compatible, right?
On 27/11/2024 12:23, Sasha Finkelstein wrote: > On Wed, 27 Nov 2024 at 11:49, Sasha Finkelstein <fnkl.kernel@gmail.com> wrote: > >>> Why is this needed? I think it is not part of common touchscreen schema. >>> Drop, devices do not need labels - node name and unit address identify >>> it. If this is needed for something else, then come with generic >>> property matching all touchscreens. >> >> I want some sort of a property to contain a human readable (ish) >> name of this device. > > Actually, nvm, i think i understand it now, you want the labels stored > in the driver and set based on device compatible, right? Yes, I think this was also suggested last time. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/input/touchscreen/apple,z2-multitouch.yaml b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-multitouch.yaml new file mode 100644 index 0000000000000000000000000000000000000000..0387e2178b91d5658dfd60cb44099a8048dc97df --- /dev/null +++ b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-multitouch.yaml @@ -0,0 +1,83 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/input/touchscreen/apple,z2-multitouch.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Apple touchscreens attached using the Z2 protocol + +maintainers: + - 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: + items: + - enum: + - apple,j293-touchbar + - apple,j493-touchbar + - const: apple,z2-touchbar + - const: apple,z2-multitouch + + interrupts: + maxItems: 1 + + reset-gpios: + maxItems: 1 + + cs-gpios: + maxItems: 1 + description: | + J293 has a hardware quirk where the CS line is unusable and has + to the be driven by a GPIO pin instead + + firmware-name: + maxItems: 1 + + label: + maxItems: 1 + + touchscreen-size-x: true + touchscreen-size-y: true + +required: + - compatible + - interrupts + - reset-gpios + - firmware-name + - label + - touchscreen-size-x + - touchscreen-size-y + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + + spi { + #address-cells = <1>; + #size-cells = <0>; + + touchscreen@0 { + compatible = "apple,j293-touchbar", "apple,z2-touchbar", + "apple,z2-multitouch"; + 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>; + label = "MacBookPro17,1 Touch Bar"; + }; + }; + +...