Re: [PATCH v5 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Frank Binns <Frank.Binns@imgtec.com>]

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