| Message ID | 20230816082531.164695-3-sarah.walker@imgtec.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Imagination Technologies PowerVR DRM driver | expand |
Hi Sarah, thanks for your patch! Patches adding device tree bindings need to be CC:ed to devicetree@vger.kernel.org and the DT binding maintainers, I have added it for now. On Wed, Aug 16, 2023 at 10:26 AM Sarah Walker <sarah.walker@imgtec.com> wrote: > Add the device tree binding documentation for the Series AXE GPU used in > TI AM62 SoCs. > > Co-developed-by: Frank Binns <frank.binns@imgtec.com> > Signed-off-by: Frank Binns <frank.binns@imgtec.com> > Signed-off-by: Sarah Walker <sarah.walker@imgtec.com> (...) > +properties: > + compatible: > + items: > + - enum: > + - ti,am62-gpu > + - const: img,powervr-seriesaxe Should there not at least be a dash there? img,powervr-series-axe? It is spelled in two words in the commit message, Series AXE not SeriesAXE? Moreover, if this pertains to the AXE-1-16 and AXE-2-16 it is kind of a wildcard and we usually don't do that, I would use the exact version instead, such as: const: img,powervr-axe-1-16 any reason not to do this? I asked about the relationship between these strings and the product designations earlier I think :/ Yours, Linus Walleij
On 16/08/2023 10:25, Sarah Walker wrote: > Add the device tree binding documentation for the Series AXE GPU used in > TI AM62 SoCs. > > Co-developed-by: Frank Binns <frank.binns@imgtec.com> > Signed-off-by: Frank Binns <frank.binns@imgtec.com> > Signed-off-by: Sarah Walker <sarah.walker@imgtec.com> > --- > Changes since v4: > - Add clocks constraint for ti,am62-gpu Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. You missed at least DT list (maybe more), so this won't be tested by automated tooling. Performing review on untested code might be a waste of time, thus I will skip this patch entirely till you follow the process allowing the patch to be tested. Please kindly resend and include all necessary To/Cc entries. You already got this comment. I think more than once. Fix your processes, so finally this is resolved. Best regards, Krzysztof
On 18/08/2023 11:36, Linus Walleij wrote: > Hi Sarah, > > thanks for your patch! > > Patches adding device tree bindings need to be CC:ed to > devicetree@vger.kernel.org > and the DT binding maintainers, I have added it for now. > This won't help, I think. Patch will not be tested. I was already asking for using get_maintainers in the past... sigh... Best regards, Krzysztof
Hi Linus, Thank you for your feedback (comments below). On Fri, 2023-08-18 at 11:36 +0200, Linus Walleij wrote: > Hi Sarah, > > thanks for your patch! > > Patches adding device tree bindings need to be CC:ed to > devicetree@vger.kernel.org > and the DT binding maintainers, I have added it for now. Thank you - it looks like something went wrong when the patch was sent. > > On Wed, Aug 16, 2023 at 10:26 AM Sarah Walker <sarah.walker@imgtec.com> wrote: > > > Add the device tree binding documentation for the Series AXE GPU used in > > TI AM62 SoCs. > > > > Co-developed-by: Frank Binns <frank.binns@imgtec.com> > > Signed-off-by: Frank Binns <frank.binns@imgtec.com> > > Signed-off-by: Sarah Walker <sarah.walker@imgtec.com> > (...) > > +properties: > > + compatible: > > + items: > > + - enum: > > + - ti,am62-gpu > > + - const: img,powervr-seriesaxe > > Should there not at least be a dash there? > > img,powervr-series-axe? > > It is spelled in two words in the commit message, > Series AXE not SeriesAXE? We've now changed the string to address your earlier feedback (see below). > > Moreover, if this pertains to the AXE-1-16 and AXE-2-16 it is kind of a wildcard > and we usually don't do that, I would use the exact version instead, > such as: > const: img,powervr-axe-1-16 > any reason not to do this? The exact GPU model/revision is fully discoverable via a register. We saw the same is also true for Mali Bifrost, where they have a single string covering multiple models [1], so we took the same approach. We'll add a comment in v6 along the lines of the one in the Mali Bifrost bindings. > > I asked about the relationship between these strings and the product > designations earlier I think :/ Sorry about that, I honestly thought we'd addressed that bit of feedback by changing the compatibility string, but clearly we hadn't :( Thank you for catching this. We've now changed the string to "img,img-axe" to align with the marketing name, along with updating the commit message and various other places to refer to PowerVR and IMG GPUs (the DRM driver supporting both). Thanks Frank [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml?h=v6.5#n29
diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml b/Documentation/devicetree/bindings/gpu/img,powervr.yaml new file mode 100644 index 000000000000..40ade5ceef6e --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml @@ -0,0 +1,75 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (c) 2023 Imagination Technologies Ltd. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/gpu/img,powervr.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Imagination Technologies PowerVR GPU + +maintainers: + - Sarah Walker <sarah.walker@imgtec.com> + +properties: + compatible: + items: + - enum: + - ti,am62-gpu + - const: img,powervr-seriesaxe + + reg: + maxItems: 1 + + clocks: + minItems: 1 + maxItems: 3 + + clock-names: + items: + - const: core + - const: mem + - const: sys + minItems: 1 + + interrupts: + maxItems: 1 + + power-domains: + maxItems: 1 + +required: + - compatible + - reg + - clocks + - clock-names + - interrupts + +additionalProperties: false + +allOf: + - if: + properties: + compatible: + contains: + const: ti,am62-gpu + then: + properties: + clocks: + maxItems: 1 + clock-names: + const: core + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/soc/ti,sci_pm_domain.h> + + gpu: gpu@fd00000 { + compatible = "ti,am62-gpu", "img,powervr-seriesaxe"; + reg = <0x0fd00000 0x20000>; + clocks = <&k3_clks 187 0>; + clock-names = "core"; + interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>; + power-domains = <&k3_pds 187 TI_SCI_PD_EXCLUSIVE>; + }; diff --git a/MAINTAINERS b/MAINTAINERS index cd882b87a3c6..f84390bb6cfe 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10138,6 +10138,13 @@ IMGTEC IR DECODER DRIVER S: Orphan F: drivers/media/rc/img-ir/ +IMGTEC POWERVR DRM DRIVER +M: Frank Binns <frank.binns@imgtec.com> +M: Sarah Walker <sarah.walker@imgtec.com> +M: Donald Robson <donald.robson@imgtec.com> +S: Supported +F: Documentation/devicetree/bindings/gpu/img,powervr.yaml + IMON SOUNDGRAPH USB IR RECEIVER M: Sean Young <sean@mess.org> L: linux-media@vger.kernel.org