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

From: Linus Walleij <linus.walleij@linaro.org>
To: Sarah Walker <sarah.walker@imgtec.com>
Cc: matthew.brost@intel.com,
	"Dr. H. Nikolaus Schaller" <hns@goldelico.com>,
	dri-devel@lists.freedesktop.org, christian.koenig@amd.com,
	luben.tuikov@amd.com, dakr@redhat.com, donald.robson@imgtec.com,
	boris.brezillon@collabora.com, sumit.semwal@linaro.org,
	faith.ekstrand@collabora.com
Subject: Re: [PATCH v3 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU
Date: Fri, 16 Jun 2023 14:48:54 +0200	[thread overview]
Message-ID: <CACRpkdaDkchJkP0MOOBWPtaBhfvR0OUEjkHAHgY0sv8T+SDdiQ@mail.gmail.com> (raw)
In-Reply-To: <20230613144800.52657-3-sarah.walker@imgtec.com>

Hi Sarah,

thanks for your patch!

On Tue, Jun 13, 2023 at 5:20 PM Sarah Walker <sarah.walker@imgtec.com> wrote:

> Add the device tree binding documentation for the Series AXE GPU used in
> TI AM62 SoCs.
>
> Signed-off-by: Sarah Walker <sarah.walker@imgtec.com>

> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - ti,am62-gpu
> +          - const: img,powervr-seriesaxe

I don't see why you need to restrict the bindings to just the stuff
you happen to
be writing Linux drivers for right now.

Add all of them!

There is this out-of-tree binding:
https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/0ddd308a78926782b8a72f75c74b91417ceb9779

With these two on top:
https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/053346e1933932815066f16ebf6e6bda45d67548
https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/1cb62c4cdcad096d438ee7d1d51f6001998acee3

They are indeed out-of-tree, but H. Nikolaus is a well-known and respected
contributor to the kernel, and I'm pretty sure he would be contributing
these upstream if he had the time and incentive.

To me it seems much more like you should talk to Nikolaus about submitting
these patches initially, and then add Rogue support with patches on top of it.
It could be a nice series with just DT bindings.

I see that your binding uses a power domain rather than a regulator
(sgx-supply) for power and that is probably a better approach but other
than that the openpvrsgx binding looks more complete and to the point?

It will also help them to get these bindings settled so they can merge all
of the DTS patches adding the SGX block to misc platforms in the
kernel upstream so they can focus their work on the actual driver.

When I look at the PowerVR wikipedia page:
https://en.wikipedia.org/wiki/PowerVR
there is no correspondence between the product names there and
"img,powervr-seriesaxe" as used here.

I think you need to explain if these are internal product names or what
is going on, and what the relationship is to the marketing names, it could
be part of the description simply, so that people know what string to use
somewhat intuitively. Maybe all the strings in the out-of-tree documentation
are just wrong because internal code names need to be used?

Yours,
Linus Walleij