On Mon, Jan 30, 2017 at 2:54 PM, Eric Anholt eric@anholt.net wrote:
Rob Clark robdclark@gmail.com writes:
On Mon, Jan 30, 2017 at 1:09 PM, Eric Anholt eric@anholt.net wrote:
Rob Clark robdclark@gmail.com writes:
The original way we determined the gpu version was based on downstream bindings from android kernel. A cleaner way is to get the version from the compatible string.
Note that no upstream dtb uses these bindings. But the code still supports falling back to the legacy bindings (with a warning), so that we are still compatible with the gpu dt node from android device kernels.
The gpulist[] seems pretty short, like you could just have 7 compatible strings in dt_match[] and point them directly at corresponding the gpulist[] entry. Or are there lots of patch levels, with the same struct adreno_info values?
The list currently is rather incomplete (which may or may not matter, I guess it comes down to how many different phones/tablets out there people try to get an upstream kernel working on). And it has those ANY_ID wildcard entries.
A full list of all the gpu's including all their patchlevels would be quite long.
We might end up wanting to split the quirks out differently, since those tend to apply to specific patchlevel's.. I had to change the existing entry for a530 from a530.* to a530.2 for this reason. But that won't effect the bindings so that is an implementation detail we can worry about later.
I think that using the of_match_device() mechanism from the specific strings listed as the compatible would make more sense than this string parsing. You have to write a gpulist[] entry anyway for the module to load. So that means adding about 7 values today to the compatible string dt_match, and the code to use of_match_device() to get your struct adreno_info. Then the current version number lookup loop would be just for the old DT compatibility and there's no string parsing.
well, the problem is still that we would end up needing entries for each gpu version + patchlevel, which I was trying to avoid.. I think otherwise, if we started adding more adreno variants the table would get quite large. The ANY_ID entries keep the table size down currently.
However, it's your driver. The code seems correct, and using specific compatible strings is an obviously good step.
I may possibly re-work this in the future, but was leaning more towards perhaps introducing some sort of of_match_device_wildcard(id, dev, ...) type function, and allowing a compat string match like "qcom,adreno-%1u%1u%1u.%u"
I guess maybe re-arranging this so multiple compat table entries point back to the same 'struct adreno_info' might be workable, but that list could still grow quite long. At any rate, that doesn't change how the bindings look so that can come later.
Reviewed-by: Eric Anholt eric@anholt.net
thanks
BR, -R