| Message ID | 20241126-adpdrm-v2-1-c90485336c09@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Driver for pre-DCP apple display controller. | expand |
On 26/11/2024 17:34, Sasha Finkelstein via B4 Relay wrote: > From: Sasha Finkelstein <fnkl.kernel@gmail.com> > > Add bindings for a secondary display controller present on certain > Apple laptops. > A nit, subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com> > --- > .../display/apple,h7-display-pipe-mipi.yaml | 89 ++++++++++++++++++++++ > .../bindings/display/apple,h7-display-pipe.yaml | 77 +++++++++++++++++++ > .../bindings/display/panel/apple,summit.yaml | 58 ++++++++++++++ > 3 files changed, 224 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/apple,h7-display-pipe-mipi.yaml b/Documentation/devicetree/bindings/display/apple,h7-display-pipe-mipi.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..2cf2f50e9fc7329a5b424d5ddf8c34cad2ebb6be > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/apple,h7-display-pipe-mipi.yaml > @@ -0,0 +1,89 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/apple,h7-display-pipe-mipi.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Apple pre-DCP display controller MIPI interface. Drop final stop, that's a title so it could be capitalized, but anyway it is not a sentence > + > +maintainers: > + - asahi@lists.linux.dev Drop, maintainer boxes are not allowed for bindings. > + - Sasha Finkelstein <fnkl.kernel@gmail.com> > + > +description: > + The MIPI controller part of the pre-DCP Apple display controller > + > +allOf: > + - $ref: dsi-controller.yaml# > + > +properties: > + compatible: > + items: > + - enum: > + - apple,t8112-display-pipe-mipi > + - apple,t8103-display-pipe-mipi > + - const: apple,h7-display-pipe-mipi > + > + reg: > + maxItems: 1 > + > + reg-names: > + const: mipi > + > + power-domains: > + maxItems: 1 > + > + ports: > + $ref: /schemas/graph.yaml#/properties/ports > + > + properties: > + port@0: > + $ref: /schemas/graph.yaml#/properties/port > + > + port@1: > + $ref: /schemas/graph.yaml#/properties/port > + > + required: > + - port@0 > + - port@1 Please take a look how other bindings define ports. You miss here several items and more important - description what are these ports for. > + > + '#address-cells': true > + > + '#size-cells': true > + > +patternProperties: > + "^panel@[0-3]$": true These look unusual. Is this a DSI controller? If so, then reference dsi-controller. See other bindings how this is done. > + > +required: > + - compatible > + - reg > + - ports > + > +additionalProperties: false > + > +examples: > + - | > + display_dfr: dsi@228200000 { Drop unused label > + compatible = "apple,t8103-display-pipe-mipi", "apple,h7-display-pipe-mipi"; > + reg-names = "mipi"; > + reg = <0x28200000 0xc000>; Order fields according to DTS coding style. > + power-domains = <&ps_dispdfr_mipi>; > + > + ports { > + #address-cells = <1>; Messed indentation. Use 4 spaces for example indentation. > + #size-cells = <0>; > + > + dfr_mipi_in: port@0 { > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0>; > + }; > + > + dfr_mipi_out: port@1 { > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <1>; > + }; > + }; > + }; > +... > diff --git a/Documentation/devicetree/bindings/display/apple,h7-display-pipe.yaml b/Documentation/devicetree/bindings/display/apple,h7-display-pipe.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..98982da9c5f672167d67e4cd3b47e1fbdafc9510 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/apple,h7-display-pipe.yaml > @@ -0,0 +1,77 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/apple,h7-display-pipe.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Apple pre-DCP display controller. > + > +maintainers: > + - asahi@lists.linux.dev > + - Sasha Finkelstein <fnkl.kernel@gmail.com> > + > +description: > + A secondary display controller used to drive the "touchbar" on certain Apple laptops. Please wrap code according to coding style (checkpatch is not a coding style description, but only a tool). > + > +properties: > + compatible: > + items: > + - enum: > + - apple,t8112-display-pipe > + - apple,t8103-display-pipe > + - const: apple,h7-display-pipe > + > + reg: > + maxItems: 2 List and describe the items instead, because be and fe are a bit cryptic. > + > + reg-names: > + items: > + - const: be > + - const: fe > + > + power-domains: > + maxItems: 2 Need to list the items instead. > + > + interrupts: > + maxItems: 2 Ditto > + > + interrupt-names: > + items: > + - const: be > + - const: fe > + > + iommus: > + maxItems: 1 > + > + port: > + $ref: /schemas/graph.yaml#/properties/port > + > +required: > + - compatible > + - reg > + - interrupts > + - port > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/apple-aic.h> > + display_dfr: display-pipe@228200000 { Drop unused label. > + compatible = "apple,t8103-display-pipe", "apple,h7-display-pipe"; > + reg-names = "be", "fe"; > + reg = <0x28200000 0xc000>, > + <0x28400000 0x4000>; Messed alignment, in other places as well. > + power-domains = <&ps_dispdfr_fe>, <&ps_dispdfr_be>; > + interrupt-parent = <&aic>; > + interrupts = <AIC_IRQ 502 IRQ_TYPE_LEVEL_HIGH>, > + <AIC_IRQ 506 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "be", "fe"; > + iommus = <&displaydfr_dart 0>; > + port { > + dfr_adp_out_mipi: endpoint { Messed indentation. > + remote-endpoint = <&dfr_mipi_in_adp>; > + }; > + }; > + }; > +... > diff --git a/Documentation/devicetree/bindings/display/panel/apple,summit.yaml b/Documentation/devicetree/bindings/display/panel/apple,summit.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..db14f7af3787076c84ccdda08fedeb8912d5514d > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/panel/apple,summit.yaml > @@ -0,0 +1,58 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/panel/apple,summit.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Apple "Summit" display panel. > + > +maintainers: > + - asahi@lists.linux.dev > + - Sasha Finkelstein <fnkl.kernel@gmail.com> > + > +allOf: > + - $ref: panel-common.yaml# > + - $ref: /schemas/leds/backlight/common.yaml# > + > +properties: > + compatible: > + items: > + - enum: > + - apple,j293-summit > + - apple,j493-summit > + - const: apple,summit Summit tells me nothing - no description, title repeats it, so I suggest using device specific compatible. > + > + reg: > + maxItems: 1 > + > + max-brightness: true > + > + port: true No, these cannot be true without definition. Again, please open existing bindings and use them as example. You probably miss here some reference, but max-brightness for panel is a bit confusing. I asked already and did not get answer: isn't this backlight property? What is this device - backlight or panel? If panel, then what bus? Best regards, Krzysztof
On 26/11/2024 17:46, Krzysztof Kozlowski wrote: > >> + >> + reg: >> + maxItems: 1 >> + >> + max-brightness: true >> + >> + port: true > > No, these cannot be true without definition. Again, please open existing > bindings and use them as example. You probably miss here some reference, > but max-brightness for panel is a bit confusing. I asked already and did > not get answer: isn't this backlight property? What is this device - > backlight or panel? If panel, then what bus? You responded to my comment in exactly the same moment you sent this patchset which gives me around 0 seconds to reply to your comment. Give reviewers some time if you disagree with them, otherwise I find wrong sending next version immediately. Best regards, Krzysztof
On Tue, 26 Nov 2024 at 17:46, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > +allOf: > > + - $ref: dsi-controller.yaml# ... > > +patternProperties: > > + "^panel@[0-3]$": true > > These look unusual. Is this a DSI controller? If so, then reference > dsi-controller. See other bindings how this is done. This is a DSI controller, as referenced above. Those properties are from dsi-controller.yaml > > +properties: > > + compatible: > > + items: > > + - enum: > > + - apple,j293-summit > > + - apple,j493-summit > > + - const: apple,summit > > Summit tells me nothing - no description, title repeats it, so I suggest > using device specific compatible. The j293/j493 are the device-specific compatibles, those are chassis names for the specific laptops the panel is present in. > No, these cannot be true without definition. Again, please open existing > bindings and use them as example. You probably miss here some reference, > but max-brightness for panel is a bit confusing. I asked already and did > not get answer: isn't this backlight property? What is this device - > backlight or panel? If panel, then what bus? Per my response on previous version, it's both, kind of. This is a self-emissive panel on a mipi-dsi bus, which can change brightness in response to mipi dcs backlight commands.
On 26/11/2024 18:00, Sasha Finkelstein wrote: > On Tue, 26 Nov 2024 at 17:46, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>> +allOf: >>> + - $ref: dsi-controller.yaml# > ... >>> +patternProperties: >>> + "^panel@[0-3]$": true >> >> These look unusual. Is this a DSI controller? If so, then reference >> dsi-controller. See other bindings how this is done. > > This is a DSI controller, as referenced above. Those properties are from > dsi-controller.yaml Ahh, indeed, I missed that and focused on the additionalProperties. Instead use unevaluatedProperties: false and drop all properties already in dsi-controller.yaml. > >>> +properties: >>> + compatible: >>> + items: >>> + - enum: >>> + - apple,j293-summit >>> + - apple,j493-summit >>> + - const: apple,summit >> >> Summit tells me nothing - no description, title repeats it, so I suggest >> using device specific compatible. > > The j293/j493 are the device-specific compatibles, those are chassis names > for the specific laptops the panel is present in. This does not address my comment. Use specific compatibles as fallback, just like we recommend for every device. This should not be different. If you do not know the hardware details, using generic is even less appropriate. > > >> No, these cannot be true without definition. Again, please open existing >> bindings and use them as example. You probably miss here some reference, >> but max-brightness for panel is a bit confusing. I asked already and did >> not get answer: isn't this backlight property? What is this device - >> backlight or panel? If panel, then what bus? > > Per my response on previous version, it's both, kind of. This is a > self-emissive panel on I see, I think I again totally missed that you have there references to backlight, so binding is in general fine. Describe the hardware in description field (see commit for mentioned Samsung panel). Best regards, Krzysztof
On 2024/11/27 2:20, Krzysztof Kozlowski wrote: > On 26/11/2024 18:00, Sasha Finkelstein wrote: >> On Tue, 26 Nov 2024 at 17:46, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>>> +properties: >>>> + compatible: >>>> + items: >>>> + - enum: >>>> + - apple,j293-summit >>>> + - apple,j493-summit >>>> + - const: apple,summit >>> >>> Summit tells me nothing - no description, title repeats it, so I suggest >>> using device specific compatible. >> >> The j293/j493 are the device-specific compatibles, those are chassis names >> for the specific laptops the panel is present in. > > This does not address my comment. Use specific compatibles as fallback, > just like we recommend for every device. This should not be different. > If you do not know the hardware details, using generic is even less > appropriate. The panel is codenamed "summit", and that tells you everything. It's a panel sold and marketed by Apple. It is used on two devices, which are specifically referred to as the device names "j293" and "j493". There is no further information to be added here, the names chosen already contain 100% of the available information and are completely and fully specific as to what devices are involved here. There is no more specific or appropriate compatible possible. "summit" literally comes from Apple's own device tree compatible in the macOS world, which is "lcd,summit". If Apple uses it as a DT compatible, then it's a good bet it is precisely what it needs to be to identify a device. The chassis-specific versions are something we added on top of that and likely aren't even necessary since it's almost certainly precisely the same exact panel in both laptops, but as you know, it's best to be specific with DT compatibles just in case. There is plenty of prior art for compatibles that don't look like random product code gobbledygook (which I think is what you were expecting?), e.g. these panels: ti,nspire-cx-lcd-panel ste,mcde-dsi raspberrypi,7inch-touchscreen-panel olimex,lcd-olinuxino focaltech,gpt3 So yeah, the correct compatible is in fact "apple,summit". Anything else would be making things up for no reason. The vendor has chosen to call this panel "summit", so "summit" it is. We're not in the business of gratuitously assigning our own product names/codes when a suitable one already exists here. - Hector
On 27/11/2024 00:34, Sasha Finkelstein via B4 Relay wrote: [...] > diff --git a/Documentation/devicetree/bindings/display/apple,h7-display-pipe-mipi.yaml b/Documentation/devicetree/bindings/display/apple,h7-display-pipe-mipi.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..2cf2f50e9fc7329a5b424d5ddf8c34cad2ebb6be > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/apple,h7-display-pipe-mipi.yaml [...] > + > + reg: > + maxItems: 1 > + > + reg-names: > + const: mipi > + Drop. Not needed as there is only one reg now. [...] Nick Chan
On Tue, 26 Nov 2024 at 17:46, Krzysztof Kozlowski <krzk@kernel.org> wrote: > Please take a look how other bindings define ports. You miss here > several items and more important - description what are these ports for. Aside from missing descriptions, this definition is copied almost verbatim from snps,dw-mipi-dsi. Ack on the rest of the comments, those and the descriptions will be fixed for v3.
diff --git a/Documentation/devicetree/bindings/display/apple,h7-display-pipe-mipi.yaml b/Documentation/devicetree/bindings/display/apple,h7-display-pipe-mipi.yaml new file mode 100644 index 0000000000000000000000000000000000000000..2cf2f50e9fc7329a5b424d5ddf8c34cad2ebb6be --- /dev/null +++ b/Documentation/devicetree/bindings/display/apple,h7-display-pipe-mipi.yaml @@ -0,0 +1,89 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/apple,h7-display-pipe-mipi.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Apple pre-DCP display controller MIPI interface. + +maintainers: + - asahi@lists.linux.dev + - Sasha Finkelstein <fnkl.kernel@gmail.com> + +description: + The MIPI controller part of the pre-DCP Apple display controller + +allOf: + - $ref: dsi-controller.yaml# + +properties: + compatible: + items: + - enum: + - apple,t8112-display-pipe-mipi + - apple,t8103-display-pipe-mipi + - const: apple,h7-display-pipe-mipi + + reg: + maxItems: 1 + + reg-names: + const: mipi + + power-domains: + maxItems: 1 + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + properties: + port@0: + $ref: /schemas/graph.yaml#/properties/port + + port@1: + $ref: /schemas/graph.yaml#/properties/port + + required: + - port@0 + - port@1 + + '#address-cells': true + + '#size-cells': true + +patternProperties: + "^panel@[0-3]$": true + +required: + - compatible + - reg + - ports + +additionalProperties: false + +examples: + - | + display_dfr: dsi@228200000 { + compatible = "apple,t8103-display-pipe-mipi", "apple,h7-display-pipe-mipi"; + reg-names = "mipi"; + reg = <0x28200000 0xc000>; + power-domains = <&ps_dispdfr_mipi>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + dfr_mipi_in: port@0 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0>; + }; + + dfr_mipi_out: port@1 { + #address-cells = <1>; + #size-cells = <0>; + reg = <1>; + }; + }; + }; +... diff --git a/Documentation/devicetree/bindings/display/apple,h7-display-pipe.yaml b/Documentation/devicetree/bindings/display/apple,h7-display-pipe.yaml new file mode 100644 index 0000000000000000000000000000000000000000..98982da9c5f672167d67e4cd3b47e1fbdafc9510 --- /dev/null +++ b/Documentation/devicetree/bindings/display/apple,h7-display-pipe.yaml @@ -0,0 +1,77 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/apple,h7-display-pipe.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Apple pre-DCP display controller. + +maintainers: + - asahi@lists.linux.dev + - Sasha Finkelstein <fnkl.kernel@gmail.com> + +description: + A secondary display controller used to drive the "touchbar" on certain Apple laptops. + +properties: + compatible: + items: + - enum: + - apple,t8112-display-pipe + - apple,t8103-display-pipe + - const: apple,h7-display-pipe + + reg: + maxItems: 2 + + reg-names: + items: + - const: be + - const: fe + + power-domains: + maxItems: 2 + + interrupts: + maxItems: 2 + + interrupt-names: + items: + - const: be + - const: fe + + iommus: + maxItems: 1 + + port: + $ref: /schemas/graph.yaml#/properties/port + +required: + - compatible + - reg + - interrupts + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/apple-aic.h> + display_dfr: display-pipe@228200000 { + compatible = "apple,t8103-display-pipe", "apple,h7-display-pipe"; + reg-names = "be", "fe"; + reg = <0x28200000 0xc000>, + <0x28400000 0x4000>; + power-domains = <&ps_dispdfr_fe>, <&ps_dispdfr_be>; + interrupt-parent = <&aic>; + interrupts = <AIC_IRQ 502 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 506 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "be", "fe"; + iommus = <&displaydfr_dart 0>; + port { + dfr_adp_out_mipi: endpoint { + remote-endpoint = <&dfr_mipi_in_adp>; + }; + }; + }; +... diff --git a/Documentation/devicetree/bindings/display/panel/apple,summit.yaml b/Documentation/devicetree/bindings/display/panel/apple,summit.yaml new file mode 100644 index 0000000000000000000000000000000000000000..db14f7af3787076c84ccdda08fedeb8912d5514d --- /dev/null +++ b/Documentation/devicetree/bindings/display/panel/apple,summit.yaml @@ -0,0 +1,58 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/panel/apple,summit.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Apple "Summit" display panel. + +maintainers: + - asahi@lists.linux.dev + - Sasha Finkelstein <fnkl.kernel@gmail.com> + +allOf: + - $ref: panel-common.yaml# + - $ref: /schemas/leds/backlight/common.yaml# + +properties: + compatible: + items: + - enum: + - apple,j293-summit + - apple,j493-summit + - const: apple,summit + + reg: + maxItems: 1 + + max-brightness: true + + port: true + +required: + - compatible + - reg + - max-brightness + - port + +additionalProperties: false + +examples: + - | + dsi { + #address-cells = <1>; + #size-cells = <0>; + + panel@0 { + compatible = "apple,j293-summit", "apple,summit"; + reg = <0>; + max-brightness = <255>; + + port { + dfr_panel_in: endpoint { + remote-endpoint = <&dfr_bridge_out>; + }; + }; + }; + }; +...